Hello Impala Public Jenkins, Internal Jenkins, Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5324
to look at the new patch set (#6).
Change subject: IMPALA-4574: Do not treat UUID() like a constant expr.
......................................................................
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
---
M be/src/exprs/scalar-fn-call.cc
M be/src/exprs/scalar-fn-call.h
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
6 files changed, 32 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/5324/6
--
To view, visit http://gerrit.cloudera.org:8080/5324
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If2499f5f6ecdcb098623202c8e6dc2d02727194a
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
