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

Refactor QueryBuilder usage and implement *char join pushdown in PostgreSQL #10059

Merged
merged 13 commits into from
Feb 25, 2022
Merged

Refactor QueryBuilder usage and implement *char join pushdown in PostgreSQL #10059

merged 13 commits into from
Feb 25, 2022

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Nov 24, 2021

This is a preparatory step to be able to provide correct behavior when pushing down joins for case-(in)sensitive columns in the join condition comparisons

@cla-bot cla-bot bot added the cla-signed label Nov 24, 2021
@wendigo wendigo force-pushed the serafin/refactor-query-builder-v2 branch 2 times, most recently from 1ddb6c5 to f754a18 Compare November 29, 2021 20:04
@grantatspothero
Copy link
Contributor

Seems straightforward, as @hashhar said having an example implementation might help in visualizing why the change is needed.

@hashhar hashhar mentioned this pull request Dec 30, 2021
Closed
@wendigo wendigo force-pushed the serafin/refactor-query-builder-v2 branch from f754a18 to 1203bd4 Compare January 3, 2022 12:59
@wendigo wendigo changed the title Refactor QueryBuilder usage and open it for extension Refactor QueryBuilder usage and implement *char join pushdown in PostgreSQL Jan 3, 2022
@wendigo
Copy link
Contributor Author

wendigo commented Jan 3, 2022

Supersedes #10435

@wendigo
Copy link
Contributor Author

wendigo commented Jan 3, 2022

I've implemented *char join pushdown for PostgreSQL

@wendigo wendigo force-pushed the serafin/refactor-query-builder-v2 branch from 1203bd4 to 1e76b97 Compare January 3, 2022 18:16
@wendigo wendigo force-pushed the serafin/refactor-query-builder-v2 branch 2 times, most recently from 1d30723 to 84f31ae Compare January 4, 2022 11:28
hashhar
hashhar reviewed Jan 5, 2022
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.

Nice. This would also allow us to accomodate other collation-sensitive pushdowns like predicates as well in a single place.

...in/trino-postgresql/src/main/java/io/trino/plugin/postgresql/CollationAwareQueryBuilder.java Show resolved Hide resolved
...in/trino-postgresql/src/main/java/io/trino/plugin/postgresql/CollationAwareQueryBuilder.java Outdated Show resolved Hide resolved
...in/trino-postgresql/src/main/java/io/trino/plugin/postgresql/CollationAwareQueryBuilder.java Show resolved Hide resolved
@wendigo wendigo force-pushed the serafin/refactor-query-builder-v2 branch 2 times, most recently from d28b177 to 31562f7 Compare January 10, 2022 11:14
@wendigo
Copy link
Contributor Author

wendigo commented Jan 10, 2022

@hashhar @ebyhr PTAL

@hashhar
Copy link
Member

hashhar commented Jan 17, 2022

Maybe squash " Pass join relation aliases as a variable " and " Extract join condition formatting to separate method " since the " Pass join relation aliases as a variable " doesn't seem to do anything by itself.

@hashhar
Copy link
Member

hashhar commented Jan 17, 2022

Squash " Move public methods to the top of the class " with " Open DefaultQueryBuilder methods for overloading "?

@hashhar
Copy link
Member

hashhar commented Jan 17, 2022

Why is " Pass ConnectorSession to isSupportedJoinCondition " needed? It's unclear from the commit history where/if it's being used?

hashhar
hashhar reviewed Jan 17, 2022
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 history is too fine-grained IMO. Makes following it a bit difficult.

The changes overall make sense.
I had a question - would just opening up the formatJoinCondition be sufficient for our case? The ability to provide different QueryBuilder seems like a hammer that can be easily misused/make code difficult to follow.

@wendigo
Copy link
Contributor Author

wendigo commented Jan 17, 2022

Why is " Pass ConnectorSession to isSupportedJoinCondition " needed? It's unclear from the commit history where/if it's being used?

https://github.com/trinodb/trino/pull/10059/files#diff-f0eab13f31619586b4c64f21d633a031ef482c6cbf37d628d6cad76bc6178cf2R802

hashhar
hashhar reviewed Jan 17, 2022
View reviewed changes
...in/trino-postgresql/src/main/java/io/trino/plugin/postgresql/CollationAwareQueryBuilder.java

if (isCollatable) {
return format(
"%s.%s COLLATE \"C\" %s %s.%s COLLATE \"C\"",
Copy link
Member

Choose a reason for hiding this comment

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

Maybe BaseJdbcClient should provide a method to fetch the collation name that matches Trino semantics?

It'll make this method more re-usable across databases since not every database has a C collation.

Also same for the collation syntax.

Or maybe this should be left for future when we notice we are adding CollationAwareQueryBuilder2?

@hashhar
Copy link
Member

hashhar commented Jan 17, 2022

Why is " Pass ConnectorSession to isSupportedJoinCondition " needed? It's unclear from the commit history where/if it's being used?

https://github.com/trinodb/trino/pull/10059/files#diff-f0eab13f31619586b4c64f21d633a031ef482c6cbf37d628d6cad76bc6178cf2R802

Ah, so it's needed for "Move collation-aware pushdown to query builder". Maybe it should be squashed together. 🙂

@wendigo
Copy link
Contributor Author

wendigo commented Jan 17, 2022

@hashhar that's correct. That's why I've made it extensible

hashhar
hashhar approved these changes Jan 17, 2022
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.

LGTM % some history re-ordering (or some squashes)

@ebyhr PTAL around the QueryBuilder extension.

ebyhr
ebyhr approved these changes Jan 18, 2022
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

It would be nice to keep past commit history of DefaultQueryBuilder (ex-QueryBuilder). I will defer the final merge to @hashhar.

Screen Shot 2022-01-18 at 18 40 46

@hashhar
Copy link
Member

hashhar commented Jan 18, 2022

@ebyhr Good point.

@wendigo Can you add a commit before "Pass QueryBuilder instance through the JdbcClient constructor" which just creates an unused exact copy of QueryBuilder as DefaultQueryBuilder so that the new file (DefaultQueryBuilder) has it's history preserved (since going forward it will be the new QueryBuilder impl.).

@wendigo wendigo force-pushed the serafin/refactor-query-builder-v2 branch from 31562f7 to c5d3cf3 Compare February 21, 2022 10:25
@wendigo
Copy link
Contributor Author

wendigo commented Feb 21, 2022

PTAL @hashhar @ebyhr

hashhar
hashhar approved these changes Feb 22, 2022
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.

Just some checkstyle fixes in "Pass QueryBuilder instance through the JdbcClient constructor"

It looks good to me otherwise.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultQueryBuilder.java Outdated Show resolved Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultQueryBuilder.java Show resolved Hide resolved
plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultQueryBuilder.java Outdated Show resolved Hide resolved
wendigo added 13 commits February 23, 2022 12:18
@wendigo wendigo force-pushed the serafin/refactor-query-builder-v2 branch from c5d3cf3 to 8c00d95 Compare February 23, 2022 11:23
@hashhar
Copy link
Member

hashhar commented Feb 24, 2022

CI hit #4948 and #10631.

cc: @grantatspothero Some Kudu flake?

@hashhar hashhar merged commit bf79840 into trinodb:master Feb 25, 2022
@hashhar hashhar mentioned this pull request Feb 25, 2022
Closed
@github-actions github-actions bot added this to the 372 milestone Feb 25, 2022
@mosabua mosabua mentioned this pull request Feb 25, 2022
Merged
@wendigo wendigo mentioned this pull request Aug 31, 2023
Closed
@wendigo wendigo deleted the serafin/refactor-query-builder-v2 branch August 31, 2023 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grantatspothero grantatspothero grantatspothero left review comments

@hashhar hashhar hashhar approved these changes

@ebyhr ebyhr ebyhr approved these changes

Assignees
No one assigned
Labels
cla-signed
Milestone
372
Development

Successfully merging this pull request may close these issues.
4 participants
@wendigo @grantatspothero @hashhar @ebyhr