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] PREVIEW: IMPALA-1788: Fold constant expressions during plan generation.
Date Thu, 17 Nov 2016 05:48:53 GMT
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.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