Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Convert LIKE to ranges #3618

Merged
merged 6 commits into from
May 7, 2020
Merged

Convert LIKE to ranges #3618

merged 6 commits into from
May 7, 2020

Conversation

findepi
Copy link
Member

@findepi findepi commented May 4, 2020

No description provided.

@cla-bot cla-bot bot added the cla-signed label May 4, 2020
@findepi findepi requested a review from martint May 4, 2020 21:03
@findepi findepi force-pushed the like-a-pro branch 4 times, most recently from f4cf7f3 to f57fb93 Compare May 6, 2020 11:48
@findepi
Copy link
Member Author

findepi commented May 6, 2020

Added a plan test verifying the pushdown end-to-end.

martint
martint approved these changes May 7, 2020
View reviewed changes
Copy link
Member

@martint martint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments. My only concern is that this optimization may break if something upstream replaced LIKE with a function call (or if we get rid of LIKE from the IR -- which will certainly happen at some point)

presto-main/src/main/java/io/prestosql/type/LikeFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/type/LikeFunctions.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/sql/planner/DomainTranslator.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/sql/planner/DomainTranslator.java Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/sql/planner/DomainTranslator.java Outdated Show resolved Hide resolved
presto-main/src/main/java/io/prestosql/sql/planner/DomainTranslator.java Outdated Show resolved Hide resolved
@findepi
Copy link
Member Author

findepi commented May 7, 2020

My only concern is that this optimization may break if something upstream replaced LIKE with a function call

I agree, that's why i added a plan test that verifies things end-to-end, at the connector table handle level.

When we move away into more powerful predicate pushdowns, things like TupleDomain will gain "better versions of themselves", and this test (every such test) will need to be updated for the new API, but it will be still applicable to verify mechanics on the higher level. And we will be able to make educated decisions on any potential steps backwards if absolutely needed.

@findepi
Copy link
Member Author

findepi commented May 7, 2020

Applied

@martint
Copy link
Member

martint commented May 7, 2020

Ship it!

@findepi findepi merged commit 6aea881 into trinodb:master May 7, 2020
@findepi findepi deleted the like-a-pro branch May 7, 2020 21:25
@findepi findepi mentioned this pull request May 8, 2020
Closed
8 tasks
@findepi findepi added this to the 334 milestone May 8, 2020
@findepi findepi mentioned this pull request Jun 28, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martint martint martint approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
334
Development

Successfully merging this pull request may close these issues.
2 participants
@findepi @martint