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-4586: don't constant fold in backend
Date Wed, 07 Dec 2016 06:06:54 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-4586: don't constant fold in backend
......................................................................


Patch Set 3:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/5391/3//COMMIT_MSG
Commit Message:

Line 15: constant fold all constant expressions anyway.
Explicitly state what the user-level workaround: Set enable_expr_rewrites=false, correct?


http://gerrit.cloudera.org:8080/#/c/5391/3/be/src/exprs/expr-context.h
File be/src/exprs/expr-context.h:

Line 93:   void EvaluateWithoutRow(TColumnValue* col_val);
These changes are not necessary for the fix, so in the interest of keeping this change minimal
we could consider leaving them out. I'll let Dan make the call on that. I'm ok with these
changes either way.


http://gerrit.cloudera.org:8080/#/c/5391/3/be/src/exprs/hive-udf-call.cc
File be/src/exprs/hive-udf-call.cc:

Line 331:   uint8_t* local_alloc = fn_ctx->impl()->AllocateLocal(result.len);
Random note: We need to check for a NULL return value here.


http://gerrit.cloudera.org:8080/#/c/5391/3/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:

Line 216:       // evaluated (see IMPALA-4586).
Might be worth explicitly pointing out the scenario we are worried about: non-deterministic
UDFs with no or only constant arguments. This condition allows us to fallback to not treating
those as constant by setting enable_expr_rewrites=false


http://gerrit.cloudera.org:8080/#/c/5391/3/fe/src/main/java/org/apache/impala/service/FeSupport.java
File fe/src/main/java/org/apache/impala/service/FeSupport.java:

Line 123:     Preconditions.checkState(expr.isConstant());
!expr.contains(SlotRef.class)


Line 196:    * exprs. In the future, we can extend it to support arbitrary constant exprs.
update comment (not only valid for "constant" exprs)


Line 205:       Preconditions.checkState(expr.isConstant());
check whether SlotRefs are contained instead


http://gerrit.cloudera.org:8080/#/c/5391/3/tests/query_test/test_udfs.py
File tests/query_test/test_udfs.py:

Line 232: class TestUdfExecution(TestUdfBase):
won't inheriting also run the same set of tests in TestUdfBase again?


Line 296:     vector = copy(vector)
skip or return if codegen is enabled, instead? looks like we're running the same test twice


Line 378:   def test_java_udfs(self, vector, unique_database):
aren't these interesting with constant folding as well?


-- 
To view, visit http://gerrit.cloudera.org:8080/5391
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0c76e3c8a8d92749256c312080ecd7aac5d99ce7
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message