Skip to content

Support TopN pushdown for text types in PostgreSQL#7494

Merged
findepi merged 1 commit intotrinodb:masterfrom
hashhar:hashhar/smarter-topn
Apr 8, 2021
Merged

Support TopN pushdown for text types in PostgreSQL#7494
findepi merged 1 commit intotrinodb:masterfrom
hashhar:hashhar/smarter-topn

Conversation

@hashhar
Copy link
Copy Markdown
Member

@hashhar hashhar commented Apr 4, 2021

This adds support for pushdown of text types to PostgreSQL by providing an explicit collation for textual columns in the generated SQL.

See also #7177 and #7022.

@hashhar
Copy link
Copy Markdown
Member Author

hashhar commented Apr 4, 2021

I still need to figure out where the predicate pushdown queries get generated so that I can provide an explicit collation there.

@hashhar hashhar force-pushed the hashhar/smarter-topn branch from 4c389fc to 487c6bc Compare April 5, 2021 06:08
@hashhar hashhar changed the title Support pushdown for text types in PostgreSQL Support TopN pushdown for text types in PostgreSQL Apr 5, 2021
@hashhar
Copy link
Copy Markdown
Member Author

hashhar commented Apr 5, 2021

I'm splitting this into multiple PRs to keep scope under control.

This PR just enables TopN pushdown for textual types in Postgres connector. I've created an issue #7496 to track this for other connectors and other operations (e.g. predicate pushdown, aggregation pushdown).

@hashhar hashhar force-pushed the hashhar/smarter-topn branch from 487c6bc to 313e2d1 Compare April 5, 2021 11:52
@hashhar hashhar requested a review from findepi April 5, 2021 11:52
@hashhar hashhar force-pushed the hashhar/smarter-topn branch from 313e2d1 to e5998e4 Compare April 5, 2021 12:40
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

((Function<String, String>) this::quoted).apply( :)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

you can remove OFFSET 0 ROWS now (it was needed for SQL Server)
or even switch back to simpler ORDER BY ... LIMIT ....

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Or we may want to model this as we did with Joins?

see SUPPORTS_JOIN_PUSHDOWN_WITH_VARCHAR_*EQUALITY

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If we can figure out a theoretical solution to allow connectors to participate in predicate pushdown query building I'd keep the override - too many similar enums will confuse implementers (and reviewers) and will make it more work to cleanup in future.

If we don't have a way to also fix predicate pushdown similar to how I fixed topn here then I'll add those new enums as you suggested.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

i don't have a short-term goal to make QueryBuilder more elastic, so it seems it's eiehter "too many enum options" or "a test override" for interim... of the two, it seems "too many enum options" is easier to maintain (and we already have the case with joins)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

picking the default value for the new option is key here -- if you do this right, only PostgreSQL tests will care about the change.

@hashhar
Copy link
Copy Markdown
Member Author

hashhar commented Apr 7, 2021

@findepi AC as fixups (will squash after review). PTAL.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't believe this will ever be done for all the connectors, so you can remove the TODO from here.
(add some info in the issue though)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this should default on a conjunction of SUPPORTS_TOPN_PUSHDOWN and SUPPORTS_PREDICATE_PUSHDOWN_WITH_VARCHAR_INEQUALITY

@hashhar hashhar force-pushed the hashhar/smarter-topn branch from b035d4b to 1d7f131 Compare April 8, 2021 09:44
@findepi findepi merged commit f19bf74 into trinodb:master Apr 8, 2021
@findepi findepi added this to the 355 milestone Apr 8, 2021
@findepi findepi added the enhancement New feature or request label Apr 8, 2021
@findepi findepi mentioned this pull request Apr 8, 2021
7 tasks
@hashhar hashhar deleted the hashhar/smarter-topn branch April 8, 2021 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed enhancement New feature or request

Development

Successfully merging this pull request may close these issues.

2 participants