-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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 dual merging in outer join queries #15959
Conversation
Signed-off-by: Manan Gupta <[email protected]>
Review ChecklistHello reviewers! 👋 Please follow this checklist when reviewing this Pull Request. General
Tests
Documentation
New flags
If a workflow is added or modified:
Backward compatibility
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15959 +/- ##
==========================================
- Coverage 68.40% 68.24% -0.17%
==========================================
Files 1556 1543 -13
Lines 195121 197592 +2471
==========================================
+ Hits 133479 134845 +1366
- Misses 61642 62747 +1105 ☔ View full report in Codecov by Sentry. |
Is this not a problem only with left join and that too when the dual table is on the left hand side of the join. |
I see the code follows this rule, it was missing in the PR description. |
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.
@harshit-gangal @systay Had to change quite a few things to get everything to work. Could I request you two to review it again?
A lot of code has changed since the approval.
Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
f0c6155
to
8d67079
Compare
Signed-off-by: Manan Gupta <[email protected]>
…ual query Signed-off-by: Manan Gupta <[email protected]>
Signed-off-by: Manan Gupta <[email protected]>
7cb04e8
to
84c968f
Compare
I think the milestone on this issue is possibly incorrect. This was merged to v21, but the milestone says v20. |
Description
This PR resolves the issue described in #15958.
The problem was that we were merging the dual query with the sharded route, and each of the shards ended up returning one row. We should not be merging dual queries into sharded routes. We can only merge them if the other side is a single shard routing.
Related Issue(s)
Checklist
Deployment Notes