-
Notifications
You must be signed in to change notification settings - Fork 22
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
Add RFC for JDBC Join Push down #32
base: main
Are you sure you want to change the base?
Add RFC for JDBC Join Push down #32
Conversation
* Add RFC for JDBC Join Push down --------- Co-authored-by: Ajas M <Ajas.M@ibm.com> Co-authored-by: Haritha K <HARITHA.K@ibm.com> Co-authored-by: Glerin <Glerin.Pinhero@ibm.com>
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.
Reviewed till high level design
RFC-0009-jdbc-join-push-down.md
Outdated
At present, when a query joins multiple tables, it creates a separate TableScanNode for each table. Each TableScanNode select all the records from that table. The join operation is then executed in-memory in Presto using a JOIN node by applying JoinCriteria, FilterPredicate and other criteria (like order by, limit, etc.). | ||
|
||
However, if the query joins tables from the same JDBC datasource, it would be more efficient to let the datasource handle the join instead of creating a separate TableScanNode for each table and joining them in Presto. If we "Push down" or send these joins to remote JDBC datasource it increases the query performance. i.e., decreases the query execution time. We have seen improvements from 3x to 10x. |
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.
it would be more efficient
This is not always true. Pushing down an expanding Join may be worse for performance.
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.
decreases the query execution time. We have seen improvements from 3x to 10x.
Can you reword this to be more specific and talk about the improvements in query latency, scanned data size, CPU/disk/network usage ?
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.
I have added a table which shows the data i got from the presto UI. I dont think it shows Disk and network usage..
Can you take a look ? @aaneja
|
||
## Proposed Implementation | ||
|
||
At present, if presto get a join query (from the CLI or UI) which is trying to join tables either from same datasource or from different datasource, it is received as a string formatted sql query. Presto validates the syntax and converts it to Query (Statement) object using presto parser and analyzer. This Query object is converted to presto internal reference architecture called Plan, using its logical and physical optimizers. Finally, this plan is executed by the executor. |
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.
IMO, instead of having the preamble here describing the basics of the parser and planner in this PR, and repeating them in multiple RFC's how about we create a dedicated section in https://prestodb.io/docs/current/ that we can link to ?
|
||
**5. Enable presto Join pushdown capabilities by setting the session flag optimizer_inner_join_pushdown_enabled = true.** | ||
|
||
## Low level Design |
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.
Please rework this section or remove it. A lot of this is implementation detail. What we should be highlighting is
- How we would group tables by connector using the same mechanism in ReorderJoins. Describe how we would limit the grouping to only join sources that are either
TableScan
orFilter<-TableScan
orProject<-Filter<-TableScan
- Describe the updates to the SPI to add a table handle or other data structure to represent a 'group' of individual table handles that are joined
- Describe how the connectors, and how the JDBC connector in particular can indicate that it wants to 'participate' in this creation of grouped-tables
- Describe how the JDBC connector will un-wrap these grouped-tables and build the SQL necessary to achieve the JOIN condition that it would use when querying the JDBC source
- Describe what the corner cases are - why we would need a way to add a table alias
Please create smaller sections for these as you see fit
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.
Additionally, please add a section dedicated to SPI changes - you can add classes, interfaces and types that are being added or removed, and add code-comments on them to describe how they would be used
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.
Removed some things in that section.. Working on the rest
Updating the RFC with the above review comments.. The main idea is to reduce the verbosity. This level of in depth details of the implementation is not required. I will make the modifications and make it easier to follow. |
50814df
to
a72a706
Compare
Remove in depth implementation of GroupInnerJoinsByConnector Optimizer Fix heading Added explain analyze outputs remove extra details added section header remove pics Added table for performance difference
a72a706
to
f1c2aee
Compare
Saved that user @Thanzeel-Hassan-IBM is from IBM |
Internal review done in this PR : #31