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.

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

Translate IN predicate to connector expression - continue #13136

Merged
merged 4 commits into from
Jul 29, 2022

Conversation

assaf2
Copy link
Member

@assaf2 assaf2 commented Jul 10, 2022

Description

Adds translation for InPredicate and InListExpression to connector expression form.

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

New feature.

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

Core engine & postgres connector.

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

This enabled pushdown of IN predicates which are not expressible with Domain (i.e. references other symbols or contains function calls)

Related issues, pull requests, and links

Documentation

(x) 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.
(x) Release notes entries required with the following suggested text:

# PostgreSQL connector
* Improve performance of queries involving `IN` expression by pushing predicate computation to the PostgreSQL database. (`#13136`)

@cla-bot cla-bot bot added the cla-signed label Jul 10, 2022
@assaf2 assaf2 force-pushed the in-list-connector-pushdown-2 branch from 70d2e3f to 03f1777 Compare July 11, 2022 11:06
@assaf2
Copy link
Member Author

assaf2 commented Jul 11, 2022

I've asked @wendigo to take over his PR #11396.
I've dropped "Rewrite typed NULL char/varchar constants" as @findepi suggested.
I've also dropped "Add explicit cast pushdown support to PostgreSQL" since it now seems to me non-mandatory to the PR and #11396 (comment) is a significant problem. I believe there might be more problematic use cases so IMO should be done on a separate PR.
I've addressed #11396 (comment).

@assaf2 assaf2 requested review from findepi and wendigo July 11, 2022 11:27
@assaf2 assaf2 marked this pull request as ready for review July 11, 2022 11:28
@assaf2 assaf2 changed the title In list connector pushdown 2 Translate IN predicate to connector expression - continue Jul 11, 2022
@findepi
Copy link
Member

findepi commented Jul 19, 2022

@assaf2 @wendigo is it supposed to supersede #11396?

@findepi
Copy link
Member

findepi commented Jul 20, 2022

@assaf2 @wendigo is it supposed to supersede #11396?

thanks for 👍 on that question, @wendigo .

@assaf2
Copy link
Member Author

assaf2 commented Jul 20, 2022

@assaf2 @wendigo is it supposed to supersede #11396?

IMO yes, the dropped commits:

  • "Rewrite typed NULL char/varchar constants"
  • "Add explicit cast pushdown support to PostgreSQL"

can be fixed and re-added on a separated PR, if @wendigo thinks they are still necessary

@assaf2 assaf2 force-pushed the in-list-connector-pushdown-2 branch from 36c6c89 to 584231a Compare July 20, 2022 13:42
@assaf2 assaf2 force-pushed the in-list-connector-pushdown-2 branch 13 times, most recently from efaca95 to f57e4e4 Compare July 28, 2022 11:30
@assaf2 assaf2 force-pushed the in-list-connector-pushdown-2 branch from f57e4e4 to 57fc12a Compare July 28, 2022 12:59
@findepi findepi merged commit c129b38 into trinodb:master Jul 29, 2022
@findepi
Copy link
Member

findepi commented Jul 29, 2022

thank you @wendigo @assaf2

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