You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by kw...@apache.org on 2017/04/26 23:22:15 UTC
[03/10] incubator-impala git commit: 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
Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/915a1634
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/915a1634
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/915a1634
Branch: refs/heads/master
Commit: 915a16345c9325f29cad2a4c113d960e434b4ba7
Parents: c1463ff
Author: Thomas Tauber-Marshall <tm...@cloudera.com>
Authored: Fri Apr 14 12:36:46 2017 -0700
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Mon Apr 24 21:41:11 2017 +0000
----------------------------------------------------------------------
.../org/apache/impala/analysis/SelectStmt.java | 13 ++++++++-----
.../impala/rewrite/SimplifyConditionalsRule.java | 19 +++++++++++++++----
.../impala/analysis/ExprRewriteRulesTest.java | 11 ++++++++++-
.../queries/QueryTest/exprs.test | 8 ++++++++
4 files changed, 41 insertions(+), 10 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/915a1634/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java b/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
index e3f4e6a..c146247 100644
--- a/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
+++ b/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
@@ -68,10 +68,6 @@ public class SelectStmt extends QueryStmt {
// set if we have AnalyticExprs in the select list/order by clause
private AnalyticInfo analyticInfo_;
- // SQL string of this SelectStmt before inline-view expression substitution.
- // Set in analyze().
- protected String sqlString_;
-
// substitutes all exprs in this select block to reference base tables
// directly
private ExprSubstitutionMap baseTblSmap_ = new ExprSubstitutionMap();
@@ -79,6 +75,10 @@ public class SelectStmt extends QueryStmt {
// END: Members that need to be reset()
/////////////////////////////////////////
+ // SQL string of this SelectStmt before inline-view expression substitution.
+ // Set in analyze().
+ protected String sqlString_;
+
SelectStmt(SelectList selectList,
FromClause fromClause,
Expr wherePredicate, ArrayList<Expr> groupingExprs,
@@ -1029,10 +1029,13 @@ public class SelectStmt extends QueryStmt {
selectList_.reset();
colLabels_.clear();
fromClause_.reset();
- baseTblSmap_.clear();
if (whereClause_ != null) whereClause_.reset();
if (groupingExprs_ != null) Expr.resetList(groupingExprs_);
if (havingClause_ != null) havingClause_.reset();
+ havingPred_ = null;
+ aggInfo_ = null;
+ analyticInfo_ = null;
+ baseTblSmap_.clear();
}
@Override
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/915a1634/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java b/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
index 8c1de39..176b89e 100644
--- a/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
+++ b/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
@@ -51,14 +51,25 @@ public class SimplifyConditionalsRule implements ExprRewriteRule {
public Expr apply(Expr expr, Analyzer analyzer) throws AnalysisException {
if (!expr.isAnalyzed()) return expr;
+ Expr simplified;
if (expr instanceof FunctionCallExpr) {
- return simplifyFunctionCallExpr((FunctionCallExpr) expr);
+ simplified = simplifyFunctionCallExpr((FunctionCallExpr) expr);
} else if (expr instanceof CompoundPredicate) {
- return simplifyCompoundPredicate((CompoundPredicate) expr);
+ simplified = simplifyCompoundPredicate((CompoundPredicate) expr);
} else if (expr instanceof CaseExpr) {
- return simplifyCaseExpr((CaseExpr) expr, analyzer);
+ simplified = simplifyCaseExpr((CaseExpr) expr, analyzer);
+ } else {
+ return expr;
}
- return expr;
+
+ // IMPALA-5125: We can't eliminate aggregates as this may change the meaning of the
+ // query, for example:
+ // 'select if (true, 0, sum(id)) from alltypes' != 'select 0 from alltypes'
+ if (expr != simplified && expr.contains(Expr.isAggregatePredicate())
+ && !simplified.contains(Expr.isAggregatePredicate())) {
+ return expr;
+ }
+ return simplified;
}
/**
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/915a1634/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java b/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
index 41dbeb8..3ee4141 100644
--- a/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
@@ -51,7 +51,7 @@ public class ExprRewriteRulesTest extends FrontendTestBase {
SelectStmt stmt = (SelectStmt) ParsesOk(stmtStr);
Analyzer analyzer = createAnalyzer(Catalog.DEFAULT_DB);
stmt.analyze(analyzer);
- Expr origExpr = stmt.getResultExprs().get(0);
+ Expr origExpr = stmt.getSelectList().getItems().get(0).getExpr();
String origSql = origExpr.toSql();
ExprRewriter rewriter = new ExprRewriter(rules);
Expr rewrittenExpr = rewriter.rewrite(origExpr, analyzer);
@@ -324,6 +324,15 @@ public class ExprRewriteRulesTest extends FrontendTestBase {
RewritesOk("decode(id, null, 0, 1)", rules, null);
// All non-constant, don't rewrite.
RewritesOk("decode(id, 1, 1, 2, 2)", rules, null);
+
+ // IMPALA-5125: Exprs containing aggregates should not be rewritten if the rewrite
+ // eliminates all aggregates.
+ RewritesOk("if(true, 0, sum(id))", rule, null);
+ RewritesOk("if(false, max(id), min(id))", rule, "min(id)");
+ RewritesOk("true || sum(id) = 0", rule, null);
+ RewritesOk("case when true then 0 when false then sum(id) end", rule, null);
+ RewritesOk(
+ "case when true then count(id) when false then sum(id) end", rule, "count(id)");
}
@Test
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/915a1634/testdata/workloads/functional-query/queries/QueryTest/exprs.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/exprs.test b/testdata/workloads/functional-query/queries/QueryTest/exprs.test
index fa3d225..d618a24 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/exprs.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/exprs.test
@@ -2733,4 +2733,12 @@ select distinct case when true then id else 0 end from functional.alltypestiny
7
---- TYPES
INT
+====
+---- QUERY
+# IMPALA-5125: test behavior when an agg function could be eliminated by expr rewrites.
+select if (true, 0, sum(id)) from functional.alltypestiny
+---- RESULTS
+0
+---- TYPES
+BIGINT
====
\ No newline at end of file