Tim Armstrong has posted comments on this change. Change subject: IMPALA-4574: Do not treat UUID() like a constant expr. ......................................................................
Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/5324/1//COMMIT_MSG Commit Message: Line 23: 1. Pass a flag from FE to BE for ever Expr indicating its constness. I think there are two low-risk ways we could solve the problem. One approach: 1. Change isConstant() to take a bool[] of the children's constness. isConstant() with no args would then be a wrapper around this. 2. Change TreeToThriftHelper() to collect const information during the traversal so isConstant() can be computed in a single pass of the tree. Second approach: 1. Add a mode to isConstant() or a helper function that sets an isConstant_ flag during the traversal. 2. Clear the flags after the traversal in treeToThriftHelper. I'm open to doing this as follow-up work, since I think we need a sane way to specify whether a function is deterministic that doesn't involve hardcoding the function name. http://gerrit.cloudera.org:8080/#/c/5324/1/be/src/exprs/scalar-fn-call.cc File be/src/exprs/scalar-fn-call.cc: Line 261: if (fn_.name.function_name == "rand" || fn_.name.function_name == "random" Are the names always converted to lower case? http://gerrit.cloudera.org:8080/#/c/5324/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: Line 245: // Non-deterministic functions are never constant. If we're sticking with this solution, should add comments that cross-reference the two places that need to be kept in sync. Line 246: if (fnName.equalsIgnoreCase("rand") || fnName.equalsIgnoreCase("random") I think we still need to consider what to do about non-deterministic UDFs. Previously they would have mostly worked aside from a couple of cases like partition-pruning. Ideally we'd have some DML to specify this, but maybe we should add a safety-valve query option like assume_udfs_nondeterministic to revert to the old behaviour? Could be a follow-up or separate JIRA. http://gerrit.cloudera.org:8080/#/c/5324/1/testdata/workloads/functional-query/queries/QueryTest/exprs.test File testdata/workloads/functional-query/queries/QueryTest/exprs.test: Line 2579: select count(*) from functional.alltypestiny group by uuid() Can you add a test where UUID() is an argument to an operator or function? I want to make sure we test the code path where there is a constant arg to ScalarFnCall. -- To view, visit http://gerrit.cloudera.org:8080/5324 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If2499f5f6ecdcb098623202c8e6dc2d02727194a Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
