Marcel Kornacker has posted comments on this change. Change subject: PREVIEW: IMPALA-1788: Fold constant expressions during plan generation. ......................................................................
Patch Set 1: (20 comments) http://gerrit.cloudera.org:8080/#/c/5109/1/be/src/exprs/expr-context.cc File be/src/exprs/expr-context.cc: Line 155: void* value = GetValue(NULL); how does this handle/bail out of cases where exprctx is accidentally not a constant expr? http://gerrit.cloudera.org:8080/#/c/5109/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 1541: // The rewrite rule for extracting conjuncts simplifies these to NULL. rather than special-casing it here, it would make more sense to special-case GetValue() so you don't need to compare a type. http://gerrit.cloudera.org:8080/#/c/5109/1/be/src/exprs/literal.h File be/src/exprs/literal.h: Line 26: #include "runtime/string-value.h" duplicated http://gerrit.cloudera.org:8080/#/c/5109/1/common/thrift/Exprs.thrift File common/thrift/Exprs.thrift: Line 41: TIMESTAMP_LITERAL please group all the literals together, so we can easily see what's still missing. http://gerrit.cloudera.org:8080/#/c/5109/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: Line 242: fnName = path.get(path.size() - 1); why is this necessary? http://gerrit.cloudera.org:8080/#/c/5109/1/fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java File fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java: Line 204: if (Double.isNaN(val.double_val) || Double.isInfinite(val.double_val)) return null; nice catch! long line Line 226: for (byte b: bytes) { single line Line 233: result = new StringLiteral(strVal, constExpr.getType(), false); did you add a new test that blows up with the previous default unescaping? also, this retains the type of the original expr, which seems like the right thing to do (and what we previously did was the wrong thing to do). does this warrant another test? http://gerrit.cloudera.org:8080/#/c/5109/1/fe/src/main/java/org/apache/impala/analysis/TimestampLiteral.java File fe/src/main/java/org/apache/impala/analysis/TimestampLiteral.java: Line 34: * produce, so it better to defer to a single source of truth (the BE implementation). it -> it's http://gerrit.cloudera.org:8080/#/c/5109/1/fe/src/main/java/org/apache/impala/planner/AggregationNode.java File fe/src/main/java/org/apache/impala/planner/AggregationNode.java: Line 164: if (analyzer.getQueryOptions().enable_expr_rewrites && rewriter != null) { pass in null for the rewriter if it's disabled, so you don't have to check that as well. move the null check into rewriteExprs() so you don't have to check that either. http://gerrit.cloudera.org:8080/#/c/5109/1/fe/src/main/java/org/apache/impala/planner/JoinNode.java File fe/src/main/java/org/apache/impala/planner/JoinNode.java: Line 523: // constant into nested-loop joins. refer to cross products, to avoid confusion with future index nlj. http://gerrit.cloudera.org:8080/#/c/5109/1/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java File fe/src/main/java/org/apache/impala/planner/KuduScanNode.java: Line 373 do we still need this function for anything? there's a call in the partition pruner, but that should also go. http://gerrit.cloudera.org:8080/#/c/5109/1/fe/src/main/java/org/apache/impala/planner/PlanNode.java File fe/src/main/java/org/apache/impala/planner/PlanNode.java: Line 434: public void initNoRewrite(Analyzer analyzer) throws ImpalaException { why not just init(Analyzer)? that would also make it clear that you're not applying transformation rules http://gerrit.cloudera.org:8080/#/c/5109/1/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: Line 577: emptySetNode.init(ctx_.getRewriter(), analyzer); instead of passing this in, maybe each PlanNode should have access to the ctx_ http://gerrit.cloudera.org:8080/#/c/5109/1/fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java File fe/src/main/java/org/apache/impala/rewrite/ExprRewriter.java: Line 93: public void rewriteList(List<Expr> exprs, Analyzer analyzer) throws AnalysisException { why rewrite as opposed to apply here? http://gerrit.cloudera.org:8080/#/c/5109/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: Line 204: * Only contains very basic tests for a few interesting cases. A more thorough ". More thorough" http://gerrit.cloudera.org:8080/#/c/5109/1/testdata/workloads/functional-planner/queries/PlannerTest/conjunct-ordering.test File testdata/workloads/functional-planner/queries/PlannerTest/conjunct-ordering.test: Line 92: predicates: a.int_col = 0, a.timestamp_col > TIMESTAMP '2000-01-01 00:00:00' cool, that'll make timestamp operations a lot faster. hm, i'm wondering how to test that/prevent that from accidentally regressing. maybe a microbenchmark would work. http://gerrit.cloudera.org:8080/#/c/5109/1/testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test File testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test: Line 121: | output: sum(1 + 1 + id), count:merge(*) hm, a bit confusing that it prints the original "column names'. it would be better if it would inherit the printed names from the 1st agg phase. (fine to leave as todo, unless easy to do right now). http://gerrit.cloudera.org:8080/#/c/5109/1/testdata/workloads/functional-planner/queries/PlannerTest/join-order.test File testdata/workloads/functional-planner/queries/PlannerTest/join-order.test: Line 1502: | predicates: timestamp_col = TIMESTAMP '2016-11-15 21:38:54.000000618' that's not good, this means every time someone runs the test it'll break. maybe now() shouldn't show up in planner tests. http://gerrit.cloudera.org:8080/#/c/5109/1/testdata/workloads/functional-planner/queries/PlannerTest/kudu.test File testdata/workloads/functional-planner/queries/PlannerTest/kudu.test: Line 205: predicates: CAST(o_orderdate AS TIMESTAMP) <= TIMESTAMP '1995-01-01 00:00:00' aren't there tests for the specialized constant folding we have for kudu? -- To view, visit http://gerrit.cloudera.org:8080/5109 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If672b703db1ba0bfc26e5b9130161798b40a69e9 Gerrit-PatchSet: 1 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: Marcel Kornacker <mar...@cloudera.com> Gerrit-HasComments: Yes
