impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <>
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:

File be/src/exprs/

Line 155:   void* value = GetValue(NULL);
how does this handle/bail out of cases where exprctx is accidentally not a constant expr?
File be/src/exprs/

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.
File be/src/exprs/literal.h:

Line 26: #include "runtime/string-value.h"
File common/thrift/Exprs.thrift:

please group all the literals together, so we can easily see what's still missing.
File fe/src/main/java/org/apache/impala/analysis/

Line 242:       fnName = path.get(path.size() - 1);
why is this necessary?
File fe/src/main/java/org/apache/impala/analysis/

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?
File fe/src/main/java/org/apache/impala/analysis/

Line 34:  * produce, so it better to defer to a single source of truth (the BE implementation).
it -> it's
File fe/src/main/java/org/apache/impala/planner/

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.
File fe/src/main/java/org/apache/impala/planner/

Line 523:       // constant into nested-loop joins.
refer to cross products, to avoid confusion with future index nlj.
File fe/src/main/java/org/apache/impala/planner/

Line 373
do we still need this function for anything?

there's a call in the partition pruner, but that should also go.
File fe/src/main/java/org/apache/impala/planner/

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
File fe/src/main/java/org/apache/impala/planner/

Line 577:       emptySetNode.init(ctx_.getRewriter(), analyzer);
instead of passing this in, maybe each PlanNode should have access to the ctx_
File fe/src/main/java/org/apache/impala/rewrite/

Line 93:   public void rewriteList(List<Expr> exprs, Analyzer analyzer) throws AnalysisException
why rewrite as opposed to apply here?
File fe/src/test/java/org/apache/impala/analysis/

Line 204:    * Only contains very basic tests for a few interesting cases. A more thorough
". More thorough"
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.
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).
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.
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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: If672b703db1ba0bfc26e5b9130161798b40a69e9
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-HasComments: Yes

View raw message