You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Riza Suminto (Code Review)" <ge...@cloudera.org> on 2024/11/08 20:10:29 UTC
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Riza Suminto has uploaded this change for review. ( http://gerrit.cloudera.org:8080/22046
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Within DistributedPlanner.java, there are several place where Planner
need to insert extra merge aggregation node. It require transferring
HAVING conjuncts from preaggregation node to merge aggregation,
unsetting limit, and recompute stats of preaggregation node. However,
the stats recompute is not consistently done, and there might be an
inefficient recompute happening.
This patch fix the order of AggregationNode creation order in
DistributedPlanner.java so that stats recomputation is done consistently
and efficiently.
Testing:
- Enable cardinality validation in testMultipleDistinct*
- Update aggregation.test to reflect current PlannerTest output.
Added some test cases as well.
- Pass core tests.
Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
M testdata/workloads/functional-planner/queries/PlannerTest/multiple-distinct-materialization.test
M testdata/workloads/functional-planner/queries/PlannerTest/multiple-distinct-predicates.test
7 files changed, 553 insertions(+), 160 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/22046/1
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 12:
> Patch Set 12:
>
> All tests pass except testFullAcidFileMetadataAfterMinorCompaction. I think it is flaky and unrelated to this patch.
>
> https://jenkins.impala.io/job/ubuntu-20.04-from-scratch/3996/testReport/junit/org.apache.impala.catalog.local/CatalogdMetaProviderTest/testFullAcidFileMetadataAfterMinorCompaction/
The flakiness might be related to IMPALA-12701. Wrote my analysis at
https://issues.apache.org/jira/browse/IMPALA-12701?focusedCommentId=17906871&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17906871
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 12
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 18 Dec 2024 23:06:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 11: Code-Review+2
Carry Michael's +1
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 11
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 18 Dec 2024 09:27:27 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22046
to look at the new patch set (#5).
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Within DistributedPlanner.java, there are several place where Planner
need to insert extra merge aggregation node. It require transferring
HAVING conjuncts from preaggregation node to merge aggregation,
unsetting limit, and recompute stats of preaggregation node. However,
the stats recompute is not consistently done, and there might be an
inefficient recompute happening.
This patch fixes the order of AggregationNode creation order in
DistributedPlanner.java so that stats recomputation is done consistently
and efficiently.
This patch also fixes a bug where the cardinality estimate of MERGE
phase aggregation is not capped against the output cardinality of
EXCHANGE node.
Skip tuple-based optimization if conjunct (HAVING predicate) ever
assigned to the AggregationNode. The optimization skip causes following
PlannerTest (under
testdata/workloads/functional-planner/queries/PlannerTest/) to revert
their cardinality estimation to their state pior to IMPALA-13405:
tpcds/tpcds-q39a.test
tpcds/tpcds-q39b.test
tpcds_cpu_cost/tpcds-q39a.test
tpcds_cpu_cost/tpcds-q39b.test
Testing:
- Enable cardinality validation in testMultipleDistinct*
- Update aggregation.test to reflect current PlannerTest output.
Added some test cases as well.
- Pass core tests.
Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
M testdata/workloads/functional-planner/queries/PlannerTest/multiple-distinct-materialization.test
M testdata/workloads/functional-planner/queries/PlannerTest/multiple-distinct-predicates.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds/tpcds-q39a.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds/tpcds-q39b.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds_cpu_cost/tpcds-q39a.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds_cpu_cost/tpcds-q39b.test
11 files changed, 1,362 insertions(+), 474 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/22046/5
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 4: Code-Review+1
(3 comments)
Just some nits.
http://gerrit.cloudera.org:8080/#/c/22046/4/fe/src/main/java/org/apache/impala/planner/AggregationNode.java
File fe/src/main/java/org/apache/impala/planner/AggregationNode.java:
http://gerrit.cloudera.org:8080/#/c/22046/4/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@223
PS4, Line 223: // to the proper place in the distributed plan via transferConjuncts().
Is this comment still accurate?
http://gerrit.cloudera.org:8080/#/c/22046/4/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java:
http://gerrit.cloudera.org:8080/#/c/22046/4/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@a1073
PS4, Line 1073:
I see you deprecated transferConjuncts.
It looks like what was happening is you did orderConjunctsByCost, then transferred additional conjuncts. And then didn't order again. So this both saves recomputing stats, and ensures all conjuncts are included in orderConjunctsByCost.
http://gerrit.cloudera.org:8080/#/c/22046/4/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@1055
PS4, Line 1055: List<Expr> havingPredicates = new ArrayList<>(node.conjuncts_);
nit: could you do
List<Expr> havingPredicates = node.conjuncts_;
node.conjuncts_ = new ArrayList<>();
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 4
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Comment-Date: Fri, 06 Dec 2024 20:18:48 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 12: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 12
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 18 Dec 2024 23:15:44 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22046/5/fe/src/main/java/org/apache/impala/planner/AggregationNode.java
File fe/src/main/java/org/apache/impala/planner/AggregationNode.java:
http://gerrit.cloudera.org:8080/#/c/22046/5/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@276
PS5, Line 276: // DistributedPlanner.java may transfer conjunct to merge phase aggregation later.
Were these part of the parent patch, and you've moved them to this one?
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 06 Dec 2024 21:36:09 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 7:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/22046/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22046/5//COMMIT_MSG@20
PS5, Line 20: Testing:
> I need to Enabling PlannerTestOption.VALIDATE_CARDINALITY in some test to v
Moved to https://gerrit.cloudera.org/c/22184.
http://gerrit.cloudera.org:8080/#/c/22046/5/fe/src/main/java/org/apache/impala/planner/AggregationNode.java
File fe/src/main/java/org/apache/impala/planner/AggregationNode.java:
http://gerrit.cloudera.org:8080/#/c/22046/5/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@276
PS5, Line 276: // DistributedPlanner.java may transfer conjunct to merge phase aggregation later.
> I moved them here since the fix share the same theme with this patch: trans
Moved to https://gerrit.cloudera.org/c/22184
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Sat, 07 Dec 2024 00:16:27 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17421/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 1
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Nov 2024 20:56:16 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 11:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/22046/9//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22046/9//COMMIT_MSG@9
PS9, Line 9: place
> nit. places
Done
http://gerrit.cloudera.org:8080/#/c/22046/9//COMMIT_MSG@10
PS9, Line 10: require
> nit. requires
Done
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 11
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 17 Dec 2024 16:04:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 5:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/22046/4/fe/src/main/java/org/apache/impala/planner/AggregationNode.java
File fe/src/main/java/org/apache/impala/planner/AggregationNode.java:
http://gerrit.cloudera.org:8080/#/c/22046/4/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@223
PS4, Line 223: // to the proper place in the distributed plan via transferConjuncts argument.
> Is this comment still accurate?
Edited the comment.
http://gerrit.cloudera.org:8080/#/c/22046/4/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java:
http://gerrit.cloudera.org:8080/#/c/22046/4/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@a1073
PS4, Line 1073:
> I see you deprecated transferConjuncts.
Correct. That is the bad behavior that I want to fix by passing transferredConjuncts during init call.
http://gerrit.cloudera.org:8080/#/c/22046/4/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@1055
PS4, Line 1055: List<Expr> havingPredicates = node.conjuncts_;
> nit: could you do
Done
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 06 Dec 2024 21:10:00 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Yida Wu (Code Review)" <ge...@cloudera.org>.
Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 9:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/22046/9//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22046/9//COMMIT_MSG@9
PS9, Line 9: place
nit. places
http://gerrit.cloudera.org:8080/#/c/22046/9//COMMIT_MSG@10
PS9, Line 10: require
nit. requires
http://gerrit.cloudera.org:8080/#/c/22046/9/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java:
http://gerrit.cloudera.org:8080/#/c/22046/9/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@1067
PS9, Line 1067: mergeAggNode.init
Just curious if there's any difference in behavior when calling init() before or after setting the limit. It doesn't seem to be mentioned in the comments. If the order matters, could we add some comments here?
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 17 Dec 2024 09:08:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22046/9/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
File fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java:
http://gerrit.cloudera.org:8080/#/c/22046/9/fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java@1067
PS9, Line 1067: mergeAggNode.init
> Just curious if there's any difference in behavior when calling init() befo
Yes. init() call computeStats(), and computeStats() takes account for Limit.
It matters, because if we setLimit() after init(), we also need to redo computeStats() call.
Calling init() the very last will relive us from redoing computeStats().
Will add comment for this.
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 17 Dec 2024 15:54:15 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has removed a vote on this change.
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Removed Verified-1 by Impala Public Jenkins <im...@cloudera.com>
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 12
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 12:
All tests pass except testFullAcidFileMetadataAfterMinorCompaction. I think it is flaky and unrelated to this patch.
https://jenkins.impala.io/job/ubuntu-20.04-from-scratch/3996/testReport/junit/org.apache.impala.catalog.local/CatalogdMetaProviderTest/testFullAcidFileMetadataAfterMinorCompaction/
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 12
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 18 Dec 2024 21:59:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 7: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Mon, 09 Dec 2024 19:03:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 5:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17627/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 06 Dec 2024 21:34:24 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 5:
(2 comments)
I agree that this patch is not narrow enough to reflect the title. I'll see if I can split it and repurpose IMPALA-13480 for the JIRA number.
http://gerrit.cloudera.org:8080/#/c/22046/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22046/5//COMMIT_MSG@20
PS5, Line 20: This patch also fixes a bug where the cardinality estimate of MERGE
> Why isn't this a separate ticket?
I need to Enabling PlannerTestOption.VALIDATE_CARDINALITY in some test to verify this patch. However, they also reveals this issue introduced by IMPALA-13405.
http://gerrit.cloudera.org:8080/#/c/22046/5/fe/src/main/java/org/apache/impala/planner/AggregationNode.java
File fe/src/main/java/org/apache/impala/planner/AggregationNode.java:
http://gerrit.cloudera.org:8080/#/c/22046/5/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@276
PS5, Line 276: // DistributedPlanner.java may transfer conjunct to merge phase aggregation later.
> Were these part of the parent patch, and you've moved them to this one?
I moved them here since the fix share the same theme with this patch: transferring HAVING predicates incorrectly can change cardinality estimate in wrong way.
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 06 Dec 2024 23:20:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22046
to look at the new patch set (#7).
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Within DistributedPlanner.java, there are several place where Planner
need to insert extra merge aggregation node. It require transferring
HAVING conjuncts from preaggregation node to merge aggregation,
unsetting limit, and recompute stats of preaggregation node. However,
the stats recompute is not consistently done, and there might be an
inefficient recompute happening.
This patch fixes the order of AggregationNode creation order in
DistributedPlanner.java so that stats recomputation is done consistently
and efficiently.
Testing:
- Pass core tests.
Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
M testdata/workloads/functional-planner/queries/PlannerTest/multiple-distinct-materialization.test
5 files changed, 43 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/22046/7
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17631/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 7
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Sat, 07 Dec 2024 00:41:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/22046/5//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/22046/5//COMMIT_MSG@20
PS5, Line 20: This patch also fixes a bug where the cardinality estimate of MERGE
Why isn't this a separate ticket?
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 5
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 06 Dec 2024 21:37:02 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 12: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/11219/
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 12
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 18 Dec 2024 21:15:28 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 12:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/11219/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 12
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 18 Dec 2024 15:35:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Yida Wu, Michael Smith, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22046
to look at the new patch set (#11).
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Within DistributedPlanner.java, there are several places where Planner
need to insert extra merge aggregation node. It requires transferring
HAVING conjuncts from preaggregation node to merge aggregation,
unsetting limit, and recompute stats of preaggregation node. However,
the stats recompute is not consistently done, and there might be an
inefficient recompute happening.
This patch fixes the order of AggregationNode creation order in
DistributedPlanner.java so that stats recomputation is done consistently
and efficiently.
Testing:
- Pass core tests.
Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
M testdata/workloads/functional-planner/queries/PlannerTest/multiple-distinct-materialization.test
5 files changed, 45 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/22046/11
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 11
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Yida Wu, Michael Smith, Csaba Ringhofer, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22046
to look at the new patch set (#10).
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Within DistributedPlanner.java, there are several place where Planner
need to insert extra merge aggregation node. It require transferring
HAVING conjuncts from preaggregation node to merge aggregation,
unsetting limit, and recompute stats of preaggregation node. However,
the stats recompute is not consistently done, and there might be an
inefficient recompute happening.
This patch fixes the order of AggregationNode creation order in
DistributedPlanner.java so that stats recomputation is done consistently
and efficiently.
Testing:
- Pass core tests.
Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
M testdata/workloads/functional-planner/queries/PlannerTest/multiple-distinct-materialization.test
5 files changed, 45 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/22046/10
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 10
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Michael Smith (Code Review)" <ge...@cloudera.org>.
Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 9: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 9
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Comment-Date: Fri, 13 Dec 2024 21:23:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 10:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/17706/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 10
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Tue, 17 Dec 2024 16:30:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
Patch Set 12: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 12
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
Gerrit-Comment-Date: Wed, 18 Dec 2024 15:35:20 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Riza Suminto has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/22046 )
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Within DistributedPlanner.java, there are several places where Planner
need to insert extra merge aggregation node. It requires transferring
HAVING conjuncts from preaggregation node to merge aggregation,
unsetting limit, and recompute stats of preaggregation node. However,
the stats recompute is not consistently done, and there might be an
inefficient recompute happening.
This patch fixes the order of AggregationNode creation order in
DistributedPlanner.java so that stats recomputation is done consistently
and efficiently.
Testing:
- Pass core tests.
Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Reviewed-on: http://gerrit.cloudera.org:8080/22046
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Riza Suminto <ri...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
M testdata/workloads/functional-planner/queries/PlannerTest/multiple-distinct-materialization.test
5 files changed, 45 insertions(+), 22 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved
Riza Suminto: Verified
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 13
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Yida Wu <wy...@gmail.com>
[Impala-ASF-CR] IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Posted by "Riza Suminto (Code Review)" <ge...@cloudera.org>.
Hello Michael Smith, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/22046
to look at the new patch set (#6).
Change subject: IMPALA-13526: Fix Agg node creation order in DistributedPlanner
......................................................................
IMPALA-13526: Fix Agg node creation order in DistributedPlanner
Within DistributedPlanner.java, there are several place where Planner
need to insert extra merge aggregation node. It require transferring
HAVING conjuncts from preaggregation node to merge aggregation,
unsetting limit, and recompute stats of preaggregation node. However,
the stats recompute is not consistently done, and there might be an
inefficient recompute happening.
This patch fixes the order of AggregationNode creation order in
DistributedPlanner.java so that stats recomputation is done consistently
and efficiently.
Testing:
- Pass core tests.
Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
---
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/main/java/org/apache/impala/planner/PlanNode.java
M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
M testdata/workloads/functional-planner/queries/PlannerTest/multiple-distinct-materialization.test
5 files changed, 43 insertions(+), 22 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/46/22046/6
--
To view, visit http://gerrit.cloudera.org:8080/22046
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ica8227fdc46a1ef59bef5ae5424ba3907827411d
Gerrit-Change-Number: 22046
Gerrit-PatchSet: 6
Gerrit-Owner: Riza Suminto <ri...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Smith <mi...@cloudera.com>
Gerrit-Reviewer: Riza Suminto <ri...@cloudera.com>