Michael Ho has posted comments on this change. Change subject: IMPALA-4192: Disentangle Expr and ExprContext ......................................................................
Patch Set 4: (12 comments) http://gerrit.cloudera.org:8080/#/c/5483/4//COMMIT_MSG Commit Message: PS4, Line 31: soly > solely Done PS4, Line 39: ignorning > ignoring Done http://gerrit.cloudera.org:8080/#/c/5483/4/be/src/exec/hash-table-test.cc File be/src/exec/hash-table-test.cc: Line 74: ASSERT_OK(ExprContext::Prepare(build_expr_ctxs_, NULL, desc, &tracker_)) > Missing semi-colon. Should have compiled the tests too. Done 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 he Done 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 tho Done Line 102: /// Convenience function for closing multiple expr trees. > Are we closing the expr trees or the contexts? Same for the comments above Done http://gerrit.cloudera.org:8080/#/c/5483/4/be/src/exprs/expr.cc File be/src/exprs/expr.cc: Line 412: string Expr::DebugString(const vector<ExprContext*>& ctxs) { > This should probably be moved to ExprContext too, right? Done 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" expressions' static states Line 110: /// TODO: > Remove TODO's while we're here? These seem too non-specific to be useful. Done PS4, Line 354: on the expr tree. > It's really opening the ExprContext and FunctionContexts instead of the exp It's about initializing ExprContext and FunctionContexts for all nodes in the ExprTree. Comment rephrased. Line 358: /// Subclasses overriding this function should call Expr::Close(). > This really closes the ExprContext instead of the Expr. Renamed to Expr::CloseContext(). PS4, Line 387: FunctionContext > FunctionContexts Done -- 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
