-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Optimize BETWEEN predicate to include both the sides. #22521
Conversation
Can you fix the test @osscm ? |
@@ -88,7 +88,7 @@ private static SortExpressionContext merge(SortExpressionContext left, SortExpre | |||
} | |||
|
|||
private static class SortExpressionVisitor | |||
extends IrVisitor<Optional<SortExpressionContext>, Void> | |||
extends IrVisitor<Optional<List<SortExpressionContext>>, Void> |
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.
Optional
is no longer needed, just List<SortExpressionContext>
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.
updated
@@ -115,7 +115,7 @@ protected Optional<SortExpressionContext> visitComparison(Comparison comparison, | |||
hasBuildReferencesOnOtherSide = hasBuildSymbolReference(buildSymbols, comparison.right()); | |||
} | |||
if (sortChannel.isPresent() && !hasBuildReferencesOnOtherSide) { | |||
yield sortChannel.map(symbolReference -> new SortExpressionContext(symbolReference, singletonList(comparison))); | |||
yield sortChannel.map(symbolReference -> singletonList(new SortExpressionContext(symbolReference, singletonList(comparison)))); |
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.
nit: ImmutableList.of(...)
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.
updated
core/trino-main/src/main/java/io/trino/sql/planner/SortExpressionExtractor.java
Show resolved
Hide resolved
@osscm please squash these two commits together - they are a single logical change |
these 2 tests are failing, so trying to check, if there is any relation with the current PR changes. Rerun of the prb looks them good, could be the flaky one. |
Description
reviving this old PR
visitBetweenPredicate in SortExpressionVisitor can only handle one side of BETWEEN (GREATER_THAN_OR_EQUAL or LESS_THAN_OR_EQUAL), this pr optimize it and let it handle both sides of BETWEEN as conjunct expressions
Fixes one par of this issue: #22520
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: