-
Notifications
You must be signed in to change notification settings - Fork 159
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
[FEAT] Support null equal safe join in SQL #3166
[FEAT] Support null equal safe join in SQL #3166
Conversation
CodSpeed Performance ReportMerging #3166 will not alter performanceComparing Summary
|
@advancedxy, #3161 is merged now, so feel free to ping me once this one has all conflicts resolved & ready for review! |
06471a2
to
cb10930
Compare
cb10930
to
0278ad1
Compare
@universalmind303 I think it's ready for review once the CI passes. |
@@ -41,6 +41,8 @@ impl OptimizerRule for EliminateCrossJoin { | |||
LogicalPlan::Join(Join { | |||
join_type: JoinType::Inner, | |||
join_strategy: None, | |||
// TODO: consider support eliminate cross join with null_equals_nulls | |||
null_equals_nulls: None, |
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.
Seems that I didn't cover this in my previous PR. I found elimate_cross_join
is not compatible with join with null equal safe yet when adding a test in the python side(a.k.a tests/sql/test_joins.py).
I simply disable this for now as it seems it might require a decent amount of work to support that.
right_on: right_keys, | ||
null_equals_nulls: None, |
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.
this is just style change, to align it with the struct definition.
0278ad1
to
f1e96f3
Compare
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.
Looks good. Thanks again @advancedxy
Thanks for reviewing and merging. It would be great if you could take a look at #3134 as well. |
One follow-up of #3161: add
on a <=> b
support for SQL's Join operation.