You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Marcel Kornacker (Code Review)" <ge...@cloudera.org> on 2017/01/10 02:20:02 UTC
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Marcel Kornacker has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5662
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
IMPALA-4739: ExprRewriter fails on HAVING clauses
SelectStmt.rewriteExprs() rewrote the pre-analysis form of the HAVING
predicate; the fix was to rewrite the post-analysis version.
SelectStmt.rewriteExprs() is also not effective for the grouping exprs,
but I decided to leave that alone, since it won't result in an error.
The rewriter rule tests are too narrow right now, because they only apply
to select list exprs (which is why it missed the problem with the Having
clause). I added a functional test for this particular jira, but we should
also restructure the rewriter rule tests themselves to apply to all
syntactic elements that can see rewrites.
Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
---
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
3 files changed, 17 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/5662/1
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
Patch Set 4:
I changed the approach, please take another look.
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
Patch Set 5:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/168/
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins, Dimitris Tsirogiannis, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5662
to look at the new patch set (#3).
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
IMPALA-4739: ExprRewriter fails on HAVING clauses
The bug was that expr rewrite rules such as ExtractCommonConjunctRule
analyzed their own output, which doesn't work for syntactic elements
that allow column aliases, such as the HAVING clause.
The fix was to remove the analysis step (the re-analysis happens anyway
in AnalysisCtx).
Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
---
M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
M fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
M fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
7 files changed, 35 insertions(+), 20 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/5662/3
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
Patch Set 1:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/157/
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5662/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:
PS1, Line 898:
In the commit msg, you mention that you didn't intend to remove this :)
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
Patch Set 5: Code-Review+2
> I don't fully understand the consequences of not analyzing these
> exprs. Based on Jim's comment, some parts ofthe code seem to assume
> that the exprs remain analyzed after rewriting.
>
> Would a more conservative approach be to check if the exprs are
> analyzed upon entry to the function, then re-analyze them only if
> they were?
Jim reported the output of a test failure, which is now fixed.
I want to leave it at that for now, the code is convoluted enough as it is.
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins, Dimitris Tsirogiannis, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5662
to look at the new patch set (#4).
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
IMPALA-4739: ExprRewriter fails on HAVING clauses
The bug was that expr rewrite rules such as ExtractCommonConjunctRule
analyzed their own output, which doesn't work for syntactic elements
that allow column aliases, such as the HAVING clause.
The fix was to remove the analysis step (the re-analysis happens anyway
in AnalysisCtx).
Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
---
M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
M fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
M fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
6 files changed, 25 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/5662/4
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins, Dimitris Tsirogiannis,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5662
to look at the new patch set (#2).
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
IMPALA-4739: ExprRewriter fails on HAVING clauses
SelectStmt.rewriteExprs() rewrote the pre-analysis form of the HAVING
predicate; the fix was to rewrite the post-analysis version.
SelectStmt.rewriteExprs() is also not effective for the grouping exprs,
but I decided to leave that alone, since it won't result in an error.
The rewriter rule tests are too narrow right now, because they only apply
to select list exprs (which is why it missed the problem with the Having
clause). I added a functional test for this particular jira, but we should
also restructure the rewriter rule tests themselves to apply to all
syntactic elements that can see rewrites.
Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
---
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
4 files changed, 27 insertions(+), 16 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/5662/2
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
Patch Set 2:
Is this ready for a new pre-commit Jenkins test?
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
Patch Set 2:
I think this requires some additional test fixes, I am looking into it.
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
Patch Set 1:
Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/156/
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
Patch Set 4:
I don't fully understand the consequences of not analyzing these exprs. Based on Jim's comment, some parts ofthe code seem to assume that the exprs remain analyzed after rewriting.
Would a more conservative approach be to check if the exprs are analyzed upon entry to the function, then re-analyze them only if they were?
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
Patch Set 2:
Marcel just informed me that he will continue working on this patch. (I am no longer working on this)
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
Patch Set 4:
org.apache.impala.common.AnalysisException
at org.apache.impala.analysis.AnalysisContext.analyze(AnalysisContext.java:424)
at org.apache.impala.common.FrontendTestBase.AnalyzesOk(FrontendTestBase.java:249)
at org.apache.impala.common.FrontendTestBase.AnalyzesOk(FrontendTestBase.java:228)
at org.apache.impala.analysis.AnalyzeExprsTest.TestBetweenPredicates(AnalyzeExprsTest.java:597)
...
Caused by: java.lang.IllegalStateException
at com.google.common.base.Preconditions.checkState(Preconditions.java:133)
at org.apache.impala.service.FeSupport.EvalPredicate(FeSupport.java:184)
at org.apache.impala.analysis.Analyzer.markConstantConjunct(Analyzer.java:1064)
at org.apache.impala.analysis.Analyzer.registerConjuncts(Analyzer.java:1038)
at org.apache.impala.analysis.SelectStmt.analyze(SelectStmt.java:247)
at org.apache.impala.analysis.AnalysisContext.analyze(AnalysisContext.java:381)
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
IMPALA-4739: ExprRewriter fails on HAVING clauses
The bug was that expr rewrite rules such as ExtractCommonConjunctRule
analyzed their own output, which doesn't work for syntactic elements
that allow column aliases, such as the HAVING clause.
The fix was to remove the analysis step (the re-analysis happens anyway
in AnalysisCtx).
Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Reviewed-on: http://gerrit.cloudera.org:8080/5662
Reviewed-by: Marcel Kornacker <ma...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
M fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
M fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
7 files changed, 28 insertions(+), 10 deletions(-)
Approvals:
Marcel Kornacker: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
Patch Set 1:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/156/
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5662/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:
PS1, Line 898:
> In the commit msg, you mention that you didn't intend to remove this :)
i didn't intend to *fix* it (hence the todo), but there's no point in leaving that line in place :)
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
zamsden@cloudera.com has posted comments on this change.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
Patch Set 6:
(2 comments)
I realize this is already merged - simply learning how to use gerrit and I actually did have two comments.
http://gerrit.cloudera.org:8080/#/c/5662/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:
PS6, Line 1065: ;
Nit: spurious semi-colon
http://gerrit.cloudera.org:8080/#/c/5662/6/testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
File testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test:
PS6, Line 67: sum(2 + id) > 1, sum(2 + id) >= 5
Looks like further opportunities for redundant subexpression elimination remain, although this one is a bit more difficult. Is there already a JIRA to track this?
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: zamsden@cloudera.com
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins, Dimitris Tsirogiannis, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5662
to look at the new patch set (#5).
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
IMPALA-4739: ExprRewriter fails on HAVING clauses
The bug was that expr rewrite rules such as ExtractCommonConjunctRule
analyzed their own output, which doesn't work for syntactic elements
that allow column aliases, such as the HAVING clause.
The fix was to remove the analysis step (the re-analysis happens anyway
in AnalysisCtx).
Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
M fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
M fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
7 files changed, 28 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/5662/5
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
Patch Set 2: Code-Review+2
Test fixes look good to me
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
Patch Set 1: Verified-1
Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/157/
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5662/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:
PS6, Line 1065: ;
> Nit: spurious semi-colon
i'll fix it in a follow-on patch (plus the long lines).
http://gerrit.cloudera.org:8080/#/c/5662/6/testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
File testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test:
PS6, Line 67: sum(2 + id) > 1, sum(2 + id) >= 5
> Looks like further opportunities for redundant subexpression elimination re
i don't think for this particular one.
--
To view, visit http://gerrit.cloudera.org:8080/5662
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes