-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
fix: Flip order on right join #20358
Conversation
@@ -81,6 +81,18 @@ pub enum MaintainOrderJoin { | |||
RightLeft, | |||
} | |||
|
|||
impl MaintainOrderJoin { | |||
pub fn flip(&self) -> Self { |
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.
Can we set this to minimal need publicity.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #20358 +/- ##
==========================================
- Coverage 79.13% 79.11% -0.03%
==========================================
Files 1572 1572
Lines 219839 219929 +90
Branches 2462 2465 +3
==========================================
+ Hits 173970 173992 +22
- Misses 45301 45369 +68
Partials 568 568 ☔ View full report in Codecov by Sentry. |
@@ -1243,6 +1243,20 @@ def test_join_preserve_order_left() -> None: | |||
5, | |||
] | |||
|
|||
right_left = left.join(right, on="a", how="right", maintain_order="left").collect() |
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.
It feels a bit odd to have tests for right joins in the test_join_preserve_order_left
function, but oh well.
IMO the fact that the right join is implemented in terms of the left join is just a (clever) implementation detail.
Leaning on that was what led to this bug in the first place 😁
Fixes #20350
When doing a right join the dataframes were swapped and a left join was done. For this the maintain_order argument also needs to be flipped. Thanks @rodrigogiraoserrao for catching this.