Dan Hecht has posted comments on this change. Change subject: IMPALA-4192: Avoid updating Expr's states from ExprContext ......................................................................
Patch Set 2: (7 comments) http://gerrit.cloudera.org:8080/#/c/5483/2//COMMIT_MSG Commit Message: Line 29: FunctionContext, ignorning the error set by its descendants. is there a way to regression test that? http://gerrit.cloudera.org:8080/#/c/5483/2/be/src/exprs/expr-context.h File be/src/exprs/expr-context.h: Line 109: /// Creates a FunctionContext and stores it at 'fn_contexts_[fn_context_index]'. public interfaces shouldn't be explained in terms of private members. is there a way to state this in a way that makes sense outside of this class? how does the caller know what value to use for fn_context_index? will this go away eventually? see my comments elsewhere about possibly getting rid of this. http://gerrit.cloudera.org:8080/#/c/5483/2/be/src/exprs/expr.h File be/src/exprs/expr.h: Line 158: inline int fn_context_index() const { return fn_context_index_; } this index used to be more opaque to this class -- it was determined by FunctionContext. Now that these are assigned by the Expr class (which I think is okay), I think it needs a little bit of explanation (beyond what is stated below in the private fields). Like a brief class level comment explaining the relationship between Expr and ExprContext/ FunctionContext. Line 159: inline int NumFnContexts() const { why is this needed outside this class? Line 184: /// TODO: Separate the creation of Expr from ExprContext. does this TODO also impact the next two methods? Line 394: void RegisterFnContexts(ExprContext* ctx, RuntimeState* state); i wonder if it'd be clearer/simpler to just move this code into ExprContext. i.e. have expr context walk the expr tree itself and do these allocations to fill out its own vector. And then we don't need this public method nor ExprContext::RegisterFnContext(), so it seems it might simplify things. PS2, Line 440: int* node_idx, bool use_all_nodes explain these -- 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
