You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "wangsheng (Code Review)" <ge...@cloudera.org> on 2021/09/07 07:09:45 UTC
[Impala-ASF-CR] IMPALA-10865: Fix initialize SelectStmt's groupingExprs in analyzeGroupingExprs
wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/17781 )
Change subject: IMPALA-10865: Fix initialize SelectStmt's groupingExprs_ in analyzeGroupingExprs
......................................................................
Patch Set 4:
(5 comments)
Hi guojing, thanks for this bugfix!
http://gerrit.cloudera.org:8080/#/c/17781/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/17781/4//COMMIT_MSG@9
PS4, Line 9: This patch rollback some changes of IMPALA-9620 which initialize
This patch also rollback some changes in IMPALA-10096, so maybe we can also explain this at first. Besides, I think this patch description maybe complex, can you reorganize the language to make it clearer and easier to understand?
http://gerrit.cloudera.org:8080/#/c/17781/4/fe/src/main/java/org/apache/impala/rewrite/ExtractCompoundVerticalBarExprRule.java
File fe/src/main/java/org/apache/impala/rewrite/ExtractCompoundVerticalBarExprRule.java:
http://gerrit.cloudera.org:8080/#/c/17781/4/fe/src/main/java/org/apache/impala/rewrite/ExtractCompoundVerticalBarExprRule.java@38
PS4, Line 38: if (!expr.isAnalyzed()) {
: pred = (CompoundVerticalBarExpr) expr.clone();
: pred.analyzeNoThrow(analyzer);
: }
This change seems unnecessary to this patch. Without this change, I can also execute this query successful:
SELECT ss_item_sk ss_item_sk_group,
ss_item_sk+300 ss_item_sk,
count(ss_ticket_number)
FROM store_sales a
WHERE ss_sold_date_sk > cast('245263' AS INT)
GROUP BY ss_item_sk_group,
ss_item_sk;
http://gerrit.cloudera.org:8080/#/c/17781/4/testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
File testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test:
http://gerrit.cloudera.org:8080/#/c/17781/4/testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test@1668
PS4, Line 1668: 3
Maybe we should change this in another commit?
http://gerrit.cloudera.org:8080/#/c/17781/4/testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test@1675
PS4, Line 1675: # Group by expr with column alias reference errors in re-analyze phase
Maybe we can add Jira here.
http://gerrit.cloudera.org:8080/#/c/17781/4/testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test@1676
PS4, Line 1676:
unnecessary
--
To view, visit http://gerrit.cloudera.org:8080/17781
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9d1779e6c282d9fd02beacf5ddfafcc5c0baf3b0
Gerrit-Change-Number: 17781
Gerrit-PatchSet: 4
Gerrit-Owner: guojingfeng <gu...@tencent.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <hu...@gmail.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Reviewer: wangsheng <sk...@163.com>
Gerrit-Comment-Date: Tue, 07 Sep 2021 07:09:45 +0000
Gerrit-HasComments: Yes