-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix column offset tracking for UNIONs to be case insensitive.
#19139
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 column offset tracking for UNIONs to be case insensitive.
#19139
Conversation
Signed-off-by: Arthur Schreiber <[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 Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #19139 +/- ##
=======================================
Coverage 69.90% 69.91%
=======================================
Files 1613 1613
Lines 216076 216071 -5
=======================================
+ Hits 151055 151060 +5
+ Misses 65021 65011 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| needsFilter = true | ||
| cursor.StopTreeWalk() | ||
| return | ||
| panic(vterrors.VT13001(fmt.Sprintf("could not find the column '%s' on the UNION", sqlparser.String(col)))) |
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.
I'm not saying this is wrong, I don't know so asking. Is panic (crashing the vtgate) the right thing to do vs returning a query error?
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 a bit confusing, but panic inside the planner package does not crash the vtgate, it's the canonical way to return an error from inside the planner back to the vtgate.
Signed-off-by: Arthur Schreiber <[email protected]>
…ensitive. (#19139) (#19162) Signed-off-by: Arthur Schreiber <[email protected]> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com>
…ensitive. (#19139) (#19161) Signed-off-by: Arthur Schreiber <[email protected]> Co-authored-by: vitess-bot[bot] <108069721+vitess-bot[bot]@users.noreply.github.com> Co-authored-by: Arthur Schreiber <[email protected]>
Description
This is a followup to #19046.
It fixes an issue where
UNIONqueries couldn't push down predicates correctly when a column name in the firstSELECTof the union was using upper case characters while the predicate used lower case names. This caused an additionalFILTERoperation to be added to queries, which in turn could prevent route merging.This pull request fixes this issue by ensuring that both building the offset map and lookups into the offset map use lowercase column names.
This also fixes an issue introduced via #19046, where we added the join predicate unwrapping. This seemingly fixed the issue that pull request attempted to fix, but the fix broke down with the changes added in this PR - it caused all sorts of issues with
information_schemaqueries.Instead of unwrapping the
JoinPredicate, we create newJoinPredicateobjects for every source of theUNION. This allows the existing code to modify/restore theJoinPredicates pushed down into theUNIONs correctly in cases where those modifications need to be reverted (which can happen if LHS and RHS of anApplyJoinget merged).Related Issue(s)
Related to #19113
Checklist
Deployment Notes
AI Disclosure