Marcel Kornacker has posted comments on this change. Change subject: IMPALA-1788: Fold constant expressions. ......................................................................
Patch Set 2: (12 comments) http://gerrit.cloudera.org:8080/#/c/5109/2/be/src/exprs/literal.cc File be/src/exprs/literal.cc: Line 249 that looks weird. Line 472: case TYPE_TIMESTAMP: do we have end-to-end tests with timestamp literals? http://gerrit.cloudera.org:8080/#/c/5109/2/be/src/service/fe-support.cc File be/src/service/fe-support.cc: Line 105: query_ctx.request.query_options.max_errors = 10; 10? http://gerrit.cloudera.org:8080/#/c/5109/2/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: Line 423: private void getResultTypesAndLabels(StatementBase stmt, List<Type> resultTypes, it looks like a better approach is to add a StatementBase.getResultExprs and .getColLabels and override appropriately in the subclasses. Line 447: private void setResultTypesAndLabels(StatementBase stmt, List<Type> resultTypes, move this to StatementBase? (maybe as castResultExprs() and setColLabels()?) http://gerrit.cloudera.org:8080/#/c/5109/2/fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java File fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java: Line 177: System.out.println(e.getMessage()); remove http://gerrit.cloudera.org:8080/#/c/5109/2/fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java File fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java: Line 48: for (Expr child: expr.getChildren()) { single line http://gerrit.cloudera.org:8080/#/c/5109/2/fe/src/test/java/org/apache/impala/testutil/TestUtils.java File fe/src/test/java/org/apache/impala/testutil/TestUtils.java: Line 260: // Disable rewrites because some analyzer tests have non-executable constant exprs does this mean they're disabled in the planner tests? 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(2 + id), count:merge(*) > Agree. It's not super easy to fix because the agg info is computed during a get rid of which part? i agree we don't need the 1+1 here. http://gerrit.cloudera.org:8080/#/c/5109/2/testdata/workloads/functional-planner/queries/PlannerTest/insert.test File testdata/workloads/functional-planner/queries/PlannerTest/insert.test: Line 551: WRITE TO HDFS [functional.alltypes, OVERWRITE=false, PARTITION-KEYS=(2009,5)] oh nice http://gerrit.cloudera.org:8080/#/c/5109/2/testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test File testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test: Line 9: tuple-ids=0 row-size=68B cardinality=unavailable what happened here, did something get screwed up when loading kudu data? http://gerrit.cloudera.org:8080/#/c/5109/2/testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test File testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test: Line 1270: predicates: g.bigint_col = 1, g.bigint_col < 1000 do you know why these get reordered? it seems like =1 should have been the first one to start with. -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-HasComments: Yes
