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.

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

Design for Same-Table-JOINs #263

Merged
merged 5 commits into from
May 10, 2023

Conversation

acarbonetto
Copy link

@acarbonetto acarbonetto commented May 3, 2023

Description

Added documentation for a same-table join proposal.

related: https://opensearch.org/docs/latest/field-types/join/

Issues Resolved

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Merging #263 (7510773) into integ-design-same-table-joins (1bda5fe) will not change coverage.
The diff coverage is n/a.

❗ Current head 7510773 differs from pull request most recent head 166a9ae. Consider uploading reports for the commit 166a9ae to get more accurate results

@@                       Coverage Diff                        @@
##             integ-design-same-table-joins     #263   +/-   ##
================================================================
  Coverage                            97.16%   97.16%           
  Complexity                            4120     4120           
================================================================
  Files                                  371      371           
  Lines                                10373    10373           
  Branches                               704      704           
================================================================
  Hits                                 10079    10079           
  Misses                                 287      287           
  Partials                                 7        7           
Flag Coverage Δ
sql-engine 97.16% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@acarbonetto acarbonetto marked this pull request as draft May 4, 2023 17:45
Signed-off-by: Andrew Carbonetto <[email protected]>

_Phase `*`_ denotes dependencies that must be completed prior to starting this work.

### 5.1 Release Schedule

Choose a reason for hiding this comment

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

Maybe a release schedule and priorities may be better stored in a github issue, and linked to in this document.

Copy link
Author

Choose a reason for hiding this comment

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

I'm going to keep this here... it's really to talk about what's a must have for the release.


The following diagram explains the proposed sequence to create JOIN request to OpenSearch

### 6.1 Logical Plan Changes - Presentation

Choose a reason for hiding this comment

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

Maybe we can change the push down OP state change to execute query or something like this. Push down happens during logical plan optimization:
LogicalPlanOptimizer.java#L67-L73
Starting from the deepest part of the Logical Plan Tree each LogicalPlan is optimized/pushed down. After push down most LogicalPlans are removed from the tree, except generally LogicalProject, a logical plan to instantiate an operator for post-processing, and the LogicalRelation will be replaced with the OpenSearchIndexScanBuilder. So we could remove LogicalFilter and LogicalSort from the middle composite state.

Copy link
Author

Choose a reason for hiding this comment

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

I see what you're saying. The "push down" terminology is a little ambiguous. I think what we're really talking about are the transitions between states. I've changes this to Build Query which I hope makes sense?

Copy link
Author

Choose a reason for hiding this comment

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

I think the optimize state also doesn't make sense. I've changed this to Create IndexScanBuilder to follow what's in query-optimizer-improvements.md.

Signed-off-by: Andrew Carbonetto <[email protected]>
@acarbonetto acarbonetto marked this pull request as ready for review May 10, 2023 16:09
Signed-off-by: Andrew Carbonetto <[email protected]>
@acarbonetto acarbonetto merged commit 055c21b into integ-design-same-table-joins May 10, 2023
@acarbonetto acarbonetto deleted the design-same-table-joins-v2 branch May 10, 2023 16:33
acarbonetto added a commit that referenced this pull request Jun 6, 2023
* Add opensearch-same-table-join-query.md

Signed-off-by: Andrew Carbonetto <[email protected]>

* Update opensearch-same-table-join-query.md for first-pass review

Signed-off-by: Andrew Carbonetto <[email protected]>

* Add same-table-joins ER relations

Signed-off-by: Andrew Carbonetto <[email protected]>

* Update document for review comments

Signed-off-by: Andrew Carbonetto <[email protected]>

---------

Signed-off-by: Andrew Carbonetto <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants