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/12/14 15:29:47 UTC
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
wangsheng has uploaded this change for review. ( http://gerrit.cloudera.org:8080/18099
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
IMPALA-11049: Substitute order by elements when creating SortInfo
Currently, Impala cloned order by elements' exprs, and substitute
these cloned exprs in-place, original exprs not changed. But when
executing 'SelectStmt.rewriteExprs()', Impala rewrite 'orderByElements_'
instead of 'sortInfo_', which will caused AnalysisException, since
these original exprs in 'orderByElements_' are 'INVALID' type.
This patch will substitute exprs in 'orderByElements_' in-place, and
use these substituted exprs to create 'SortInfo'. But Pay attention,
this will changed explain result when printing sql.
Besides, we also add a condition check in 'SimplifyCastExprRule' to
keep exprs must analyzed before rewrite.
Testing:
- Added test cases in 'explain-level3.test'
Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
---
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyCastExprRule.java
M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test
5 files changed, 78 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/18099/1
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 1
Gerrit-Owner: wangsheng <sk...@163.com>
[Impala-ASF-CR] IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
......................................................................
Patch Set 7:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/10140/ : 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/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 7
Gerrit-Owner: wangsheng <sk...@163.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: Fri, 11 Feb 2022 12:21:15 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
......................................................................
Patch Set 9:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7847/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 9
Gerrit-Owner: wangsheng <sk...@163.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: Wed, 16 Feb 2022 06:56:37 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
Hello Quanlong Huang, Zoltan Borok-Nagy, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/18099
to look at the new patch set (#8).
Change subject: IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
......................................................................
IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
We added a new expr rewrite rule 'SimplifyCastExprRule.java' in
IMPALA-10836. If expr is not analyzed, this rewrite rule would throw
an 'AnalysisException', this is due to 'orderByElements_' not been
analyzed. We try to substitute order by elements when creating
'SortInfo', but caused some other problems. So we only add expr
analyzed check in this rule to solve this problem. When adding other
expr rewrite rules in the future, we should also add this check.
Testing:
- Added test cases in 'explain-level3.test'
Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
---
M fe/src/main/java/org/apache/impala/rewrite/SimplifyCastExprRule.java
M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test
2 files changed, 62 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/18099/8
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 8
Gerrit-Owner: wangsheng <sk...@163.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>
[Impala-ASF-CR] IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
......................................................................
Patch Set 8:
> Could you please update the commit message?
Of course, done!
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 8
Gerrit-Owner: wangsheng <sk...@163.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: Fri, 11 Feb 2022 12:00:17 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has uploaded a new patch set (#2). ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
IMPALA-11049: Substitute order by elements when creating SortInfo
Currently, Impala cloned order by elements' exprs, and substitute
these cloned exprs in-place, original exprs not changed. But when
executing 'SelectStmt.rewriteExprs()', Impala rewrite 'orderByElements_'
instead of 'sortInfo_', which causes AnalysisException due to 'INVALID'
exprs in 'orderByElements_'. They are 'INVALID' type because they
weren't not analyzed.
This patch will substitute exprs in 'orderByElements_' in-place, and
use these substituted exprs to create 'SortInfo'. But Pay attention,
this will changed explain result when printing sql. For example:
explain select cast(id as INT),count(1) from functional.alltypes
group by 1 order by 1;
In EXPLAIN output:
Analyzed query: SELECT id, count(*) FROM functional.alltypes
GROUP BY id ORDER BY id ASC
Besides, we also add a condition check in 'SimplifyCastExprRule' to
ensure that exprs are already analyzed.
Testing:
- Added test cases in 'explain-level3.test'
Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
---
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyCastExprRule.java
M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test
5 files changed, 77 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/18099/2
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 2
Gerrit-Owner: wangsheng <sk...@163.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>
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
Patch Set 1:
(6 comments)
Thanks for working on this. I've only spotted a few nits.
http://gerrit.cloudera.org:8080/#/c/18099/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18099/1//COMMIT_MSG@12
PS1, Line 12: will caused
nit: causes
http://gerrit.cloudera.org:8080/#/c/18099/1//COMMIT_MSG@13
PS1, Line 13: 'INVALID' type
Maybe it's worth to mention that they are INVALID type because they weren't not analyzed.
http://gerrit.cloudera.org:8080/#/c/18099/1//COMMIT_MSG@15
PS1, Line 15: But Pay attention,
: this will changed explain result when printing sql.
I think we are already printing out the rewritten query in explain. E.g. for a table with 3 integer columns:
explain select cast(i as INT), cast(j as INT), max(k) from ice_void group by 2, 1;
In EXPLAIN output:
Analyzed query: SELECT i, j, max(k) FROM `default`.ice_void GROUP BY j, i
Or if I'm missing something, then could you provide an example in the commit message?
http://gerrit.cloudera.org:8080/#/c/18099/1//COMMIT_MSG@16
PS1, Line 16: changed
nit: change
http://gerrit.cloudera.org:8080/#/c/18099/1//COMMIT_MSG@17
PS1, Line 17: to
: keep exprs must analyzed before rewrite.
nit: to ensure that exprs are already analyzed.
http://gerrit.cloudera.org:8080/#/c/18099/1/fe/src/main/java/org/apache/impala/rewrite/SimplifyCastExprRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyCastExprRule.java:
http://gerrit.cloudera.org:8080/#/c/18099/1/fe/src/main/java/org/apache/impala/rewrite/SimplifyCastExprRule.java@61
PS1, Line 61: Expr must execute analyze before rewrite
'castExpr' must be analyzed before rewrite. Maybe we could add this as an error message instead of code comment.
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 1
Gerrit-Owner: wangsheng <sk...@163.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: Wed, 15 Dec 2021 14:53:10 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
IMPALA-11049: Substitute order by elements when creating SortInfo
Currently, Impala cloned order by elements' exprs, and substitute
these cloned exprs in-place, original exprs not changed. But when
executing 'SelectStmt.rewriteExprs()', Impala rewrite 'orderByElements_'
instead of 'sortInfo_', which causes AnalysisException due to 'INVALID'
exprs in 'orderByElements_'. They are 'INVALID' type because they
weren't not analyzed.
This patch will substitute exprs in 'orderByElements_' in-place, and
use these substituted exprs to create 'SortInfo'. But Pay attention,
this will changed explain result when printing sql. For example:
explain select id, count(1) from functional.alltypes
group by 1 order by 1;
Without this patch, the EXPLAIN output is:
Analyzed query: SELECT id, count(*) FROM functional.alltypes
GROUP BY id ORDER BY CAST(1 AS TINYINT) ASC
After rewrite 'sortInfo_', the EXPLAIN output is:
Analyzed query: SELECT id, count(*) FROM functional.alltypes
GROUP BY id ORDER BY id ASC
Besides, we also add a condition check in 'SimplifyCastExprRule' to
ensure that exprs are already analyzed.
Testing:
- Added test cases in 'explain-level3.test'
Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
---
M fe/src/main/java/org/apache/impala/rewrite/SimplifyCastExprRule.java
M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test
2 files changed, 62 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/18099/6
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 6
Gerrit-Owner: wangsheng <sk...@163.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>
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
IMPALA-11049: Substitute order by elements when creating SortInfo
Currently, Impala cloned order by elements' exprs, and substitute
these cloned exprs in-place, original exprs not changed. But when
executing 'SelectStmt.rewriteExprs()', Impala rewrite 'orderByElements_'
instead of 'sortInfo_', which causes AnalysisException due to 'INVALID'
exprs in 'orderByElements_'. They are 'INVALID' type because they
weren't not analyzed.
This patch will substitute exprs in 'orderByElements_' in-place, and
use these substituted exprs to create 'SortInfo'. But Pay attention,
this will changed explain result when printing sql. For example:
explain select id, count(1) from functional.alltypes
group by 1 order by 1;
Without this patch, the EXPLAIN output is:
Analyzed query: SELECT id, count(*) FROM functional.alltypes
GROUP BY id ORDER BY CAST(1 AS TINYINT) ASC
After rewrite 'sortInfo_', the EXPLAIN output is:
Analyzed query: SELECT id, count(*) FROM functional.alltypes
GROUP BY id ORDER BY id ASC
Besides, we also add a condition check in 'SimplifyCastExprRule' to
ensure that exprs are already analyzed.
Testing:
- Added test cases in 'explain-level3.test'
Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
---
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/SetOperationStmt.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyCastExprRule.java
M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test
5 files changed, 77 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/18099/4
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 4
Gerrit-Owner: wangsheng <sk...@163.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>
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
Patch Set 5:
I think it's not guaranteed that exprs used in rewrite rules are all analyzed. So we have codes like this to analyze exprs in time: https://github.com/apache/impala/blob/200d3664f0117e1f7a9b01941d004189d4343920/fe/src/main/java/org/apache/impala/rewrite/ConvertToCNFRule.java#L114-L118
I'll run a GVO to see how many tests required changes due to the EXPLAIN difference.
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 5
Gerrit-Owner: wangsheng <sk...@163.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: Wed, 12 Jan 2022 11:40:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
Patch Set 6:
> > Patch Set 6:
> >
> > Hi Quanlong and Zoltan, there are almost 25 tests failed in
> jenkins job with this patch. I checked these tests one by one, and
> found at least two problems.
> > 1. UNION and VALUES execute failed, such as: select 1+1,2,3 UNION
> select 4,5,6 order by 2. After patch, 'orderByElements_' been
> rewrite to SlotRef, in 'reAnalyze' phase, when execute substitute
> again, SlotRef analyze failed due to 'Preconditions.checkState(rawPath_
> != null);', since example sql clause is not a real column, just a
> 'NumericLiteral'. We can remove this check to slove this problem.
> > 2. Another problem is hard to resloved, INTERSECT sql return
> exception like this: 'IllegalStateException: Illegal reference to
> non-materialized slot: tid=3 sid=39'. I also read the code. The
> main reason is that when transform INTERSECT to a SelectStmt like
> 'select count(*) xxx' in StmtRewriter.rewriteSetOperationStatement(),
> all select list been materizlized in SelectStmt.SelectAnalyzer.expandStar(),
> but SlotRef in 'orderByElements_' not been materizlized, and add to
> this new SelectStmt's order by directly. When this new SelectStmt
> execute analyze, there will be some SlotRef in 'materializedExprs_'
> which are not 'materizlized'. Here is a example:
> > select id, bool_col, tinyint_col, smallint_col, int_col,
> bigint_col, float_col, double_col, date_string_col, string_col,
> timestamp_col, year, month from alltypestiny where year=2009 and
> month in (1,2,3)
> > intersect
> > select id, bool_col, tinyint_col, smallint_col, int_col,
> bigint_col, float_col, double_col, date_string_col, string_col,
> timestamp_col, year, month from alltypestiny where year=2009 and
> month in (1,2)
> > intersect
> > (select id, bool_col, tinyint_col, smallint_col, int_col,
> bigint_col, float_col, double_col, date_string_col, string_col,
> timestamp_col, year, month from alltypestiny where year=2009 and
> month=1)
> > order by 1 limit 1;
> >
> > Besides, I'm not sure if there any other problems not been found,
> so I think maybe we should just keep this patch adding check and
> analyze in 'SimplifyCastExprRule', substitute order by elements
> maybe need to modify a lot of code. How do you think?
>
> Thanks for digging into this! I tend to add checks and analyze the
> exprs in need in the rules, which is we already done in
> ConvertToCNFRule.
I found that most expr rewrite rules already added 'isAnalyzed' check, but some of return directly like this:
if (!expr.isAnalyzed()) return expr;
Do you mean replace these 'return' by 'expr.analyze(analyzer)'?
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 6
Gerrit-Owner: wangsheng <sk...@163.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: Thu, 27 Jan 2022 09:33:10 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
Patch Set 6:
Hi Quanlong and Zoltan, there are almost 25 tests failed in jenkins job with this patch. I checked these tests one by one, and found at least two problems.
1. UNION and VALUES execute failed, such as: select 1+1,2,3 UNION select 4,5,6 order by 2. After patch, 'orderByElements_' been rewrite to SlotRef, in 'reAnalyze' phase, when execute substitute again, SlotRef analyze failed due to 'Preconditions.checkState(rawPath_ != null);', since example sql clause is not a real column, just a 'NumericLiteral'. We can remove this check to slove this problem.
2. Another problem is hard to resloved, INTERSECT sql return exception like this: 'IllegalStateException: Illegal reference to non-materialized slot: tid=3 sid=39'. I also read the code. The main reason is that when transform INTERSECT to a SelectStmt like 'select count(*) xxx' in StmtRewriter.rewriteSetOperationStatement(), all select list been materizlized in SelectStmt.SelectAnalyzer.expandStar(), but SlotRef in 'orderByElements_' not been materizlized, and add to this new SelectStmt's order by directly. When this new SelectStmt execute analyze, there will be some SlotRef in 'materializedExprs_' which are not 'materizlized'. Here is a example:
select id, bool_col, tinyint_col, smallint_col, int_col, bigint_col, float_col, double_col, date_string_col, string_col, timestamp_col, year, month from alltypestiny where year=2009 and month in (1,2,3)
intersect
select id, bool_col, tinyint_col, smallint_col, int_col, bigint_col, float_col, double_col, date_string_col, string_col, timestamp_col, year, month from alltypestiny where year=2009 and month in (1,2)
intersect
(select id, bool_col, tinyint_col, smallint_col, int_col, bigint_col, float_col, double_col, date_string_col, string_col, timestamp_col, year, month from alltypestiny where year=2009 and month=1)
order by 1 limit 1;
Besides, I'm not sure if there any other problems not been found, so I think maybe we should just keep this patch adding check and analyze in 'SimplifyCastExprRule', substitute order by elements maybe need to modify a lot of code. How do you think?
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 6
Gerrit-Owner: wangsheng <sk...@163.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: Wed, 26 Jan 2022 13:02:56 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
Patch Set 6:
pre-review-test passed: https://jenkins.impala.io/job/pre-review-test/1266/
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 6
Gerrit-Owner: wangsheng <sk...@163.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: Fri, 28 Jan 2022 02:03:52 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
Patch Set 5: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/7741/
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 5
Gerrit-Owner: wangsheng <sk...@163.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: Wed, 12 Jan 2022 18:02:22 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
......................................................................
Patch Set 8: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 8
Gerrit-Owner: wangsheng <sk...@163.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: Mon, 14 Feb 2022 01:46:03 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
......................................................................
Patch Set 9: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 9
Gerrit-Owner: wangsheng <sk...@163.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: Wed, 16 Feb 2022 13:28:26 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
......................................................................
IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
We added a new expr rewrite rule 'SimplifyCastExprRule.java' in
IMPALA-10836. If expr is not analyzed, this rewrite rule would throw
an 'AnalysisException', this is due to 'orderByElements_' not been
analyzed. We try to substitute order by elements when creating 'SortInfo',
but caused some other problems. So we only add expr analyzed check in this
rule to solve this problem. When adding other expr rewrite rules in the
future, we should also add this check.
Testing:
- Added test cases in 'explain-level3.test'
Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
---
M fe/src/main/java/org/apache/impala/rewrite/SimplifyCastExprRule.java
M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test
2 files changed, 62 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/99/18099/7
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 7
Gerrit-Owner: wangsheng <sk...@163.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>
[Impala-ASF-CR] IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
......................................................................
Patch Set 8: Code-Review+2
Thanks for fixing this, LGTM!
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 8
Gerrit-Owner: wangsheng <sk...@163.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: Mon, 14 Feb 2022 10:03:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
Patch Set 1:
Hi Zoltan, I found that we don't need to rewrite 'sortInfo_' in SelectStmt.rewriteExprs(). Currently, when substitute exprs in order by elements. After rewrite order by elements' exprs, Impala will use substituted exprs to create SortInfo in reAnalyze phase.
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 1
Gerrit-Owner: wangsheng <sk...@163.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, 14 Dec 2021 15:36:08 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9958/ : 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/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 4
Gerrit-Owner: wangsheng <sk...@163.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: Mon, 27 Dec 2021 14:55:00 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
......................................................................
Patch Set 9: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 9
Gerrit-Owner: wangsheng <sk...@163.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: Wed, 16 Feb 2022 06:56:37 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
Patch Set 6: Code-Review+1
Could you please update the commit message?
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 6
Gerrit-Owner: wangsheng <sk...@163.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: Fri, 11 Feb 2022 10:42:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
......................................................................
IMPALA-11049: Added expr analyzed check in 'SimplifyCastExprRule.java'
We added a new expr rewrite rule 'SimplifyCastExprRule.java' in
IMPALA-10836. If expr is not analyzed, this rewrite rule would throw
an 'AnalysisException', this is due to 'orderByElements_' not been
analyzed. We try to substitute order by elements when creating
'SortInfo', but caused some other problems. So we only add expr
analyzed check in this rule to solve this problem. When adding other
expr rewrite rules in the future, we should also add this check.
Testing:
- Added test cases in 'explain-level3.test'
Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Reviewed-on: http://gerrit.cloudera.org:8080/18099
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/rewrite/SimplifyCastExprRule.java
M testdata/workloads/functional-query/queries/QueryTest/explain-level3.test
2 files changed, 62 insertions(+), 0 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 10
Gerrit-Owner: wangsheng <sk...@163.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>
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/9924/ : 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/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 1
Gerrit-Owner: wangsheng <sk...@163.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, 14 Dec 2021 15:53:21 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
Patch Set 2:
Build Failed
https://jenkins.impala.io/job/gerrit-code-review-checks/9931/ : Initial code review checks failed. See linked job for details on the failure.
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 2
Gerrit-Owner: wangsheng <sk...@163.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: Wed, 15 Dec 2021 16:01:45 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
Patch Set 5: Code-Review+1
LGTM
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 5
Gerrit-Owner: wangsheng <sk...@163.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: Fri, 07 Jan 2022 15:42:38 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
Patch Set 5:
(1 comment)
Hi Quanlong, thanks for review this patch. I also submit a pre-review-test, almost 25 test cases failed. I will check these failed tests one by one.
http://gerrit.cloudera.org:8080/#/c/18099/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18099/1//COMMIT_MSG@15
PS1, Line 15: ts_' in-place, and
: use these substituted exprs to create 'SortInfo'. B
> I mean the EXPLAIN output already prints out the rewritten SQL, even withou
Thanks for explain, how about this new example? Compare 'sortInfo_' before and after this patch.
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 5
Gerrit-Owner: wangsheng <sk...@163.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: Sun, 23 Jan 2022 12:33:25 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "wangsheng (Code Review)" <ge...@cloudera.org>.
wangsheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
Patch Set 2:
(5 comments)
Thanks for review!
http://gerrit.cloudera.org:8080/#/c/18099/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18099/1//COMMIT_MSG@12
PS1, Line 12: causes Anal
> nit: causes
Done
http://gerrit.cloudera.org:8080/#/c/18099/1//COMMIT_MSG@13
PS1, Line 13: type because
> Maybe it's worth to mention that they are INVALID type because they weren't
Done
http://gerrit.cloudera.org:8080/#/c/18099/1//COMMIT_MSG@15
PS1, Line 15: ts_' in-place, and
: use these substituted exprs to create 'SortInfo'. B
> I think we are already printing out the rewritten query in explain. E.g. fo
Done
http://gerrit.cloudera.org:8080/#/c/18099/1//COMMIT_MSG@17
PS1, Line 17:
:
> nit: to ensure that exprs are already analyzed.
Done
http://gerrit.cloudera.org:8080/#/c/18099/1/fe/src/main/java/org/apache/impala/rewrite/SimplifyCastExprRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyCastExprRule.java:
http://gerrit.cloudera.org:8080/#/c/18099/1/fe/src/main/java/org/apache/impala/rewrite/SimplifyCastExprRule.java@61
PS1, Line 61: econditions.checkState(castExpr.getType(
> 'castExpr' must be analyzed before rewrite. Maybe we could add this as an e
Done
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 2
Gerrit-Owner: wangsheng <sk...@163.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: Wed, 15 Dec 2021 15:57:07 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>.
Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
Thanks for applying the changes. LGTM!
http://gerrit.cloudera.org:8080/#/c/18099/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/18099/1//COMMIT_MSG@15
PS1, Line 15: ts_' in-place, and
: use these substituted exprs to create 'SortInfo'. B
> Done
I mean the EXPLAIN output already prints out the rewritten SQL, even without this patch. So maybe we don't need this sentence (and neither the new examples)?
Unless something significant changed in the EXPLAIN output, but AFAIK this is not the case here.
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 3
Gerrit-Owner: wangsheng <sk...@163.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: Fri, 17 Dec 2021 11:10:24 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/7741/ DRY_RUN=true
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 5
Gerrit-Owner: wangsheng <sk...@163.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: Wed, 12 Jan 2022 11:41:54 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
Patch Set 6:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/10045/ : 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/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 6
Gerrit-Owner: wangsheng <sk...@163.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: Wed, 26 Jan 2022 13:23:10 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-11049: Substitute order by elements when creating SortInfo
Posted by "Quanlong Huang (Code Review)" <ge...@cloudera.org>.
Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/18099 )
Change subject: IMPALA-11049: Substitute order by elements when creating SortInfo
......................................................................
Patch Set 6:
> Patch Set 6:
>
> Hi Quanlong and Zoltan, there are almost 25 tests failed in jenkins job with this patch. I checked these tests one by one, and found at least two problems.
> 1. UNION and VALUES execute failed, such as: select 1+1,2,3 UNION select 4,5,6 order by 2. After patch, 'orderByElements_' been rewrite to SlotRef, in 'reAnalyze' phase, when execute substitute again, SlotRef analyze failed due to 'Preconditions.checkState(rawPath_ != null);', since example sql clause is not a real column, just a 'NumericLiteral'. We can remove this check to slove this problem.
> 2. Another problem is hard to resloved, INTERSECT sql return exception like this: 'IllegalStateException: Illegal reference to non-materialized slot: tid=3 sid=39'. I also read the code. The main reason is that when transform INTERSECT to a SelectStmt like 'select count(*) xxx' in StmtRewriter.rewriteSetOperationStatement(), all select list been materizlized in SelectStmt.SelectAnalyzer.expandStar(), but SlotRef in 'orderByElements_' not been materizlized, and add to this new SelectStmt's order by directly. When this new SelectStmt execute analyze, there will be some SlotRef in 'materializedExprs_' which are not 'materizlized'. Here is a example:
> select id, bool_col, tinyint_col, smallint_col, int_col, bigint_col, float_col, double_col, date_string_col, string_col, timestamp_col, year, month from alltypestiny where year=2009 and month in (1,2,3)
> intersect
> select id, bool_col, tinyint_col, smallint_col, int_col, bigint_col, float_col, double_col, date_string_col, string_col, timestamp_col, year, month from alltypestiny where year=2009 and month in (1,2)
> intersect
> (select id, bool_col, tinyint_col, smallint_col, int_col, bigint_col, float_col, double_col, date_string_col, string_col, timestamp_col, year, month from alltypestiny where year=2009 and month=1)
> order by 1 limit 1;
>
> Besides, I'm not sure if there any other problems not been found, so I think maybe we should just keep this patch adding check and analyze in 'SimplifyCastExprRule', substitute order by elements maybe need to modify a lot of code. How do you think?
Thanks for digging into this! I tend to add checks and analyze the exprs in need in the rules, which is we already done in ConvertToCNFRule.
--
To view, visit http://gerrit.cloudera.org:8080/18099
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2780e04a6d5a32e224cd0470cf6f166a832363ec
Gerrit-Change-Number: 18099
Gerrit-PatchSet: 6
Gerrit-Owner: wangsheng <sk...@163.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: Thu, 27 Jan 2022 06:57:32 +0000
Gerrit-HasComments: No