impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1788: Fold constant expressions.
Date Tue, 22 Nov 2016 22:18:47 GMT
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 <alex.behm@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message