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-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:

(6 comments)

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.

http://gerrit.cloudera.org:8080/#/c/5483/2/be/src/exprs/expr-context.h
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
delineated.

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_'
and
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.


http://gerrit.cloudera.org:8080/#/c/5483/2/be/src/exprs/expr.h
File be/src/exprs/expr.h:

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


-- 
To view, visit http://gerrit.cloudera.org:8080/5483
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Iefdc9aeeba033355cb9497e3a5d2363627dcf2f3
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message