Translate IN predicate to connector expression by wendigo · Pull Request #11396 · trinodb/trino

wendigo · 2022-03-09T14:53:01Z

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

( ) 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`)

core/trino-spi/src/main/java/io/trino/spi/expression/StandardFunctions.java

findepi · 2022-03-09T15:03:31Z

core/trino-main/src/main/java/io/trino/sql/planner/ConnectorExpressionTranslator.java

I think we need to think about efficiency of the representation, since IN list can be long and can be processed multiple times. This is the same problem as with large Domains, and with same performance challenges. See 82e8f93 and cc @raunaqmorarka

Now, I am aware that InListExpression doesn't have an efficient representation, but it can be evolved to have one. With ConnectorExpressions, such an evolution would be an SPI breaking change, so I'd suggest we consider performance in the first version already.

cc @martint

@martint any thoughts here?

core/trino-main/src/main/java/io/trino/sql/planner/ConnectorExpressionTranslator.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteIn.java

core/trino-main/src/main/java/io/trino/sql/planner/ConnectorExpressionTranslator.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteIn.java

findepi

"Translate IN predicate to connector expression" LGTM

core/trino-spi/src/main/java/io/trino/spi/expression/ConnectorExpressions.java

core/trino-spi/src/main/java/io/trino/spi/expression/StandardFunctions.java

...lugin-toolkit/src/main/java/io/trino/plugin/base/expression/ConnectorExpressionPatterns.java

findepi

"Rewrite connector IN expression in PostgreSQL connector"

...lugin-toolkit/src/main/java/io/trino/plugin/base/expression/ConnectorExpressionPatterns.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteIn.java

...n/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java

core/trino-main/src/main/java/io/trino/sql/planner/ConnectorExpressionTranslator.java

core/trino-spi/src/main/java/io/trino/spi/expression/ConnectorExpressions.java

plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlClient.java

wendigo · 2022-03-17T18:39:18Z

@findepi PTAL

wendigo · 2022-03-24T13:30:40Z

Rebased & resolved conflicts. PTAL @findepi

findepi · 2022-03-24T20:14:03Z

@wendigo did the CI run? seems not. can you add empty / rebase?

wendigo · 2022-03-28T07:53:18Z

@findepi fixed, after merge checkstyle violation :(

...bc/src/main/java/io/trino/plugin/jdbc/expression/JdbcConnectorExpressionRewriterBuilder.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteIn.java

plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlClient.java

...n/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java

core/trino-main/src/main/java/io/trino/sql/planner/ExpressionInterpreter.java

core/trino-main/src/test/java/io/trino/sql/TestExpressionInterpreter.java

findepi · 2022-04-01T08:09:33Z

AFAIU the "Improve NULL handling in IN predicates optimizations" commit prevents some problems/regressions that would be introduced by previous commits. is it correct?

wendigo · 2022-04-01T18:04:53Z

All tests are passing. Seems in a good shape for next round of the review @findepi

wendigo · 2022-04-08T13:10:34Z

@findepi rebased and solved conflicts. please review

findepi

Please help me understand the purpose and the mechanics of "Improve NULL handling in IN predicates optimizations" commit

core/trino-main/src/main/java/io/trino/sql/planner/ExpressionInterpreter.java

core/trino-main/src/test/java/io/trino/sql/TestExpressionInterpreter.java

core/trino-main/src/main/java/io/trino/sql/planner/ExpressionInterpreter.java

...in/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteVarcharConstant.java

core/trino-spi/src/main/java/io/trino/spi/statistics/StatsUtil.java

wendigo · 2022-05-06T14:28:38Z

@findepi PTAL

findepi

LGTM except of "Rewrite typed NULL char/varchar constants".

I'd suggest dropping this commit.

...in/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteVarcharConstant.java

...bc/src/main/java/io/trino/plugin/jdbc/expression/JdbcConnectorExpressionRewriterBuilder.java

findepi · 2022-05-09T07:48:39Z

...bc/src/main/java/io/trino/plugin/jdbc/expression/JdbcConnectorExpressionRewriterBuilder.java

+        return addStandardRules(identifierQuote, type -> Optional.empty());
+    }
+
+    public JdbcConnectorExpressionRewriterBuilder addStandardRules(Function<String, String> identifierQuote, Function<Type, Optional<String>> typeMapping)


This is simple for PostgreSQL and varchar case, but generally this is tricky, since remote database and Trino have different type systems.

In case of

where x IN (a, b, c, NULL)

the NULL should be eliminated on the engine side (generic engine rule).

Do we have other cases where this is actually useful?

x IN (NULL) is always false, right? Even for NULL IN (NULL) so eliminating NULLs is safe?

also x IN () (empty list) is also false

x IN (NULL) is always false, right?

result is NULL

also x IN () (empty list) is also false

This is not allowed in SQL, but it it was allowed, it would be false.

the NULL should be eliminated on the engine side (generic engine rule).

BTW such rule, if written, should go in separate PR.

in this PR please keep the tests exercising IN pushdown with NULLs (asserting that these doesn't get pushed down).

findepi · 2022-05-09T07:51:06Z

plugin/trino-postgresql/src/main/java/io/trino/plugin/postgresql/PostgreSqlClient.java

+    {
+        if (type instanceof VarcharType) {
+            VarcharType varcharType = (VarcharType) type;
+            return varcharType.getLength().map(length -> format("varchar(%d)", length)).or(() -> Optional.of("varchar"));


Trino bounded varchar can be of length 2147483646, PostgreSQL has a different limit

findepi · 2022-05-09T07:52:58Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteCast.java

+                .with(functionName().equalTo(CAST_FUNCTION_NAME))
+            .with(argument(0).capturedAs(ARGUMENT));


findepi · 2022-05-09T07:55:29Z

...n/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlConnectorTest.java

+                        "3, 'c'",
+                        "4, 'd'",
+                        "5, 'f'"))) {
+            assertThat(query("SELECT id FROM " + table.getName() + " WHERE CAST(id AS VARCHAR(1)) = '2' OR id2 = 'd'"))


Trino and PostgreSQL have different CAST behavior for BIGINT -> varchar(1) cast.

trino> SELECT CAST(BIGINT '12' AS varchar(1)); Query 20220509_075440_00000_3ce4u failed: Value 12 cannot be represented as varchar(1)

PostgreSQL

SELECT CAST(BIGINT '12' AS varchar(1)); 1

Postgres ended up going the Hive way? Does the SQL spec say anything about this?

@hashhar see #552 (comment)

bitsondatadev · 2022-11-19T06:45:32Z

👋 @ping - this PR is inactive and doesn't seem to be under development, and it might already be implemented in #13136. If you'd like to continue work on this at any point in the future, feel free to re-open.

cla-bot bot added the cla-signed label Mar 9, 2022

wendigo requested review from assaf2, findepi and hashhar and removed request for findepi March 9, 2022 14:53

findepi reviewed Mar 9, 2022

View reviewed changes

wendigo force-pushed the serafin/in-list-connector-pushdown branch 2 times, most recently from f886581 to b44ef48 Compare March 10, 2022 17:41

wendigo requested a review from findepi March 10, 2022 18:01

assaf2 reviewed Mar 13, 2022

View reviewed changes

core/trino-main/src/main/java/io/trino/sql/planner/ConnectorExpressionTranslator.java Outdated Show resolved Hide resolved

grantatspothero reviewed Mar 14, 2022

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteIn.java Outdated Show resolved Hide resolved

findepi reviewed Mar 15, 2022

View reviewed changes

wendigo force-pushed the serafin/in-list-connector-pushdown branch from b44ef48 to d624b09 Compare March 15, 2022 12:25

wendigo requested a review from findepi March 15, 2022 12:33

findepi reviewed Mar 15, 2022

View reviewed changes

wendigo force-pushed the serafin/in-list-connector-pushdown branch from d624b09 to 871d94c Compare March 15, 2022 18:57

wendigo requested a review from findepi March 15, 2022 18:58

findepi reviewed Mar 16, 2022

View reviewed changes

plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlClient.java Outdated Show resolved Hide resolved

plugin/trino-postgresql/src/test/java/io/trino/plugin/postgresql/TestPostgreSqlClient.java Outdated Show resolved Hide resolved

wendigo force-pushed the serafin/in-list-connector-pushdown branch from 871d94c to 9138284 Compare March 16, 2022 11:25

wendigo requested review from assaf2, findepi and grantatspothero March 16, 2022 11:26

assaf2 approved these changes Mar 21, 2022

View reviewed changes

wendigo force-pushed the serafin/in-list-connector-pushdown branch 2 times, most recently from 01875b6 to e629253 Compare March 24, 2022 13:28

findepi reviewed Mar 28, 2022

View reviewed changes

findepi reviewed Apr 1, 2022

View reviewed changes

wendigo force-pushed the serafin/in-list-connector-pushdown branch from dd7cbb3 to a22f682 Compare April 1, 2022 11:17

wendigo requested a review from findepi April 1, 2022 11:18

wendigo force-pushed the serafin/in-list-connector-pushdown branch from ea916b7 to dd51415 Compare April 8, 2022 13:10

findepi requested changes Apr 8, 2022

View reviewed changes

core/trino-main/src/main/java/io/trino/sql/planner/ExpressionInterpreter.java Outdated Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/ExpressionInterpreter.java Outdated Show resolved Hide resolved

wendigo force-pushed the serafin/in-list-connector-pushdown branch 2 times, most recently from 5114d72 to 666be07 Compare April 15, 2022 08:10

findepi requested changes Apr 15, 2022

View reviewed changes

core/trino-main/src/test/java/io/trino/sql/TestExpressionInterpreter.java Outdated Show resolved Hide resolved

core/trino-main/src/main/java/io/trino/sql/planner/ExpressionInterpreter.java Outdated Show resolved Hide resolved

wendigo force-pushed the serafin/in-list-connector-pushdown branch from 666be07 to 6336f75 Compare April 22, 2022 11:48

wendigo requested a review from findepi April 22, 2022 11:51

findepi reviewed Apr 22, 2022

View reviewed changes

...in/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteVarcharConstant.java Outdated Show resolved Hide resolved

core/trino-spi/src/main/java/io/trino/spi/statistics/StatsUtil.java Outdated Show resolved Hide resolved

findepi force-pushed the serafin/in-list-connector-pushdown branch from d9cd56b to 9933b83 Compare April 22, 2022 14:02

wendigo added 4 commits May 6, 2022 16:26

Fix varchar rewrite for NULL literal

6c89d7c

Translate IN predicate to connector expression

c568122

Rewrite connector IN expression in PostgreSQL connector

4ad0dfd

Rewrite typed NULL char/varchar constants

94432ee

wendigo force-pushed the serafin/in-list-connector-pushdown branch from 9933b83 to 94432ee Compare May 6, 2022 14:27

Add explicit cast pushdown support to PostgreSQL

7390a25

findepi reviewed May 9, 2022

View reviewed changes

findepi requested changes May 9, 2022

View reviewed changes

assaf2 mentioned this pull request Jul 11, 2022

Translate IN predicate to connector expression - continue #13136

Merged

bitsondatadev closed this Nov 19, 2022

wendigo deleted the serafin/in-list-connector-pushdown branch January 21, 2025 11:55

		.with(functionName().equalTo(CAST_FUNCTION_NAME))
		.with(argument(0).capturedAs(ARGUMENT));

Conversation

wendigo commented Mar 9, 2022

Description

Related issues, pull requests, and links

Documentation

Release notes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wendigo commented Mar 17, 2022

Uh oh!

wendigo commented Mar 24, 2022

Uh oh!

findepi commented Mar 24, 2022

Uh oh!

wendigo commented Mar 28, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

findepi commented Apr 1, 2022

Uh oh!

wendigo commented Apr 1, 2022

Uh oh!

wendigo commented Apr 8, 2022

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wendigo commented May 6, 2022

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment