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