impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4192: Avoid updating Expr's states from ExprContext
Date Wed, 04 Jan 2017 20:37:12 GMT
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 <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