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

Push down NOT, IS NULL, NOT IS NULL in PostgreSQL connector #11514

Merged
merged 2 commits into from
Mar 22, 2022
Merged

Push down NOT, IS NULL, NOT IS NULL in PostgreSQL connector #11514

merged 2 commits into from
Mar 22, 2022

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Mar 16, 2022

Description

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

Improvement.

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

Core query engine & PostgreSQL connector.

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

Enables pushdown of NOT expression and IS NULL, NOT IS NULL predicates in PostgreSQL connector

Related issues, pull requests, and links

#9506

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 `IS NULL`, `IS NOT NULL` by pushing predicate
  computation to the underlying database. ({issue}`issuenumber`)
* Improve performance of queries involving `NOT`  by pushing expression
  computation to the underlying database. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label Mar 16, 2022
@wendigo wendigo requested review from findepi and hashhar and removed request for findepi March 16, 2022 13:25
@wendigo wendigo requested a review from findepi March 16, 2022 13:27
@wendigo wendigo changed the title Pushdown NOT, IS NULL, NOT IS NULL in PostgreSQL connector Push down NOT, IS NULL, NOT IS NULL in PostgreSQL connector Mar 16, 2022
@findepi
Copy link
Member

findepi commented Mar 16, 2022

@wendigo can you please rebase on top of #11515 ?

@wendigo wendigo force-pushed the serafin/expression-pushdown-support branch from 5c9367b to c0bcdf7 Compare March 16, 2022 14:16
@wendigo
Copy link
Contributor Author

wendigo commented Mar 16, 2022

@findepi done

@findepi findepi mentioned this pull request Mar 16, 2022
Open
findepi
findepi reviewed Mar 16, 2022
View reviewed changes
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.

"Translate NOT, IS NULL, NOT IS NULL to connector expression(s)"

@wendigo wendigo force-pushed the serafin/expression-pushdown-support branch from b3eea04 to 74a07e5 Compare March 16, 2022 17:01
@wendigo wendigo requested a review from findepi March 16, 2022 17:02
hashhar
hashhar reviewed Mar 16, 2022
View reviewed changes
plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlClient.java Outdated
@Test
public void testConvertIsNotNull()
{
// c_varchar IS NULL
Copy link
Member

Choose a reason for hiding this comment

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

stale comment?

...n/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java Outdated
@Test
public void testIsNullPredicatePushdown()
{
assertThat(query("SELECT nationkey FROM nation WHERE name IS NULL"))
Copy link
Member

Choose a reason for hiding this comment

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

Let's also test with a column that actually contains NULLs.

Copy link
Member

Choose a reason for hiding this comment

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

Also IS NULL is possible to represent as a TupleDomain (none) so should we add an OR to ensure expression pushdown path?

@ebyhr ebyhr mentioned this pull request Mar 17, 2022
Merged
@wendigo wendigo force-pushed the serafin/expression-pushdown-support branch 2 times, most recently from c03af14 to 2f7bf47 Compare March 21, 2022 11:29
findepi
findepi reviewed Mar 21, 2022
View reviewed changes
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.

"Translate NOT, IS NULL, NOT IS NULL to connector expression(s)"

core/trino-main/src/test/java/io/trino/sql/planner/TestConnectorExpressionTranslator.java Outdated
}

@Test
public void testTranslateNotIsNull()
Copy link
Member

Choose a reason for hiding this comment

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

NotIsNull -> IsNotNull

findepi
findepi reviewed Mar 21, 2022
View reviewed changes
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.

"Implement NOT, IS NULL, NOT IS NULL pushdown in PostgreSQL connector"

...n/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java Outdated
"2, 'Bb'",
"1, NULL",
"2, NULL"))) {
assertThat(query("SELECT a_int FROM " + table.getName() + " WHERE NOT(a_varchar LIKE 'A%' OR a_int = 2)"))
Copy link
Member

Choose a reason for hiding this comment

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

NOT (a OR b) can be replaced with (NOT a) AND (NOT b).

The test still would test what it tests, but having NOT around LIKE only would make it simpler.
Please change.

@findepi
Copy link
Member

findepi commented Mar 21, 2022

LGTM % comments.

@findepi findepi force-pushed the serafin/expression-pushdown-support branch from 2f7bf47 to 3a20817 Compare March 21, 2022 14:48
@findepi
Copy link
Member

findepi commented Mar 21, 2022

(applied myself)

@assaf2 assaf2 self-requested a review March 21, 2022 17:42
@findepi findepi merged commit 5e500ef into trinodb:master Mar 22, 2022
@github-actions github-actions bot added this to the 375 milestone Mar 22, 2022
@findepi findepi mentioned this pull request Mar 22, 2022
Closed
@wendigo wendigo deleted the serafin/expression-pushdown-support branch March 22, 2022 09:31
@mosabua mosabua mentioned this pull request Mar 22, 2022
Merged
@hackeryang hackeryang mentioned this pull request Jan 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@findepi findepi findepi approved these changes

@assaf2 assaf2 assaf2 approved these changes

@hashhar hashhar hashhar approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
375
Development

Successfully merging this pull request may close these issues.
4 participants
@wendigo @findepi @assaf2 @hashhar