Tim Armstrong has posted comments on this change. Change subject: IMPALA-4192: Disentangle Expr and ExprContext ......................................................................
Patch Set 4: (12 comments) I had some high level comments. Still trying to understand the flow of execution with the various Open()/Close()/Clone() etc calls. http://gerrit.cloudera.org:8080/#/c/5483/4//COMMIT_MSG Commit Message: PS4, Line 31: soly solely PS4, Line 39: ignorning ignoring http://gerrit.cloudera.org:8080/#/c/5483/4/be/src/exprs/expr-context.cc File be/src/exprs/expr-context.cc: PS4, Line 129: RegisterFnContexts CreateFnContexts()? I don't think there's any real registration going on here. http://gerrit.cloudera.org:8080/#/c/5483/4/be/src/exprs/expr-context.h File be/src/exprs/expr-context.h: Line 86: /// Use this interface except for creating ExprContext on a stack. It looks like we only do this in two places in ExprTest. Why not change those to use Create() and hide the constructor? Line 102: /// Convenience function for closing multiple expr trees. Are we closing the expr trees or the contexts? Same for the comments above too. http://gerrit.cloudera.org:8080/#/c/5483/4/be/src/exprs/expr.cc File be/src/exprs/expr.cc: Line 376: Status Expr::Prepare(RuntimeState* state, const RowDescriptor& row_desc) { It's kind of weird that the scope of what is initialised is different for Expr::Prepare() vs Expr::Open() and Expr::Close(). Expr::Prepare() just initialises the Expr and doesn't do anything with ExprContext, but Expr::Open() and Expr::Close() initialize the ExprContext. We need something like this because there's Expr-subclass-specific logic in Open() and Close(). Maybe Open() and Close() should be OpenContext() and CloseContext() instead so the distinction is clearer? Line 412: string Expr::DebugString(const vector<ExprContext*>& ctxs) { This should probably be moved to ExprContext too, right? http://gerrit.cloudera.org:8080/#/c/5483/4/be/src/exprs/expr.h File be/src/exprs/expr.h: PS4, Line 100: its : /// states What is "its states" Line 110: /// TODO: Remove TODO's while we're here? These seem too non-specific to be useful. PS4, Line 354: on the expr tree. It's really opening the ExprContext and FunctionContexts instead of the expr tree, right? Line 358: /// Subclasses overriding this function should call Expr::Close(). This really closes the ExprContext instead of the Expr. This makes sense since we may need Expr-subclass-specific logic, but the comment should be clearer. PS4, Line 387: FunctionContext FunctionContexts -- 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: 4 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
