You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2016/12/06 22:11:28 UTC
incubator-impala git commit: IMPALA-4574: Do not treat UUID() like a
constant expr.
Repository: incubator-impala
Updated Branches:
refs/heads/master 9b80224f9 -> 534999382
IMPALA-4574: Do not treat UUID() like a constant expr.
A recent change (IMPALA-1788) lead UUID() to be constant folded,
and therefore, produce the same value for every invocation across
rows. Similar issues might also occur due to the BE optimizing
UUID() during codegen of scalar-fn-call.h/cc.
The fix is to not treat UUID() like a constant expr in both
the FE and BE.
Discussion:
The fix in this patch is rather blunt, but minimally invasive to
reduce the risk of adding new bugs. Ideally, the constness of an
Expr should be determined in one place and the FE and BE should agree
on which Exprs are constant. I considered the following alternatives
but concluded they were too risky:
1. Pass a flag from FE to BE for ever Expr indicating its constness.
This simple solution would populate a thrift field with the
result of Expr.isConstant() for every Expr in an Expr tree.
There are several issues. Calling isConstant() for every Expr
in an Expr tree is rather expensive due to repeated traversals
of the tree. That could be mitigated by populating an isConstant
flag during Expr.analyze() to avoid re-computing the constness
repeatedly. This requires changes to analyze(), clone(), reset(),
and possibly other places for many Exprs. There is potential
for missing a place and adding a new bug.
2. The above solution could be limited to only FunctionCallExpr.
However, the BE expr type FUNCTION_CALL which maps to
scalar-fn-call.h/cc is created from various FE Exprs, not just
FunctionCallExpr. So adding a flag only to scalar-fn-call.h/cc
would be confusing because it would only sometimes be set
in a meaningful way. This seems more confusing than the current
straightforward solution.
Testing: Added FE and EE tests.
Change-Id: If2499f5f6ecdcb098623202c8e6dc2d02727194a
Reviewed-on: http://gerrit.cloudera.org:8080/5324
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Internal 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/53499938
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/53499938
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/53499938
Branch: refs/heads/master
Commit: 534999382d5e75ce49c57b4223adc1e3b021778b
Parents: 9b80224
Author: Alex Behm <al...@cloudera.com>
Authored: Thu Dec 1 23:12:30 2016 -0800
Committer: Internal Jenkins <cl...@gerrit.cloudera.org>
Committed: Tue Dec 6 22:03:01 2016 +0000
----------------------------------------------------------------------
be/src/exprs/scalar-fn-call.cc | 5 ++++-
be/src/exprs/scalar-fn-call.h | 2 ++
.../java/org/apache/impala/analysis/Analyzer.java | 4 +++-
.../org/apache/impala/analysis/FunctionCallExpr.java | 7 ++++++-
.../apache/impala/analysis/ExprRewriteRulesTest.java | 3 ++-
.../functional-query/queries/QueryTest/exprs.test | 15 +++++++++++++++
6 files changed, 32 insertions(+), 4 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/53499938/be/src/exprs/scalar-fn-call.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/scalar-fn-call.cc b/be/src/exprs/scalar-fn-call.cc
index c80cfcb..4cbd4d8 100644
--- a/be/src/exprs/scalar-fn-call.cc
+++ b/be/src/exprs/scalar-fn-call.cc
@@ -258,7 +258,10 @@ void ScalarFnCall::Close(RuntimeState* state, ExprContext* context,
}
bool ScalarFnCall::IsConstant() const {
- if (fn_.name.function_name == "rand") return false;
+ if (fn_.name.function_name == "rand" || fn_.name.function_name == "random"
+ || fn_.name.function_name == "uuid" || fn_.name.function_name == "sleep") {
+ return false;
+ }
return Expr::IsConstant();
}
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/53499938/be/src/exprs/scalar-fn-call.h
----------------------------------------------------------------------
diff --git a/be/src/exprs/scalar-fn-call.h b/be/src/exprs/scalar-fn-call.h
index 530f1b1..1cb3743 100644
--- a/be/src/exprs/scalar-fn-call.h
+++ b/be/src/exprs/scalar-fn-call.h
@@ -65,6 +65,8 @@ class ScalarFnCall: public Expr {
virtual void Close(RuntimeState* state, ExprContext* context,
FunctionContext::FunctionStateScope scope = FunctionContext::FRAGMENT_LOCAL);
+ /// Needs to be kept in sync with the FE understanding of constness in
+ /// FuctionCallExpr.java.
virtual bool IsConstant() const;
virtual BooleanVal GetBooleanVal(ExprContext* context, const TupleRow*);
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/53499938/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
index 6bba436..1e88862 100644
--- a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
+++ b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
@@ -1090,7 +1090,9 @@ public class Analyzer {
registerFullOuterJoinedConjunct(e);
// register single tid conjuncts
- if (tupleIds.size() == 1) globalState_.singleTidConjuncts.add(e.getId());
+ if (tupleIds.size() == 1 && !e.isAuxExpr()) {
+ globalState_.singleTidConjuncts.add(e.getId());
+ }
if (LOG.isTraceEnabled()) {
LOG.trace("register tuple/slotConjunct: " + Integer.toString(e.getId().asInt())
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/53499938/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java b/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
index 19f0e49..3452b05 100644
--- a/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
+++ b/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
@@ -232,6 +232,10 @@ public class FunctionCallExpr extends Expr {
}
}
+ /**
+ * Needs to be kept in sync with the BE understanding of constness in
+ * scalar-fn-call.h/cc.
+ */
@Override
public boolean isConstant() {
// Aggregate functions are never constant.
@@ -243,7 +247,8 @@ public class FunctionCallExpr extends Expr {
fnName = path.get(path.size() - 1);
}
// Non-deterministic functions are never constant.
- if (fnName.equalsIgnoreCase("rand") || fnName.equalsIgnoreCase("random")) {
+ if (fnName.equalsIgnoreCase("rand") || fnName.equalsIgnoreCase("random")
+ || fnName.equalsIgnoreCase("uuid")) {
return false;
}
// Sleep is a special function for testing.
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/53499938/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 f1b5195..f31d50d 100644
--- a/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
@@ -39,7 +39,6 @@ public class ExprRewriteRulesTest extends FrontendTestBase {
public Expr RewritesOk(String expr, ExprRewriteRule rule, String expectedExpr)
throws AnalysisException {
- System.out.println(expr);
String stmtStr = "select " + expr + " from functional.alltypessmall";
SelectStmt stmt = (SelectStmt) ParsesOk(stmtStr);
Analyzer analyzer = createAnalyzer(Catalog.DEFAULT_DB);
@@ -226,6 +225,8 @@ public class ExprRewriteRulesTest extends FrontendTestBase {
RewritesOk("hex(unhex(hex(unhex('D3'))))", rule, null);
// Tests that non-deterministic functions are not folded.
RewritesOk("rand()", rule, null);
+ RewritesOk("random()", rule, null);
+ RewritesOk("uuid()", rule, null);
// Tests that exprs that warn during their evaluation are not folded.
RewritesOk("coalesce(1.8, cast(int_col as decimal(38,38)))", rule, null);
}
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/53499938/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 f5e4bab..78c3e09 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/exprs.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/exprs.test
@@ -2622,3 +2622,18 @@ TIMESTAMP
---- ERRORS
UDF WARNING: Cannot subtract interval 1: Year is out of valid range: 1400..10000
====
+---- QUERY
+# IMPALA-4574: UUID() is not a constant expression.
+select count(*) from functional.alltypestiny group by concat(uuid(), "_test")
+---- RESULTS
+1
+1
+1
+1
+1
+1
+1
+1
+---- TYPES
+BIGINT
+====