-
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
change AndExpr to contain arbitrary many predicates #16671
Conversation
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[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
|
Signed-off-by: Andres Taylor <[email protected]>
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 like it!
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
913f027
to
20f76ae
Compare
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
20f76ae
to
1de8b08
Compare
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Signed-off-by: Andres Taylor <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #16671 +/- ##
==========================================
+ Coverage 68.92% 68.94% +0.01%
==========================================
Files 1562 1564 +2
Lines 200941 201360 +419
==========================================
+ Hits 138497 138825 +328
- Misses 62444 62535 +91 ☔ View full report in Codecov by Sentry. |
Hello! 👋 This Pull Request is now handled by arewefastyet. The current HEAD and future commits will be benchmarked. You can find the performance comparison on the arewefastyet website. |
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 looks good to me. We also have a bunch of queries with AND (with or without ORs) in our vitess-tester tests, nice.
I would not necessarily expect anything significant out of the benchmarks, query planning is a fraction of the overall performance. |
The performance numbers don't look good enough to warrant merging this |
Description
During planning, we often want to have predicates as a slice and not in a tree structure. This PR changes the
AndExpr
to contain a slice if inner predicates instead ofLeft/Right
fields.This PR also moves the checking for constant values in predicates to the planning stage. No need to do these rewrites if the whole query will be sent to MySQL.
Summary of Performance Changes:
The recent changes introduced in this PR have resulted in mixed performance outcomes across various benchmarks:
These results reflect targeted optimizations and regressions across different query plans and scenarios, highlighting areas where the changes have improved or slightly degraded performance.
Details:
Related Issue(s)
Checklist
Deployment Notes