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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
