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] IMPALA-4192: Avoid updating Expr's states from ExprContext
Date Thu, 05 Jan 2017 01:24:54 GMT
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4192: Avoid updating Expr's states from ExprContext

Patch Set 2:


i'm not sure this chunk of work is a meaningful intermediate milestone or whether at this
stage the triumvirate of expr/exprctx/functionctx needs to be rethought/tossed out. 

i find the cross-dependencies of these classes hard to follow.
File be/src/exprs/expr-context.h:

Line 44: /// concurrently. A single ExprContext is not thread-safe.
is it time to clean up the terminology as well? i (and apparently almost everybody else as
well) have always found this collection of classes (exprctx, functionctx) confusing and poorly

also, i'd like to see a crisp characterization of the division of labor between expr and exprctx
(in particular if expr only contains static data/data that doesn't change after initialization),
the class comments for those two classes are pretty meaningless.

Line 52:   static ExprContext* Create(ObjectPool* pool, Expr* expr);
how is this different from the preceding c'tor and when would i call one vs the other? do
we still need the c'tor to be public?

Line 99:   /// have any error. ['start', 'end') is the range in 'fn_contexts_' for 'root_'
don't reference internal state in the comments for public functions.

Line 110:   /// Called by Expr::Prepare() for exprs which need FunctionContexts (i.e. its
there still seems to be some entanglement of expr and exprctx. if exprs are purely static,
there shouldn't be backreferences to exprctx.

i agree with dan that this interface, taken on its own, doesn't make sense. this might need
more invasive surgery/wholesale restructuring.

Line 130:   int output_scale() const { return output_scale_; }
why is this part of the exprctx and not the expr itself? this is static data.
File be/src/exprs/expr.h:

Line 196:   static Status CreateInputExprTrees(ObjectPool* pool, const TExpr& texpr,
why is this public?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Iefdc9aeeba033355cb9497e3a5d2363627dcf2f3
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message