impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4192: Disentangle Expr and ExprContext
Date Wed, 19 Apr 2017 19:32:56 GMT
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4192: Disentangle Expr and ExprContext
......................................................................


Patch Set 7:

(31 comments)

initial comments

http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exec/exec-node.h
File be/src/exec/exec-node.h:

Line 49: /// flag gets set.
leave todo to outline very briefly what's going to happen with PlanNode.


Line 285:   /// Created in Prepare().
is there a need to have one per exec node? how about one per fragment instance?


http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exprs/agg-fn.h
File be/src/exprs/agg-fn.h:

Line 21: /// An aggregate function generates an output over a set of tuple rows..
..


Line 28: /// AggFn contains static states about an aggregate function such as the aggregation
the comment about "static states"/compile-time information applies to all exprs and should
move in the class comment for Expr (adapted, of course).


Line 97:   /// 'output_slot_desc' is the slot descriptor of the output value. Returns error
can intermediate and output slodesc be null?


Line 101:       const SlotDescriptor* output_slot_desc, const TExpr& texpr, AggFn** agg_fn);
who owns agg_fn?


http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exprs/case-expr.h
File be/src/exprs/case-expr.h:

Line 38:   friend class ScalarExpr;
this is needed?


Line 41:   CaseExpr(const TExprNode& node, int fn_context_index);
it would be nice to keep the fnctx index out of every class. is it possible to have only generic
logic in Expr deal with this? (could createtree set this in a final pass over the tree?)


http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exprs/expr.h
File be/src/exprs/expr.h:

Line 19: /// --- Expr overview
move directly above class, not sure why the class comment was up here to start with


Line 34: /// Please see the headers of ScalarExpr and AggFn for details.
explain function context index concept here


Line 81:       ObjectPool* pool, const TExpr& texpr, Expr* root, int* fn_ctx_idx_ptr);
why does this not take an Expr**?

unclear what the significance of the idx_ptr is at this point.


Line 113:   LibCacheEntry* cache_entry_;
initialize all pointers to nullptr here, that way you can't forget it in the c'tor.


Line 134:   void AddChild(ScalarExpr* expr) { children_.push_back(expr); }
doesn't look like a useful function (if you see it being called you probably want to look
at the implementation to figure out if it does anything beyond the obvious).


Line 137:   /// Called recursively to create expr tree of sub-expression.
"to create expr trees for subexpressions"


http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exprs/scalar-expr.h
File be/src/exprs/scalar-expr.h:

Line 28: /// ScalarExpr implements compute functions which given a row, performs the computation
stick to singular or plural in this and the following sentence.

which,


Line 40: /// in FunctionContext in ScalarExprEvaluator which is the interface to evaluate
a
..evaluator,


Line 44: /// initializes itself and frees its resources by calling OpenEvaluator() and
move comments about evaluator to evaluator class comment.


Line 50: /// GetCodegendComputeFn() to either generate custom IR compute functions using IRBuilder
..builder, which inlines ...functions, or...


Line 107:   static Status Create(ObjectPool* pool, RuntimeState* state,
why do we need this and Expr::CreateTree?


Line 155:   static void Close(std::vector<ScalarExpr*>& exprs);
move to Expr?


Line 223:   /// Initializes the static states of all nodes in the expr tree.
what is the non-static state in the expr tree?


Line 280:   /// evaluator and initialized by calling ScalarExpr::OpenEvaluator().
would be good to have that in the Expr class comment, maybe with some more context.


http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/runtime/data-stream-sender.cc
File be/src/runtime/data-stream-sender.cc:

Line 379:     const vector<TExpr>& thrift_output_exprs) {
unused parameter


http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/runtime/data-stream-sender.h
File be/src/runtime/data-stream-sender.h:

Line 133:   /// per-row partition values for shuffling exchange;
also leave todo that the exprs go into a separate class.


http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/runtime/data-stream-test.cc
File be/src/runtime/data-stream-test.cc:

Line 198:   scoped_ptr<MemPool> mem_pool_;
more mem pools?


Line 207:   vector<ScalarExpr*> ordering_exprs_;
?


http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/runtime/descriptors.h
File be/src/runtime/descriptors.h:

Line 288:   /// TODO: Move these into the new query-wide state, indexed by partition id.
does this todo still apply, given that the descriptor tbl itself moves into the querystate?


Line 291:   std::vector<ScalarExprEvaluator*> partition_key_value_evaluators_;
why is there an evaluator in here, instead of the expr? this is shared by all instance executors.


http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/runtime/sorter.h
File be/src/runtime/sorter.h:

Line 107:   ///
?


Line 120:   /// XXX
?


http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:

Line 76: static void SetTColumnValue(void* value, ScalarExprEvaluator* evaluator,
function comment missing


-- 
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: 7
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