-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Improve UNION query merging
#18289
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
Improve UNION query merging
#18289
Conversation
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
|
UNION query merging
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18289 +/- ##
==========================================
- Coverage 67.48% 67.47% -0.02%
==========================================
Files 1603 1603
Lines 262337 262384 +47
==========================================
+ Hits 177034 177039 +5
- Misses 85303 85345 +42 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
go/vt/sqlparser/normalizer.go
Outdated
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 awkward - We can't use bval directly as the map key, because Values on querypb.BindVariable is a slice, and structs containing slices can't be used as map keys.
So I'm using MarshalVT in combination with string to generate a value that can be used as a key. This is probably not super-optimal, but 🤷
a8d2976 to
b94dd79
Compare
Signed-off-by: Arthur Schreiber <[email protected]>
b94dd79 to
5b6a8c2
Compare
Signed-off-by: Arthur Schreiber <[email protected]>
5b6a8c2 to
73dc771
Compare
Signed-off-by: Arthur Schreiber <[email protected]>
Signed-off-by: Arthur Schreiber <[email protected]>
Signed-off-by: Arthur Schreiber <[email protected]>
Signed-off-by: Arthur Schreiber <[email protected]>
Description
This PR adds two "naive" improvements to allow
UNIONqueries to be merged in more cases.The first change allows
UNIONsubqueries that useINconditions to be merged together correctly. It only works if tuples on the two subqueries we try to merge match exactly (same elements in the exact same order), or the tuple was normalized to the same bind variable (see the change below). This could be improved upon by doing a true "set" comparison (so that the order of values and duplicates don't affect the merge result).This probably should also be extended to allow merging queries where one side usesMergingfoobar IN (1)and the other side usesfoobar = 1.foobar IN (1)withfoobar = 1already works, so I just added a test case to make sure it's covered.This change is currently missing test coverage, but I'll make sure to add some.Various test cases to cover this change have been added.
The second change improves bind variable normalization so that bind variables for
ValTupleget de-duplicated, similar to other bind variables. That de-duplication only happens if the tuples contain exactly the same values (with matching types) in the same order.This could probably be improved upon by pre-sorting and de-duplicating the tuple values, to ensure that tuples like
(1, 2, 3)(1, 1, 2, 3)and(3, 2, 1)all end up with the same bind variable value (and thus allowUNIONqueries that use such tuples for routing to be merged).Related Issue(s)
#18288
Checklist
Deployment Notes