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 in SortedPositionLinks #14598
base: master
Are you sure you want to change the base?
Optimize BETWEEN predicate in SortedPositionLinks #14598
Conversation
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: duripeng.
|
Change-Id: Ifb4434a1babe9205461492a3572f6ec4aa6368cd
feb32c1
to
17911aa
Compare
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
@WinkerDu could you file CLA? |
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.
lgtm % comments
@@ -24,6 +24,7 @@ | |||
import io.trino.sql.tree.Node; |
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.
Could you add a change description instead of Change-Id: Ifb4434a1babe9205461492a3572f6ec4aa6368cd
@@ -93,7 +95,7 @@ private static SortExpressionContext merge(SortExpressionContext left, SortExpre | |||
} | |||
|
|||
private static class SortExpressionVisitor | |||
extends AstVisitor<Optional<SortExpressionContext>, Void> | |||
extends AstVisitor<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.
Just List
is sufficient. We don't need to wrap it in Optional
anymore
@@ -120,7 +122,7 @@ protected Optional<SortExpressionContext> visitComparisonExpression(ComparisonEx | |||
hasBuildReferencesOnOtherSide = hasBuildSymbolReference(buildSymbols, comparison.getRight()); | |||
} | |||
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.
use ImmutableList.of()
@sopel39 Thanks for the CR, I've receive CLA reply yesterday, will push a new commit soon |
@WinkerDu are you still working on this? |
👋 @WinkerDu - this PR has become inactive. We hope you are still interested in working on it. Please let us know, and we can try to get reviewers to help with that. We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks. Alternatively @sopel39 did you want to check and run with this if necessary? |
We should land this PR, but it needs minor tweaks |
Description
visitBetweenPredicate
inSortExpressionVisitor
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 expressionsFixes #12096
Documentation
(x) Sufficient documentation is included in this PR.
Release notes
(x) Release notes are required, please propose a release note for me.