impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4574: Do not treat UUID() like a constant expr.
Date Tue, 06 Dec 2016 04:50:55 GMT
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 (#5).

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, 31 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/24/5324/5
-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>

Mime
View raw message