Skip to content

Push down IF in PostgreSQL connector#11533

Closed
ebyhr wants to merge 2 commits intotrinodb:masterfrom
ebyhr:ebi/connector-expression-if
Closed

Push down IF in PostgreSQL connector#11533
ebyhr wants to merge 2 commits intotrinodb:masterfrom
ebyhr:ebi/connector-expression-if

Conversation

@ebyhr
Copy link
Member

@ebyhr ebyhr commented Mar 17, 2022 •
edited
Loading

Description

Push down IF in PostgreSQL connector.
Relates to #11699

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
* Improve performance of queries involving `IF` by pushing expression
  computation to the underlying database. ({issue}`11533`)

@cla-bot cla-bot bot added the cla-signed label Mar 17, 2022
@electrum
Copy link
Member

electrum commented Mar 17, 2022

Since IF is lazy, it would be good to test an expression that will fail if not lazy (maybe division by zero).

@ebyhr ebyhr force-pushed the ebi/connector-expression-if branch from 1fb872b to cb467cf Compare March 18, 2022 02:12
@findepi
Copy link
Member

findepi commented Mar 18, 2022

Since IF is lazy, it would be good to test an expression that will fail if not lazy (maybe division by zero).

AND is also lazy and is modeled as a function call in Connector Expressions.
We don't really expose laziness as a property.

cc @martint

@ebyhr ebyhr force-pushed the ebi/connector-expression-if branch from cb467cf to fa6db8b Compare March 23, 2022 00:40
@assaf2 assaf2 mentioned this pull request Mar 29, 2022
Closed
assaf2
assaf2 reviewed Mar 29, 2022
View reviewed changes
core/trino-main/src/main/java/io/trino/sql/planner/ConnectorExpressionTranslator.java Outdated
Copy link
Member

@assaf2 assaf2 Mar 29, 2022

Choose a reason for hiding this comment

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

I talked with @findepi about this, we're not sure if we should translate to $if or to $case as a private case. WDYT?

Copy link
Member Author

@ebyhr ebyhr Mar 29, 2022

Choose a reason for hiding this comment

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

What does "a private case" mean here?

@ebyhr ebyhr force-pushed the ebi/connector-expression-if branch from fa6db8b to 77a61ea Compare March 29, 2022 09:48
@assaf2
Copy link
Member

assaf2 commented Mar 29, 2022

Replying to: #11533 (comment)
I meant that IF can be converted to CASE (i.e. IF is a private case of CASE - see https://trino.io/docs/current/functions/conditional.html#if)
We decided to convert BETWEEN to $and(x >= y, x <= z) (see #11684 (comment))
So maybe we want to do a similar thing here.

assaf2
assaf2 approved these changes Mar 29, 2022
View reviewed changes
@ebyhr ebyhr force-pushed the ebi/connector-expression-if branch from 77a61ea to e3e99dc Compare March 29, 2022 12:23
@ebyhr ebyhr requested a review from findepi March 30, 2022 08:58
findepi
findepi reviewed Mar 31, 2022
View reviewed changes
plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlClient.java
Comment on lines +252 to +253
@Test(dataProvider = "testConvertComparisonDataProvider")
public void testConvertIf(ComparisonExpression.Operator operator)
Copy link
Member

@findepi findepi Mar 31, 2022

Choose a reason for hiding this comment

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

the <test-name>DataProvider is intended to be used by <test-name> test method only

why do you need it here?
just use equality

Copy link
Member Author

@ebyhr ebyhr Apr 1, 2022

Choose a reason for hiding this comment

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

I put it to catch future changes. I will just use equality.

findepi
findepi reviewed Mar 31, 2022
View reviewed changes
...n/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java
@Test
public void testIfPredicatePushdown()
{
assertThat(query("SELECT nationkey FROM nation WHERE IF(name = 'ALGERIA', true, false)"))
Copy link
Member

@findepi findepi Mar 31, 2022

Choose a reason for hiding this comment

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

IF(condition, true, false) can be simplified by the engine to condition.

IF(name = 'ALGERIA', regionkey, nationkey) = <regionekey of Algeria>

findepi
findepi reviewed Apr 5, 2022
View reviewed changes
core/trino-spi/src/main/java/io/trino/spi/expression/StandardFunctions.java
* $if is a function accepting 2 arguments - condition and trueValue, or 3 arguments - condition, trueValue and falseValue.
* Evaluates and returns true_value if condition is true, otherwise evaluates and returns false_value.
*/
public static final FunctionName IF_FUNCTION_NAME = new FunctionName("$if");
Copy link
Member

@findepi findepi Apr 5, 2022

Choose a reason for hiding this comment

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

@martint and I discussed this and the conclusion was to model IF via CASE.
see #11699 (comment) for more details.

@ebyhr
Copy link
Member Author

ebyhr commented Apr 28, 2022

Let me close and continue in #11699 as we need re-modeling.

@ebyhr ebyhr closed this Apr 28, 2022
@ebyhr ebyhr deleted the ebi/connector-expression-if branch April 28, 2022 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@findepi findepi findepi left review comments

+1 more reviewer

@assaf2 assaf2 assaf2 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ebyhr @electrum @findepi @assaf2