-
Notifications
You must be signed in to change notification settings - Fork 4
Add pushdown on inner join that equals should not be null #811
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances inner join performance by adding pushdown filters to ensure join columns, when compared for equality, are not null. Key changes include updating the Check method in MergeJoinFindVisitor to be internal static for broader reuse, adding left/right pushdown conditions based on IS NOT NULL checks, and refactoring filter relation creation in JoinFilterPushdownVisitor.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/FlowtideDotNet.Core/Optimizer/MergeJoinFindVisitor.cs | Updated Check method accessibility to internal static to enable reuse |
src/FlowtideDotNet.Core/Optimizer/FIlterPushdown/JoinFilterPushdownVisitor.cs | Added pushdown logic for inner join keys and refactored filter relation construction |
Comments suppressed due to low confidence (2)
src/FlowtideDotNet.Core/Optimizer/FIlterPushdown/JoinFilterPushdownVisitor.cs:105
- [nitpick] Modifying the collection while iterating can be error-prone; consider building a new collection with the filtered expressions to improve readability and reduce potential iteration issues.
andFunctionScalar.Arguments.RemoveAt(i);
src/FlowtideDotNet.Core/Optimizer/MergeJoinFindVisitor.cs:22
- [nitpick] Changing the Check method to internal static improves reusability; please ensure this complies with overall design considerations, including any potential thread safety concerns.
internal static bool Check(JoinRelation joinRelation, Expression? expression, [NotNullWhen(true)] out DirectFieldReference? leftKey, [NotNullWhen(true)] out DirectFieldReference? rightKey)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Benchmark
Benchmark suite | Current: 94974e0 | Previous: 22d4209 | Ratio |
---|---|---|---|
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.InnerJoin |
310335266.6666667 ns (± 5560519.379518428 ) |
458826533.3333333 ns (± 10640413.19005047 ) |
0.68 |
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.LeftJoin |
497183170 ns (± 15587112.692794932 ) |
564994640 ns (± 28550338.329818636 ) |
0.88 |
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.ProjectionAndNormalization |
147668030 ns (± 12749678.858182184 ) |
169842820 ns (± 7144067.768715523 ) |
0.87 |
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.SumAggregation |
155192116.66666666 ns (± 4220818.031791941 ) |
171581250 ns (± 12415362.812660772 ) |
0.90 |
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.ListAggWithMapAggregation |
1858048100 ns (± 91553618.50820899 ) |
1982296640 ns (± 123349811.75190067 ) |
0.94 |
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.WindowSum |
359827280 ns (± 11920569.255031405 ) |
||
FlowtideDotNet.Benchmarks.Stream.StreamBenchmark.ListAggWithStructAggregation |
1479757100 ns (± 77255673.9240412 ) |
This comment was automatically generated by workflow using github-action-benchmark.
this should help performance if one is joining columns that contains null