Skip to content

HAVING pushdown and more advanced aggregation pushdown in JDBC connectors#6667

Merged
findepi merged 5 commits intotrinodb:masterfrom
findepi:findepi/synthetic-relations
Jan 25, 2021
Merged

HAVING pushdown and more advanced aggregation pushdown in JDBC connectors#6667
findepi merged 5 commits intotrinodb:masterfrom
findepi:findepi/synthetic-relations

Conversation

@findepi
Copy link
Copy Markdown
Member

@findepi findepi commented Jan 20, 2021

for #6620

@findepi findepi added the enhancement New feature or request label Jan 20, 2021
@findepi findepi force-pushed the findepi/synthetic-relations branch from 5b388ea to dfd6219 Compare January 20, 2021 22:38
@cla-bot cla-bot bot added the cla-signed label Jan 20, 2021
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jan 20, 2021

I still need to go over the commits and see whether commit boundaries still make sense.
Still, I will appreciate any and all feedback you may have at this point.

@findepi findepi mentioned this pull request Jan 21, 2021
Merged
@findepi findepi force-pushed the findepi/synthetic-relations branch 2 times, most recently from b7f9fe2 to 003b894 Compare January 21, 2021 09:59
losipiuk
losipiuk reviewed Jan 21, 2021
View reviewed changes
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadata.java Outdated
Copy link
Copy Markdown
Member

@losipiuk losipiuk Jan 21, 2021

Choose a reason for hiding this comment

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

tf...?

Copy link
Copy Markdown
Member Author

@findepi findepi Jan 21, 2021

Choose a reason for hiding this comment

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

lo... ?

Copy link
Copy Markdown
Member

@kokosing kokosing Jan 21, 2021

Choose a reason for hiding this comment

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

_trino_generated_?

Copy link
Copy Markdown
Member

@losipiuk losipiuk Jan 21, 2021

Choose a reason for hiding this comment

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

Oh makes sense :P

Copy link
Copy Markdown
Contributor

@ssheikin ssheikin Jun 7, 2024

Choose a reason for hiding this comment

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

🤣

@findepi findepi mentioned this pull request Jan 21, 2021
Open
losipiuk
losipiuk reviewed Jan 21, 2021
View reviewed changes
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/QueryBuilder.java Outdated
Copy link
Copy Markdown
Member

@losipiuk losipiuk Jan 21, 2021

Choose a reason for hiding this comment

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

can o here raise name conflicts?

Copy link
Copy Markdown
Member Author

@findepi findepi Jan 21, 2021

Choose a reason for hiding this comment

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

i don't think it can, that's why i used a fixed thing.

losipiuk
losipiuk reviewed Jan 21, 2021
View reviewed changes
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadata.java Outdated
Copy link
Copy Markdown
Member

@losipiuk losipiuk Jan 21, 2021

Choose a reason for hiding this comment

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

I don't get this one :/

Copy link
Copy Markdown
Member Author

@findepi findepi Jan 21, 2021

Choose a reason for hiding this comment

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

the conditions here are a bit reordered (needed to be), so the diff is not very smart

losipiuk
losipiuk reviewed Jan 21, 2021
View reviewed changes
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/QueryBuilder.java Outdated
Copy link
Copy Markdown
Member

@losipiuk losipiuk Jan 21, 2021

Choose a reason for hiding this comment

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

It feels like you did not need to introduce Table and Query and already go with JdbcRelationHandle and JdbcQueryRelationHandle from the start.

Copy link
Copy Markdown
Member Author

@findepi findepi Jan 21, 2021

Choose a reason for hiding this comment

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

Correct, #6667 (comment), i need to clean this up

losipiuk
losipiuk reviewed Jan 21, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

I like the approach.

@findepi findepi mentioned this pull request Jan 21, 2021
Merged
kokosing
kokosing reviewed Jan 21, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@kokosing kokosing left a comment

Choose a reason for hiding this comment

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

just skimmed few first commits.

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/BaseMySqlIntegrationSmokeTest.java Outdated
// predicate over aggregation result
assertThat(query("SELECT regionkey, sum(nationkey) FROM nation GROUP BY regionkey HAVING sum(nationkey) = 77"))
.matches("VALUES (BIGINT '3', BIGINT '77')")
.isNotFullyPushedDown(FilterNode.class);
Copy link
Copy Markdown
Member

@kokosing kokosing Jan 21, 2021

Choose a reason for hiding this comment

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

I think we should have generic test like that, which could be reused for other connectors. Copy pasting test cases does not look like a good idea. Something like: io.trino.testing.AbstractTestDistributedQueries#testDataMappingSmokeTest ?

Copy link
Copy Markdown
Member Author

@findepi findepi Jan 22, 2021

Choose a reason for hiding this comment

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

let's not address this here

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/PreparedQuery.java Outdated
Copy link
Copy Markdown
Member

@kokosing kokosing Jan 21, 2021

Choose a reason for hiding this comment

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

Why it is serializable?

Copy link
Copy Markdown
Member Author

@findepi findepi Jan 22, 2021

Choose a reason for hiding this comment

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

because it's part of table handle

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcMetadata.java Outdated
Copy link
Copy Markdown
Member

@kokosing kokosing Jan 21, 2021

Choose a reason for hiding this comment

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

_trino_generated_?

@findepi findepi force-pushed the findepi/synthetic-relations branch from 8a7485a to 23a089d Compare January 22, 2021 10:39
This was referenced Jan 22, 2021
Merged
Merged
@findepi findepi force-pushed the findepi/synthetic-relations branch from 23a089d to 8d952ed Compare January 22, 2021 15:27
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jan 22, 2021

Rebased after the extracted pieces have been merged.

@losipiuk @kokosing PTAL & please resolve comments where you're good with my replies.

@findepi findepi force-pushed the findepi/synthetic-relations branch from 8d952ed to e5de3ac Compare January 22, 2021 16:04
@findepi findepi mentioned this pull request Jan 22, 2021
Merged
alexjo2144
alexjo2144 reviewed Jan 22, 2021
View reviewed changes
plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestJdbcQueryBuilder.java Outdated
Copy link
Copy Markdown
Member

@alexjo2144 alexjo2144 Jan 22, 2021

Choose a reason for hiding this comment

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

Does it make sense to have a test similar to this but where the tupleDomain contains a constraint on s? Or is that impossible?

Copy link
Copy Markdown
Member Author

@findepi findepi Jan 22, 2021

Choose a reason for hiding this comment

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

Good point. This wouldn't work today (or more precisely -- would depend on whether downstream database is SQL standard compliant or eg MySQL).
I can add some validation.

@findepi
Remove JdbcColumnHandle.isSynthetic
78c96cf 
This is just removal of some safety checks. In the follow-up commit, the
expressions are removed from `JdbcColumnHandle` and so the distinction
is going to be removed.
@findepi
Remove Presto reference
524c3dc
@findepi findepi force-pushed the findepi/synthetic-relations branch from dc2710f to deb2d51 Compare January 22, 2021 22:13
@findepi
Copy link
Copy Markdown
Member Author

findepi commented Jan 22, 2021

AC, PTAL
@losipiuk @kokosing @alexjo2144

@findepi
Represent query-based relation in JdbcTableHandle
1cc663c 
Now `JdbcTableHandle` can represent a table with  optional constraints
and aggregates (as it used to be), or a query.

Due to the fact how  is serialization of `ConnectorTableHandle` handled,
`JdbcTableHandle` cannot have subclasses, so the semantics are expressed
with a polymorphic field, `JdbcRelationHandle`.
@findepi
Accept PreparedQuery in QueryBuilder
781d40f
@findepi
Use Query-based table handles in JDBC connectors
2215497 
This changes how aggregation pushdown is modelled in `JdbcTableHandle`.
Instead of  `JdbcTableHandle.groupingSets` and
`JdbcColumnHandle.expression`, it is converted directly into a query.

This allows further pushdowns, e.g. predicate after aggregation.
@findepi findepi force-pushed the findepi/synthetic-relations branch from deb2d51 to 2215497 Compare January 22, 2021 22:29
@findepi findepi requested a review from losipiuk January 24, 2021 19:40
losipiuk
losipiuk reviewed Jan 25, 2021
View reviewed changes
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcNamedRelationHandle.java
}
JdbcNamedRelationHandle that = (JdbcNamedRelationHandle) o;
return Objects.equals(schemaTableName, that.schemaTableName)
// remoteTableName is not compared here, as required by TestJdbcTableHandle#testEquivalence TODO document why this is important
Copy link
Copy Markdown
Member

@losipiuk losipiuk Jan 25, 2021

Choose a reason for hiding this comment

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

Can you explain it alrady in this PR?

Copy link
Copy Markdown
Member Author

@findepi findepi Jan 25, 2021

Choose a reason for hiding this comment

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

Sorry, missed this. I think i know, but @electrum would do this better (he authored the test)

Copy link
Copy Markdown
Member

@losipiuk losipiuk Jan 25, 2021

Choose a reason for hiding this comment

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

Fine. It was not documented previoulsly :)

@findepi findepi merged commit 02b59cd into trinodb:master Jan 25, 2021
@findepi findepi deleted the findepi/synthetic-relations branch January 25, 2021 09:26
@findepi findepi added this to the 352 milestone Jan 25, 2021
This was referenced Jan 25, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@losipiuk losipiuk losipiuk approved these changes

@kokosing kokosing kokosing left review comments

@electrum electrum Awaiting requested review from electrum

@martint martint Awaiting requested review from martint

+2 more reviewers

@ssheikin ssheikin ssheikin left review comments

@alexjo2144 alexjo2144 alexjo2144 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed enhancement New feature or request

Milestone

352

Development

Successfully merging this pull request may close these issues.

5 participants

@findepi @losipiuk @ssheikin @alexjo2144 @kokosing