You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org> on 2017/04/17 15:02:35 UTC
[Impala-ASF-CR] IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
Thomas Tauber-Marshall has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6653
Change subject: IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
......................................................................
IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
This patch addresses 3 issues:
- SelectList.reset didn't properly reset some of its members, though
they're documented as needing to be reset. This was causing a crash
when the Planner attempted to make an aggregation node for an agg
function that had been eliminated by expr rewriting. While I'm here,
I added reseting of all of SelectList's members that need to be reset,
and fixed the documentation of one member that shouldn't be reset.
- SimplifyConditionalsRule was changing the meaning of queries that
contains agg functions, e.g. because "select if(true, 0, sum(id))"
is not equivalent to "select 0". The fix is to not return the simplfied
expr if it removes all aggregates.
- ExprRewriteRulesTest was performing rewrites on the result exprs of the
SelectStmt, which causes problems if the result exprs have been
substituted. In normal query execution, we don't rewrite the result exprs
anyways, so the fix is to match normal query execution and rewrite the select
list exprs.
Testing:
- Added e2e test to exprs.test.
- Added unit test to ExprRewriteRulesTest.
Change-Id: Ic20b1621753980b47a612e0885804363b733f6da
---
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
4 files changed, 37 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/6653/1
--
To view, visit http://gerrit.cloudera.org:8080/6653
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic20b1621753980b47a612e0885804363b733f6da
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
......................................................................
IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
This patch addresses 3 issues:
- SelectList.reset() didn't properly reset some of its members, though
they're documented as needing to be reset. This was causing a crash
when the Planner attempted to make an aggregation node for an agg
function that had been eliminated by expr rewriting. While I'm here,
I added resetting of all of SelectList's members that need to be
reset, and fixed the documentation of one member that shouldn't be
reset.
- SimplifyConditionalsRule was changing the meaning of queries that
contain agg functions, e.g. because "select if(true, 0, sum(id))"
is not equivalent to "select 0". The fix is to not return the
simplfied expr if it removes all aggregates.
- ExprRewriteRulesTest was performing rewrites on the result exprs of
the SelectStmt, which causes problems if the result exprs have been
substituted. In normal query execution, we don't rewrite the result
exprs anyway, so the fix is to match normal query execution and
rewrite the select list exprs.
Testing:
- Added e2e test to exprs.test.
- Added unit test to ExprRewriteRulesTest.
Change-Id: Ic20b1621753980b47a612e0885804363b733f6da
Reviewed-on: http://gerrit.cloudera.org:8080/6653
Reviewed-by: Thomas Tauber-Marshall <tm...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
4 files changed, 41 insertions(+), 10 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Thomas Tauber-Marshall: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/6653
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic20b1621753980b47a612e0885804363b733f6da
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.
Change subject: IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6653/2/testdata/workloads/functional-query/queries/QueryTest/exprs.test
File testdata/workloads/functional-query/queries/QueryTest/exprs.test:
Line 2738: # IMPALA-5125: test behavior when an agg function could be eliminated by expr rewrites.
> typo: eliminated
Done
--
To view, visit http://gerrit.cloudera.org:8080/6653
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic20b1621753980b47a612e0885804363b733f6da
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.
Change subject: IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
......................................................................
Patch Set 2:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/6653/1//COMMIT_MSG
Commit Message:
Line 10: - SelectList.reset() didn't properly reset some of its members, though
> reset()
Done
Line 14: I added resetting of all of SelectList's members that need to be
> resetting
Done
Line 17: - SimplifyConditionalsRule was changing the meaning of queries that
> contains -> contain
Done
Line 21: - ExprRewriteRulesTest was performing rewrites on the result exprs of
> long lines
Done
Line 23: substituted. In normal query execution, we don't rewrite the result
> anyways -> anyway
Done
http://gerrit.cloudera.org:8080/#/c/6653/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:
Line 80: protected String sqlString_;
> fter this change will we still see the new SQL after subquery rewriting? Fo
Yes, this is not changing the behavior of sqlString_, just moving it out of the "Members that need to be reset()" block.
http://gerrit.cloudera.org:8080/#/c/6653/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 328: // IMPALA-5125: Exprs containing aggregates should not be rewritten if the rewrite
> Exprs containing aggregates should not be rewritten if the rewrite eliminat
Done
Line 331: RewritesOk("if(false, max(id), min(id))", rule, "min(id)");
> Add tests for cases where some but not all aggregates are eliminated.
Done
--
To view, visit http://gerrit.cloudera.org:8080/6653
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic20b1621753980b47a612e0885804363b733f6da
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
......................................................................
Patch Set 3:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/497/
--
To view, visit http://gerrit.cloudera.org:8080/6653
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic20b1621753980b47a612e0885804363b733f6da
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Alex Behm,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6653
to look at the new patch set (#3).
Change subject: IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
......................................................................
IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
This patch addresses 3 issues:
- SelectList.reset() didn't properly reset some of its members, though
they're documented as needing to be reset. This was causing a crash
when the Planner attempted to make an aggregation node for an agg
function that had been eliminated by expr rewriting. While I'm here,
I added resetting of all of SelectList's members that need to be
reset, and fixed the documentation of one member that shouldn't be
reset.
- SimplifyConditionalsRule was changing the meaning of queries that
contain agg functions, e.g. because "select if(true, 0, sum(id))"
is not equivalent to "select 0". The fix is to not return the
simplfied expr if it removes all aggregates.
- ExprRewriteRulesTest was performing rewrites on the result exprs of
the SelectStmt, which causes problems if the result exprs have been
substituted. In normal query execution, we don't rewrite the result
exprs anyway, so the fix is to match normal query execution and
rewrite the select list exprs.
Testing:
- Added e2e test to exprs.test.
- Added unit test to ExprRewriteRulesTest.
Change-Id: Ic20b1621753980b47a612e0885804363b733f6da
---
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
4 files changed, 41 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/6653/3
--
To view, visit http://gerrit.cloudera.org:8080/6653
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic20b1621753980b47a612e0885804363b733f6da
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has uploaded a new patch set (#2).
Change subject: IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
......................................................................
IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
This patch addresses 3 issues:
- SelectList.reset() didn't properly reset some of its members, though
they're documented as needing to be reset. This was causing a crash
when the Planner attempted to make an aggregation node for an agg
function that had been eliminated by expr rewriting. While I'm here,
I added resetting of all of SelectList's members that need to be
reset, and fixed the documentation of one member that shouldn't be
reset.
- SimplifyConditionalsRule was changing the meaning of queries that
contain agg functions, e.g. because "select if(true, 0, sum(id))"
is not equivalent to "select 0". The fix is to not return the
simplfied expr if it removes all aggregates.
- ExprRewriteRulesTest was performing rewrites on the result exprs of
the SelectStmt, which causes problems if the result exprs have been
substituted. In normal query execution, we don't rewrite the result
exprs anyway, so the fix is to match normal query execution and
rewrite the select list exprs.
Testing:
- Added e2e test to exprs.test.
- Added unit test to ExprRewriteRulesTest.
Change-Id: Ic20b1621753980b47a612e0885804363b733f6da
---
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
4 files changed, 41 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/53/6653/2
--
To view, visit http://gerrit.cloudera.org:8080/6653
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic20b1621753980b47a612e0885804363b733f6da
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
[Impala-ASF-CR] IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6653/2/testdata/workloads/functional-query/queries/QueryTest/exprs.test
File testdata/workloads/functional-query/queries/QueryTest/exprs.test:
Line 2738: # IMPALA-5125: test behavior when an agg function could be elimincated by expr rewrites.
typo: eliminated
--
To view, visit http://gerrit.cloudera.org:8080/6653
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic20b1621753980b47a612e0885804363b733f6da
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
......................................................................
Patch Set 3: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/6653
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic20b1621753980b47a612e0885804363b733f6da
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change.
Change subject: IMPALA-5125: SimplifyConditionalsRule incorrectly handles aggregates
......................................................................
Patch Set 1:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/6653/1//COMMIT_MSG
Commit Message:
Line 10: - SelectList.reset didn't properly reset some of its members, though
reset()
Line 14: I added reseting of all of SelectList's members that need to be reset,
resetting
Line 17: contains agg functions, e.g. because "select if(true, 0, sum(id))"
contains -> contain
Line 21: SelectStmt, which causes problems if the result exprs have been
long lines
Line 23: anyways, so the fix is to match normal query execution and rewrite the select
anyways -> anyway
http://gerrit.cloudera.org:8080/#/c/6653/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:
Line 80: protected String sqlString_;
fter this change will we still see the new SQL after subquery rewriting? For example, look for:
LOG.trace("rewrittenStmt: " + analysisResult_.stmt_.toSql());
in AnalysisContext.java
Pick any query from AnalyzeSubqueriesTest.java for testing
http://gerrit.cloudera.org:8080/#/c/6653/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 328: // IMPALA-5125: Exprs containing aggregates shouldn't be rewritten.
Exprs containing aggregates should not be rewritten if the rewrite eliminates all aggregates.
Line 331: RewritesOk("case when true then 0 when false then sum(id) end", rule, null);
Add tests for cases where some but not all aggregates are eliminated.
--
To view, visit http://gerrit.cloudera.org:8080/6653
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic20b1621753980b47a612e0885804363b733f6da
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-HasComments: Yes