-
Notifications
You must be signed in to change notification settings - Fork 3k
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.
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
Document Top-N pushdown #8468
Document Top-N pushdown #8468
Conversation
Do we also want to document partial topN replacement by partial Limit for pre-sorted inputs? |
name := name:varchar:text | ||
id := id:integer:int4 | ||
|
||
Note within ``SOURCE`` fragment that the ``TableScan`` element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This depends on the connector though. The string comes from how the connector implemented ConnectorTableHandle#toString
.
The actual thing which lets you know pushdown happenned is the fact that there's no TopNNode or LimitNode in the plan - only a TableScan.
@mosabua can point to some other pushdown docs which can be consulted for the best way to word it. Maybe https://github.com/trinodb/trino/pull/8463/files#diff-3a35320d12c22f7d413cda275bad87d6467ea3482f35eda3f89c51af38c99e85R163-R165?
@lhofhansl yes, sure. Can you please point me to a concrete sample of a partial TOP N query? |
6fbec3d
to
575d3c1
Compare
@findinpath That feature was implemented in #6634. Do you need an example query? |
@lhofhansl yes, please do share a sample query for partial top N |
@findinpath Partial TopN example - trino/plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/BaseJdbcConnectorTest.java Lines 884 to 897 in 338425d
See trino/plugin/trino-hive/src/test/java/io/trino/plugin/hive/TestHiveConnectorTest.java Lines 7616 to 7640 in 5b785cf
trino/plugin/trino-phoenix5/src/test/java/io/trino/plugin/phoenix5/TestPhoenixConnectorTest.java Lines 514 to 528 in 5b785cf
I don't think you can use an just an example query, you'd need to include CREATE TABLE (since that's what will let Trino know that the table has pre-sorted data). |
👋 @findinpath - this PR is inactive and doesn't seem to be under development. If you'd like to continue work on this at any point in the future, feel free to re-open. cc @mosabua as well, we could potentially pick this up instead. |
575d3c1
to
ed5e3af
Compare
ed5e3af
to
4cab369
Compare
We should continue this with @findinpath .. can you review @colebow ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments. Technically accurate.
4cab369
to
187cae4
Compare
187cae4
to
b24f061
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % nits.
Thanks a lot for adding this.
@@ -271,3 +271,92 @@ FETCH FIRST N ROWS``. | |||
|
|||
Implementation and support is connector-specific since different data sources | |||
support different SQL syntax and processing. | |||
|
|||
Find two similar Top-N queries in the following sections. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe @mosabua can suggest some better opening statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, you can examine at two queries that implement a Top-N behavior in the following section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDYT? For example, you can find two queries to learn how to identify Top-N pushdown behavior in the following section.
b24f061
to
9aae77d
Compare
9aae77d
to
4262627
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hashhar can we get this merged? It looks good to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this slipped through cracks. LGTM
Resolves: #7786