Skip to content

Improve IN list performance in ExpressionInterpreter#11902

Merged
sopel39 merged 3 commits intotrinodb:masterfrom
starburstdata:ks/do_not_Create
Apr 14, 2022
Merged

Improve IN list performance in ExpressionInterpreter#11902
sopel39 merged 3 commits intotrinodb:masterfrom
starburstdata:ks/do_not_Create

Conversation

@sopel39
Copy link
Member

@sopel39 sopel39 commented Apr 11, 2022

Description

Is this change a fix, improvement, new feature, refactoring, or other?

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@sopel39 sopel39 requested a review from findepi April 11, 2022 14:41
@cla-bot cla-bot bot added the cla-signed label Apr 11, 2022
@sopel39 sopel39 requested a review from wendigo April 11, 2022 14:42
@sopel39
Copy link
Member Author

sopel39 commented Apr 11, 2022

Part of #5950

@sopel39 sopel39 force-pushed the ks/do_not_Create branch 3 times, most recently from c8e5e39 to bbf7d17 Compare April 11, 2022 15:07
@sopel39
Copy link
Member Author

sopel39 commented Apr 11, 2022

Failed due to: #11825

@findepi
Copy link
Member

findepi commented Apr 13, 2022

Does this improve io.trino.sql.planner.BenchmarkPlanner#planLargeInQuery results?

cc @wendigo @martint @kasiafi

Copy link
Member

@findepi findepi left a comment

Choose a reason for hiding this comment

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

LGTM, just one question

@sopel39 sopel39 force-pushed the ks/do_not_Create branch 2 times, most recently from e0ff5e9 to 4fb87a6 Compare April 14, 2022 16:30
sopel39 added 2 commits April 14, 2022 18:32
benchmarks
before
planLargeInQuery 2695,225ms/op

after
planLargeInQuery 2525,273ms/op
before
planLargeInQuery 2525,273ms/op

after
planLargeInQuery 2260,965ms/op
@sopel39 sopel39 changed the title Do not create a new instance of InPredicate if it consists of literals Improve IN list performance in ExpressionInterpreter Apr 14, 2022
@sopel39
Copy link
Member Author

sopel39 commented Apr 14, 2022

I've removed preserving of InPredicate instance for now. Will introduce it back when ExpressionInterpreter results are cached

Planner benchmark results:

before
planLargeInQuery 2706,743ms/op

Skip interpreting of literals when IN value is unbounded:
planLargeInQuery 2525,273ms/op

Do not use inListCache when value is an expression
planLargeInQuery 2260,965ms/op

without IN optimization (return node)
planLargeInQuery 1929,047ms/op

@sopel39 sopel39 merged commit ef75c65 into trinodb:master Apr 14, 2022
@github-actions github-actions bot added this to the 378 milestone Apr 14, 2022
@sopel39 sopel39 mentioned this pull request Apr 14, 2022
@findepi findepi deleted the ks/do_not_Create branch April 14, 2022 20:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants