misc: Move IndexJoinNode to SPI#26218
Conversation
Reviewer's GuideRefactored IndexJoinNode into the SPI module and updated all dependent code, serialization, and optimizers to recognize the relocated class, while replacing Guava ImmutableList usage with java.util unmodifiableList. Class diagram for relocated IndexJoinNode in SPIclassDiagram
class PlanNode {
<<abstract>>
+List<VariableReferenceExpression> getOutputVariables()
+List<PlanNode> getSources()
+<R, C> R accept(PlanVisitor<R, C> visitor, C context)
}
class JoinType
class RowExpression
class VariableReferenceExpression
class IndexJoinNode {
+JoinType type
+PlanNode probeSource
+PlanNode indexSource
+List<Pair<VariableReferenceExpression, VariableReferenceExpression>> criteria
+Optional<RowExpression> filter
+Optional<VariableReferenceExpression> probeHashVariable
+Optional<VariableReferenceExpression> indexHashVariable
+List<VariableReferenceExpression> getLookupVariables()
+List<PlanNode> getSources()
+List<VariableReferenceExpression> getOutputVariables()
+<R, C> R accept(PlanVisitor<R, C> visitor, C context)
}
PlanNode <|-- IndexJoinNode
IndexJoinNode o-- JoinType
IndexJoinNode o-- PlanNode : probeSource
IndexJoinNode o-- PlanNode : indexSource
IndexJoinNode o-- RowExpression : filter
IndexJoinNode o-- VariableReferenceExpression : probeHashVariable
IndexJoinNode o-- VariableReferenceExpression : indexHashVariable
IndexJoinNode o-- VariableReferenceExpression : criteria
Class diagram for PlanVisitor with new visitIndexJoin methodclassDiagram
class PlanVisitor {
+R visitJoin(JoinNode node, C context)
+R visitIndexJoin(IndexJoinNode node, C context)
+R visitSemiJoin(SemiJoinNode node, C context)
+R visitPlan(PlanNode node, C context)
}
class IndexJoinNode
PlanVisitor ..> IndexJoinNode : visitIndexJoin
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
77ec333 to
985aec0
Compare
There was a problem hiding this comment.
remove index join from internalPlanVisitor
There was a problem hiding this comment.
we should have some sanity check for this. It's very easy to miss.
There was a problem hiding this comment.
Removed from internalPlanVisitor
985aec0 to
9d4e93d
Compare
9d4e93d to
d0234e5
Compare
d0234e5 to
7852931
Compare
hantangwangd
left a comment
There was a problem hiding this comment.
Thanks @feilong-liu, lgtm!
7cd48bf to
5433959
Compare
|
Thanks @hantangwangd and @rschlussel for reviewing this PR. The prestocpp-linux-build / prestocpp-linux-build-engine (pull_request) CI job has been consistently failing due to disk full error, so I created another PR #26270 following a previous PR #24331 to save some disk space, can I get a review for that PR too? Thanks! |
…inux-build-engine job (#26270) ## Description Reduce disk consumption for prestocpp-linux-build-engine. I have a PR #26218 which consistently fail due to disk full. Similar to #24331, this PR further remove files which are not used in the CI build. ## Motivation and Context As in description. ## Impact Fix failed CI test ## Test Plan The CI test completes successfully https://github.com/prestodb/presto/actions/runs/18389979579/job/52397841783?pr=26270 ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [ ] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [ ] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [ ] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. If release note is NOT required, use: ``` == NO RELEASE NOTE == ```
5433959 to
38ec074
Compare
38ec074 to
c824980
Compare
|
Replaced by #26289 |
…inux-build-engine job (prestodb#26270) ## Description Reduce disk consumption for prestocpp-linux-build-engine. I have a PR prestodb#26218 which consistently fail due to disk full. Similar to prestodb#24331, this PR further remove files which are not used in the CI build. ## Motivation and Context As in description. ## Impact Fix failed CI test ## Test Plan The CI test completes successfully https://github.com/prestodb/presto/actions/runs/18389979579/job/52397841783?pr=26270 ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [ ] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [ ] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [ ] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. If release note is NOT required, use: ``` == NO RELEASE NOTE == ```
## Description As title, move IndexJoinNode to SPI so that it's available in ApplyConnectorOptimizer The original PR #26218 always run out of disk space for job `prestocpp-linux-build / prestocpp-linux-build-engine`, even after multiple attempts to save disk space #26276 and #26270 A fresh build, i.e. without cache for branch, seems to solve this problem, so create a new PR with the same change. ## Motivation and Context In description. ## Impact In description. ## Test Plan In description. ## Contributor checklist - [ ] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [ ] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [ ] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [ ] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [ ] Adequate tests were added if applicable. - [ ] CI passed. ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. If release note is NOT required, use: ``` == NO RELEASE NOTE == ```
Description
As title, move IndexJoinNode to SPI so that it's available in ApplyConnectorOptimizer
Motivation and Context
As title
Impact
In description.
Test Plan
Easy change, existing unit tests
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.