Alex Behm has posted comments on this change. Change subject: IMPALA-1788: Fold constant expressions. ......................................................................
Patch Set 2: (11 comments) Thanks for the speedy review, Marcel! http://gerrit.cloudera.org:8080/#/c/5109/2/be/src/exprs/literal.cc File be/src/exprs/literal.cc: Line 249 > that looks weird. yes, we were doing something very strange before Line 472: case TYPE_TIMESTAMP: > do we have end-to-end tests with timestamp literals? There are quite a few interesting tests in exprs.test. I modified test_exprs.py to run with and without constant folding to make sure we don't lose the old coverage. We also have a few end-to-end tests littered in various .test files (checked it with git grep). I extended exprs.test to give more coverage. 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? Arbitrary non-zero number. Changed to 1 and updated comment. 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 an Done Line 447: private void setResultTypesAndLabels(StatementBase stmt, List<Type> resultTypes, > move this to StatementBase? (maybe as castResultExprs() and setColLabels()? Done 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 Done 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 Done 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? No. The planner tests always explicitly set the query options, so they are not affected by changing the default here. I also added a comment to this function that expr rewrites are disabled by default. 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? My Kudu setup was messed up again (needed to compute stats). Fixed it. 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 The original expr was g.bigint_col = TRUE which had a CastExpr on the RHS, so it appeared more expensive. After folding we just have a literal. -- 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
