Introduce projection pushdown framework for JDBC connectors #22203

Praveen2112 · 2024-05-30T09:36:22Z

Description

This framework would allow us to pushdown specific functions closer to the JDBC datasource. We use this function to pushdown reverse function in case of PostgreSQL connector. In order to push down a specific function to the underlying JDBC datasource, we need two parts

Client should implement convertProjection
Write a ProjectFunctionRule specific to the function to be pushed down.

We also support partial pushdown of expression i.e LOWER(REVERSE(varchar_col)) will be translated into LOWER(synthetic_col_from_query)

Spl thanks to @SemionPar for inspiring me to use reverse function for this framework.

Additional context and related issues

Without this change when we try to run a query like this

explain select reverse(name) from nation

Current master

                                                               Query Plan
-----------------------------------------------------------------------------------------------------------------------------------------
 Trino version: testversion
 Fragment 0 [SOURCE]
     Output layout: [expr]
     Output partitioning: SINGLE []
     Output[columnNames = [_col0]]
     │   Layout: [expr:varchar(25)]
     │   Estimates: {rows: 25 (1.34kB), cpu: 0, memory: 0B, network: 0B}
     │   _col0 := expr
     └─ ScanProject[table = postgresql:tpch.nation tpch.nation columns=[name:varchar(25):varchar]]
            Layout: [expr:varchar(25)]
            Estimates: {rows: 25 (1.34kB), cpu: 1.34k, memory: 0B, network: 0B}/{rows: 25 (1.34kB), cpu: 1.34k, memory: 0B, network: 0B}
            expr := reverse(name)
            name := name:varchar(25):varchar

With this optimization

trino:tpch> explain select reverse(name) from nation;
                                                                    Query Plan
--------------------------------------------------------------------------------------------------------------------------------------------------
 Trino version: testversion
 Fragment 0 [SOURCE]
     Output layout: [pfgnrtd]
     Output partitioning: SINGLE []
     Output[columnNames = [_col0]]
     │   Layout: [pfgnrtd:varchar(25)]
     │   Estimates: {rows: ? (?), cpu: 0, memory: 0B, network: 0B}
     │   _col0 := pfgnrtd
     └─ TableScan[table = postgresql:Query[SELECT REVERSE("name") AS "_pfgnrtd_0" FROM "tpch"."nation"] columns=[_pfgnrtd_0:varchar(25):varchar]]
            Layout: [pfgnrtd:varchar(25)]
            Estimates: {rows: ? (?), cpu: ?, memory: 0B, network: 0B}
            pfgnrtd := _pfgnrtd_0:varchar(25):varchar

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
(x) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# PostgreSQL
* Pushdown `reverse` function to the datasource when applied as a projection expression

Praveen2112 · 2024-05-30T09:37:31Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

+        ImmutableSet.Builder<JdbcColumnHandle> columnsBuilder = ImmutableSet.builder();
+        ImmutableList.Builder<Assignment> assignmentBuilder = ImmutableList.builder();
+        ImmutableMap.Builder<ConnectorExpression, Variable> syntheticVariablesBuilder = ImmutableMap.builder();
+        ImmutableMap.Builder<String, ParameterizedExpression> columnExpressionsBuilder = ImmutableMap.builder();
+        for (ConnectorExpression child : projection.getChildren()) {
+            RewrittenExpression rewrittenExpression = rewriteExpression(
+                    session,
+                    nextSyntheticColumnId,
+                    child,
+                    assignments,
+                    translatedExpression);
+            nextSyntheticColumnId = rewrittenExpression.nextSyntheticColumnId;
+            columnsBuilder.addAll(rewrittenExpression.columnHandles);
+            assignmentBuilder.addAll(rewrittenExpression.assignments);
+            syntheticVariablesBuilder.putAll(rewrittenExpression.syntheticVariables);
+            columnExpressionsBuilder.putAll(rewrittenExpression.columnExpressions);


Too many objects might be created - I'm not sure if we could pass builder as arguments here.

huw0 · 2024-05-30T17:46:23Z

This PR is quite timely as I was looking at the possibility to do something similar today. Thanks @Praveen2112.

My primary use case is pushdown of JSON field extraction which causes a significant speedup to the overall query.

However ideally it'd be good to enable pushdown of all functions that the connector source supports where the expression matches that of Trino's own.

It'd be really helpful if function pushdown could be easily reusable between convertPredicate and convertProjection?
This adds some complexity so may be better as a separate PR but I wonder if it is worth early consideration.

...o-postgresql/src/main/java/io/trino/plugin/postgresql/rule/RewriteStringReverseFunction.java

ssheikin · 2024-06-07T10:21:54Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

+        ImmutableMap.Builder<String, ParameterizedExpression> columnExpressionsBuilder = ImmutableMap.builder();
+        Set<ConnectorExpression> translatedExpression = new HashSet<>();
+
+        if (isComplexExpressionPushdown(session)) {


This could be a first statement in the method, and if false - return Optional.empty();

I don't think we need to return Optional#empty as we need to additional changes for normal expression.

return Optional.empty(); if if (!newVariables.isEmpty()) { is false.
if (!newVariables.isEmpty()) { is true only if newVariables is populated.
newVariables is populated only if newVariablesBuilder is populated.
newVariablesBuilder is populated only when if (isComplexExpressionPushdown(session)) { is true.
What do I miss?

Okay !! I get it - Previously I was a bit confused as this code snippet was a part of applyProjection. Thanks for pointing it out.

ssheikin · 2024-06-07T10:30:25Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

+                nextSyntheticColumnId = rewrittenExpression.nextSyntheticColumnId;
+                newColumnsBuilder.addAll(rewrittenExpression.columnHandles);
+                assignmentBuilder.addAll(rewrittenExpression.assignments);
+                newVariablesBuilder.putAll(rewrittenExpression.syntheticVariables);
+                columnExpressionsBuilder.putAll(rewrittenExpression.columnExpressions);


no need in this assignments and builders. Just use rewrittenExpression

ssheikin · 2024-06-07T10:32:03Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

+        if (isComplexExpressionPushdown(session)) {
+            for (ConnectorExpression projection : projections) {
+                RewrittenExpression rewrittenExpression = rewriteExpression(session, nextSyntheticColumnId, projection, assignments, translatedExpression);
+                nextSyntheticColumnId = rewrittenExpression.nextSyntheticColumnId;


nextSyntheticColumnId -> nextSyntheticColumnId()

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

ssheikin · 2024-06-07T11:10:59Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

+        ImmutableList.Builder<Assignment> assignmentBuilder = ImmutableList.builder();
+        ImmutableMap.Builder<ConnectorExpression, Variable> newVariablesBuilder = ImmutableMap.builder();
+        ImmutableMap.Builder<String, ParameterizedExpression> columnExpressionsBuilder = ImmutableMap.builder();
+        Set<ConnectorExpression> translatedExpression = new HashSet<>();


I didn't grasp the PR fully yet, but for now it looks like this variable could have a little bit more ugly name, to gather more attention. E.g. translatedExpressionAccumulator

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

ssheikin · 2024-06-07T11:57:59Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

+        // If the parent expression cannot be translated try translating its argument
+        ImmutableSet.Builder<JdbcColumnHandle> columnsBuilder = ImmutableSet.builder();
+        ImmutableList.Builder<Assignment> assignmentBuilder = ImmutableList.builder();
+        ImmutableMap.Builder<ConnectorExpression, Variable> syntheticVariablesBuilder = ImmutableMap.builder();
+        ImmutableMap.Builder<String, ParameterizedExpression> columnExpressionsBuilder = ImmutableMap.builder();
+        for (ConnectorExpression child : projection.getChildren()) {
+            RewrittenExpression rewrittenExpression = rewriteExpression(
+                    session,
+                    nextSyntheticColumnId,
+                    child,
+                    assignments,
+                    translatedExpression);
+            nextSyntheticColumnId = rewrittenExpression.nextSyntheticColumnId;
+            columnsBuilder.addAll(rewrittenExpression.columnHandles);
+            assignmentBuilder.addAll(rewrittenExpression.assignments);
+            syntheticVariablesBuilder.putAll(rewrittenExpression.syntheticVariables);
+            columnExpressionsBuilder.putAll(rewrittenExpression.columnExpressions);
+        }
+
+        return new RewrittenExpression(
+                nextSyntheticColumnId,
+                syntheticVariablesBuilder.buildOrThrow(),
+                columnExpressionsBuilder.buildOrThrow(),
+                columnsBuilder.build(),
+                assignmentBuilder.build());
+    }


Looks like it can be extacted to a separate commit

ssheikin · 2024-06-07T11:59:15Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

+                assignmentBuilder.build());
+    }
+
+    public record RewrittenExpression(


Should we move it to the very bottom of the class?

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/projection/ProjectFunctionRule.java

ssheikin · 2024-06-07T12:07:44Z

skimmed

Praveen2112 · 2024-06-26T11:17:50Z

@ssheikin Thanks a lot for the review. AC

ebyhr · 2024-07-01T06:05:48Z

The change may cause regression when reverse function pushdown happens on non-varchar types (e.g. money type) in PostgreSQL.

Praveen2112 · 2024-07-01T06:38:58Z

The same can be restricted via rules right ? Reverse pushdown rule can be applied only on JdbcTypeHandle which corresponds to VARCHAR - Would it solve the issue ?

Praveen2112 · 2024-07-01T06:57:13Z

@ebyhr Thank you for pointing the regression. Have fixed it and also added a test case to verify it doesn't break for columns with special data type.

ssheikin

lgtm

ssheikin · 2024-07-01T10:16:58Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

+        ImmutableMap.Builder<ConnectorExpression, Variable> newVariablesBuilder = ImmutableMap.builder();
+        ImmutableMap.Builder<String, ParameterizedExpression> columnExpressionsBuilder = ImmutableMap.builder();
+
+        for (ConnectorExpression projection : ImmutableSet.copyOf(projections)) {


nit: Maybe it's cheaper to copy to ImmutableList, instead of Set, as projections is a list, which may be ImmutableList too, avoiding actual copying.

This would allow to skip processing redundant expression - if propagated

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

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

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

findinpath · 2024-07-01T13:45:56Z

@Praveen2112 would you mind adding more hands-on details on the description of the PR about the problem that you're solving?

Also helpful would be potentially to share an EXPLAIN plan of a query with and without your changes.

Praveen2112 · 2024-07-01T13:51:46Z

@findinpath Yeah sure !! Would add the EXPLAIN set of changes -

ssheikin · 2024-07-01T14:04:20Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

+
+        List<Assignment> outputAssignments = assignmentBuilder.build();
+
+        if (newVariables.isEmpty()) {


may be moved even upper.

Praveen2112 · 2024-07-01T14:13:42Z

@ssheikin Thanks for the review. AC

...o-postgresql/src/main/java/io/trino/plugin/postgresql/rule/RewriteStringReverseFunction.java

We use this framework for pushing down `REVERSE` function to PostgreSql connector. This framework doesn't support partial pushdown of expressions.

Praveen2112 · 2024-07-02T04:50:58Z

@ebyhr Thanks a lot for the review - AC

Praveen2112 · 2024-07-02T11:56:54Z

@findinpath I have updated the description with the changes in the plan as well

Praveen2112 requested review from wendigo, findepi, ssheikin, hashhar, krvikash, vlad-lyutenko and marcinsbd May 30, 2024 09:36

cla-bot bot added the cla-signed label May 30, 2024

Praveen2112 commented May 30, 2024

View reviewed changes

ssheikin reviewed Jun 7, 2024

View reviewed changes

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

ssheikin reviewed Jun 7, 2024

View reviewed changes

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

ssheikin reviewed Jun 7, 2024

View reviewed changes

lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/projection/ProjectFunctionRule.java Show resolved Hide resolved

Praveen2112 force-pushed the praveen/jdbc_projection_pushdown branch 3 times, most recently from 31d24f6 to 45a08c7 Compare June 26, 2024 11:16

Praveen2112 force-pushed the praveen/jdbc_projection_pushdown branch from 45a08c7 to 31c973c Compare June 27, 2024 12:26

Praveen2112 force-pushed the praveen/jdbc_projection_pushdown branch from 31c973c to 7735be0 Compare July 1, 2024 06:56

ssheikin approved these changes Jul 1, 2024

View reviewed changes

Praveen2112 force-pushed the praveen/jdbc_projection_pushdown branch from 7735be0 to 054aa3e Compare July 1, 2024 13:48

ssheikin reviewed Jul 1, 2024

View reviewed changes

Praveen2112 force-pushed the praveen/jdbc_projection_pushdown branch from 054aa3e to 6662a68 Compare July 1, 2024 14:13

ssheikin approved these changes Jul 1, 2024

View reviewed changes

ebyhr reviewed Jul 2, 2024

View reviewed changes

...o-postgresql/src/main/java/io/trino/plugin/postgresql/rule/RewriteStringReverseFunction.java Outdated Show resolved Hide resolved

...o-postgresql/src/main/java/io/trino/plugin/postgresql/rule/RewriteStringReverseFunction.java Outdated Show resolved Hide resolved

Praveen2112 added 2 commits July 2, 2024 10:20

Introduce projection pushdown framework for JDBC connectors

dd597a9

We use this framework for pushing down `REVERSE` function to PostgreSql connector. This framework doesn't support partial pushdown of expressions.

Add support for partial expressions in JDBC projection pushdown

696532c

Praveen2112 force-pushed the praveen/jdbc_projection_pushdown branch from 6662a68 to 696532c Compare July 2, 2024 04:50

ebyhr approved these changes Jul 2, 2024

View reviewed changes

Praveen2112 merged commit 3acbb30 into trinodb:master Jul 2, 2024
95 checks passed

github-actions bot added this to the 452 milestone Jul 2, 2024

colebow mentioned this pull request Jul 3, 2024

Add Trino 452 release notes #22573

Merged

SemionPar mentioned this pull request Jul 4, 2024

Push join into TableScan through Project #21607

Closed

2 tasks

krvikash mentioned this pull request Jul 18, 2024

Support cast projection pushdown in oracle #22728

Merged

krvikash mentioned this pull request Aug 6, 2024

Support integral cast projection pushdown in redshift #22951

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce projection pushdown framework for JDBC connectors #22203

Introduce projection pushdown framework for JDBC connectors #22203

Praveen2112 commented May 30, 2024 •

edited

Loading

Praveen2112 May 30, 2024

huw0 commented May 30, 2024 •

edited

Loading

ssheikin Jun 7, 2024

Praveen2112 Jun 26, 2024

ssheikin Jun 26, 2024 •

edited

Loading

Praveen2112 Jun 27, 2024

ssheikin Jun 7, 2024

ssheikin Jun 7, 2024

ssheikin Jun 7, 2024 •

edited

Loading

ssheikin Jun 7, 2024

ssheikin Jun 7, 2024

ssheikin commented Jun 7, 2024

Praveen2112 commented Jun 26, 2024

ebyhr commented Jul 1, 2024

Praveen2112 commented Jul 1, 2024

Praveen2112 commented Jul 1, 2024

ssheikin left a comment

ssheikin Jul 1, 2024

Praveen2112 Jul 1, 2024

findinpath commented Jul 1, 2024

Praveen2112 commented Jul 1, 2024

ssheikin Jul 1, 2024

Praveen2112 commented Jul 1, 2024

Praveen2112 commented Jul 2, 2024

Praveen2112 commented Jul 2, 2024


		List<Assignment> outputAssignments = assignmentBuilder.build();

		if (newVariables.isEmpty()) {

Introduce projection pushdown framework for JDBC connectors #22203

Introduce projection pushdown framework for JDBC connectors #22203

Conversation

Praveen2112 commented May 30, 2024 • edited Loading

Description

Additional context and related issues

Release notes

Choose a reason for hiding this comment

huw0 commented May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssheikin Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssheikin Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ssheikin commented Jun 7, 2024

Praveen2112 commented Jun 26, 2024

ebyhr commented Jul 1, 2024

Praveen2112 commented Jul 1, 2024

Praveen2112 commented Jul 1, 2024

ssheikin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findinpath commented Jul 1, 2024

Praveen2112 commented Jul 1, 2024

Choose a reason for hiding this comment

Praveen2112 commented Jul 1, 2024

Praveen2112 commented Jul 2, 2024

Praveen2112 commented Jul 2, 2024

Praveen2112 commented May 30, 2024 •

edited

Loading

huw0 commented May 30, 2024 •

edited

Loading

ssheikin Jun 26, 2024 •

edited

Loading

ssheikin Jun 7, 2024 •

edited

Loading