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

Use JDBC parameters in JDBC complex expression pushdown #16616

Merged
merged 2 commits into from
Mar 22, 2023
Merged

Use JDBC parameters in JDBC complex expression pushdown #16616

merged 2 commits into from
Mar 22, 2023

Conversation

findepi
Copy link
Member

@findepi findepi commented Mar 17, 2023

No description provided.

@cla-bot cla-bot bot added the cla-signed label Mar 17, 2023
@findepi findepi force-pushed the findepi/jdbc-param branch from 6194d47 to 8d21a48 Compare March 20, 2023 10:04
@findepi findepi force-pushed the findepi/jdbc-param branch from 8d21a48 to 25d1dc7 Compare March 20, 2023 11:01
@findepi
Copy link
Member Author

findepi commented Mar 20, 2023

ci / check-commit (0fea223476b591d2472b9f89bc3afe5ed8561b99) (pull_request) failure seems unrelated

Error:  Error downloading dependencies for project
Error:  The following artifacts could not be resolved: io.trino:trino-matching:jar:411-SNAPSHOT, io.trino:trino-main:jar:tests:411-SNAPSHOT, io.trino:trino-main:jar:411-SNAPSHOT, io.trino:trino-testing-kafka:jar:411-SNAPSHOT, io.trino:trino-testing:jar:411-SNAPSHOT, io.trino:trino-tpch:jar:411-SNAPSHOT, io.trino:trino-collect:jar:411-SNAPSHOT, io.trino:trino-spi:jar:411-SNAPSHOT, io.trino:trino-plugin-toolkit:jar:411-SNAPSHOT, io.trino:trino-spi:jar:tests:411-SNAPSHOT: Could not find artifact io.trino:trino-matching:jar:411-SNAPSHOT in sonatype-nexus-snapshots (https://oss.sonatype.org/content/repositories/snapshots)
Warning:  The following artifacts could not be resolved: io.trino:trino-matching:jar:411-SNAPSHOT, io.trino:trino-main:jar:tests:411-SNAPSHOT, io.trino:trino-main:jar:411-SNAPSHOT, io.trino:trino-testing-kafka:jar:411-SNAPSHOT, io.trino:trino-testing:jar:411-SNAPSHOT, io.trino:trino-tpch:jar:411-SNAPSHOT, io.trino:trino-collect:jar:411-SNAPSHOT, io.trino:trino-spi:jar:411-SNAPSHOT, io.trino:trino-plugin-toolkit:jar:411-SNAPSHOT, io.trino:trino-spi:jar:tests:411-SNAPSHOT: Could not find artifact io.trino:trino-matching:jar:411-SNAPSHOT in sonatype-nexus-snapshots (https://oss.sonatype.org/content/repositories/snapshots)

ignoring

cc @nineinchnick

@findepi findepi marked this pull request as ready for review March 20, 2023 12:03
@findepi
Copy link
Member Author

findepi commented Mar 20, 2023

CI #13199 and ci / check-commit again

@findepi findepi requested review from ebyhr and losipiuk March 20, 2023 21:22
hashhar
hashhar reviewed Mar 21, 2023
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

the API seems good, yet to review the changes in all the rewrite rules

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/ExpressionRewrite.java Outdated Show resolved Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultQueryBuilder.java Show resolved Hide resolved
wendigo
wendigo reviewed Mar 21, 2023
View reviewed changes
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/ExpressionRewrite.java Outdated Show resolved Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java
Stream.concat(constraintExpressions.stream(), splitPredicate.stream())
.collect(joining(") AND (", "(", ")")));
return Optional.of(new ParameterizedExpression(
Stream.concat(constraintExpressions.stream().map(ParameterizedExpression::expression), splitPredicate.stream())
Copy link
Contributor

Choose a reason for hiding this comment

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

This block is hard to read and follow. Can we make it easier to reason about?

Copy link
Member Author

Choose a reason for hiding this comment

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

this line was as-is here before, right?

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java
@@ -374,7 +375,7 @@ public Optional<AggregationApplicationResult<ConnectorTableHandle>> applyAggrega
newColumns.add(newColumn);
projections.add(new Variable(newColumn.getColumnName(), aggregate.getOutputType()));
resultAssignments.add(new Assignment(newColumn.getColumnName(), newColumn, aggregate.getOutputType()));
expressions.put(columnName, expression.get().getExpression());
expressions.put(columnName, new ParameterizedExpression(expression.get().getExpression(), expression.get().getParameters()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we introduce a factory method that will produce ParametrizedExpression from JdbcExpression?

Copy link
Member Author

Choose a reason for hiding this comment

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

is this the only place where this would be useful?

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/QueryParameter.java
@@ -92,4 +103,14 @@ public int hashCode()
{
return Objects.hash(jdbcType, type, value);
}

@Override
public String toString()
Copy link
Contributor

Choose a reason for hiding this comment

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

Separate commit?

...n/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/ParameterizedExpression.java Show resolved Hide resolved
...in/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/expression/RewriteVarcharConstant.java Show resolved Hide resolved
plugin/trino-oracle/src/main/java/io/trino/plugin/oracle/OracleClient.java Show resolved Hide resolved
@wendigo
Copy link
Contributor

wendigo commented Mar 21, 2023

I like this change very much. Thank you @findepi

@findepi findepi force-pushed the findepi/jdbc-param branch from 426182a to dc58b78 Compare March 21, 2023 21:27
hashhar
hashhar approved these changes Mar 22, 2023
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this.

LGTM % comment

...n/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/RewriteUnicodeVarcharConstant.java
Copy link
Member

Choose a reason for hiding this comment

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

For other reviewers: this is now un-needed since we use prepared statements and the driver will handle this part now. (It took me quite some time to understand this).

@findepi As part of this we should also add a test which verifies pushdown works for unicode literals instead of query failures.

Copy link
Member Author

Choose a reason for hiding this comment

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

@hashhar i am not introducing new functionality. What testing was RewriteUnicodeVarcharConstant.java covered with?

@findepi
Copy link
Member Author

findepi commented Mar 22, 2023

@kokosing ptal

@kokosing
Copy link
Member

ACK. I skimmed the code and I trust @hashhar review here.

@findepi findepi merged commit e3bbf6c into master Mar 22, 2023
@findepi findepi deleted the findepi/jdbc-param branch March 22, 2023 10:31
@findepi findepi mentioned this pull request Mar 22, 2023
Merged
@github-actions github-actions bot added this to the 411 milestone Mar 22, 2023
@colebow colebow mentioned this pull request Mar 29, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wendigo wendigo wendigo left review comments

@hashhar hashhar hashhar approved these changes

@vlad-lyutenko vlad-lyutenko Awaiting requested review from vlad-lyutenko

@ssheikin ssheikin Awaiting requested review from ssheikin

@jklamer jklamer Awaiting requested review from jklamer

@kokosing kokosing Awaiting requested review from kokosing

@ebyhr ebyhr Awaiting requested review from ebyhr

@losipiuk losipiuk Awaiting requested review from losipiuk

Assignees
No one assigned
Labels
cla-signed
Milestone
411
Development

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