Marcel Kornacker has posted comments on this change. Change subject: IMPALA-4192: Disentangle Expr and ExprContext ......................................................................
Patch Set 9: (45 comments) first half http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exec/exec-node.h File be/src/exec/exec-node.h: Line 285: boost::scoped_ptr<MemTracker> expr_mem_tracker_; > There will be one per PlanNode in the future. Currently, it still counts to if this is per querystate, the mempool would need to be thread-safe, not sure that's worth it (and if this is for evaluators, why have it per plannode?). also, since we now clearly distinguish exprs and their evaluators, and this is specifically for evaluators, let's express that in the variable name. expr_eval_mem_pool_? http://gerrit.cloudera.org:8080/#/c/5483/9/be/src/exprs/agg-fn-evaluator.cc File be/src/exprs/agg-fn-evaluator.cc: Line 96: const AggFn& agg_fn, AggFnEvaluator** result) { move input params to front Line 161: Status AggFnEvaluator::Open(vector<AggFnEvaluator*>& evaluators, RuntimeState* state) { why not const&? Line 514: for (int i = 0; i < evaluators.size(); ++i) { single line, here and subsequent functions, where possible http://gerrit.cloudera.org:8080/#/c/5483/9/be/src/exprs/agg-fn-evaluator.h File be/src/exprs/agg-fn-evaluator.h: Line 80: /// and caches all constant input arguments. regarding the caching: is there already a todo to move that into the expr setup (as opposed to evaluator setup)? http://gerrit.cloudera.org:8080/#/c/5483/9/be/src/exprs/agg-fn.cc File be/src/exprs/agg-fn.cc: Line 68: input_expr->AssignFnCtxIdx(&fn_ctx_idx); scalarexpr does that in create. do the same for this class? http://gerrit.cloudera.org:8080/#/c/5483/9/be/src/exprs/expr.cc File be/src/exprs/expr.cc: Line 87: const int num_children = nodes[*node_idx].num_children; we don't use 'const ..' elsewhere for automatic variables. Line 88: const bool isAggFn = root->IsAggFn(); formatting Line 92: // Reset the function context index for each input expression. what is this referring to? http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exprs/expr.h File be/src/exprs/expr.h: Line 81: /// status on failure. > 'fn_ctx_idx_ptr' removed. It doesn't take Expr** as 'root' is expected to b should this be a private function and the two subclasses be friends (to call this)? http://gerrit.cloudera.org:8080/#/c/5483/9/be/src/exprs/expr.h File be/src/exprs/expr.h: Line 82: static Status CreateTree(ObjectPool* pool, const TExpr& texpr, Expr* root); this is really CreateChildren, not CreateTree. move in/out params to end Line 134: /// Creates an expr tree with root 'parent' via depth-first traversal. misleading: the root of the created tree is not 'parent' (the root is installed as a new child into 'parent') Line 135: /// Called recursively to create expr trees of sub-expressions. "for subexpressions" Line 140: /// node_idx: index in 'nodes' of the root of the next child TExprNode tree let's call this root_idx, makes it easier to follow Line 146: static Status CreateTreeFromThrift(ObjectPool* pool, while we're cleaning up, move in/out params to end http://gerrit.cloudera.org:8080/#/c/5483/9/be/src/exprs/scalar-expr-evaluator.cc File be/src/exprs/scalar-expr-evaluator.cc: Line 1: // Licensed to the Apache Software Foundation (ASF) under one could you flag what's new in this file that i should look at? Line 69: inited_(false), initialize vars in .h where practical Line 82: (*evaluator)->fn_ctxs_.clear(); why do you need that (for a newly created object)? Line 86: for (FunctionContext* fn_ctx : (*evaluator)->fn_ctxs_) DCHECK(fn_ctx != nullptr); do that in CreateFnCtxs? Line 133: if (opened_) return Status::OK(); what's the need for guarding against multiple open calls? (how would that happen?) Line 138: is_clone_? FunctionContext::THREAD_LOCAL : FunctionContext::FRAGMENT_LOCAL; is this really instance_local? if so, leave todo in the right places (presumably at least udf.h) Line 155: delete fn_ctxs_[i]; instead put a unique_ptr into fn_ctxs? http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exprs/scalar-expr-evaluator.h File be/src/exprs/scalar-expr-evaluator.h: Line 72: std::vector<ScalarExprEvaluator*>* evaluators) WARN_UNUSED_RESULT; > Yes, ideally, that should be done in Expr::Init(). This is being tracked by todo? Line 111: /// should only be called after Open() has been called on this expr. Returns an error if > There is subtlety in this. "expr" is potentially a sub-expression of root_ but why not just call GetValue(nullptr) on the subexpr? http://gerrit.cloudera.org:8080/#/c/5483/9/be/src/exprs/scalar-expr-evaluator.h File be/src/exprs/scalar-expr-evaluator.h: Line 55: /// rooted at a node is defind by [fn_ctx_idx_start_, fn_ctx_idx_end_). defined Line 65: static Status Create(ObjectPool* pool, RuntimeState* state, MemPool* mem_pool, move in/out params to back Line 81: static Status Open(std::vector<ScalarExprEvaluator*>& evaluators, why a & and not a const&? Line 89: static void Close(std::vector<ScalarExprEvaluator*>& evaluators, RuntimeState* state); same here Line 108: /// If this expr is constant, evaluates the expr with no input row argument and returns 'this expr': does this refer to 'this' or to parameter 'expr'? same for 'the expr with...'. Line 182: /// and lives in the same object pool as this evaluator (i.e. same life span as the "and live" Line 205: bool inited_; initialized_ is less awkward http://gerrit.cloudera.org:8080/#/c/5483/9/be/src/exprs/scalar-expr.cc File be/src/exprs/scalar-expr.cc: Line 87: (*scalar_expr)->AssignFnCtxIdx(&fn_ctx_idx); is there a reason this gets called before init()? if so, point it out. http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exprs/scalar-expr.h File be/src/exprs/scalar-expr.h: Line 155: friend class AggFn; > move to Expr? ? http://gerrit.cloudera.org:8080/#/c/5483/9/be/src/exprs/scalar-expr.h File be/src/exprs/scalar-expr.h: Line 78: /// ScalarExpr contains static states about an expression (e.g. sub-expressions "static states": in expr.h you call this query compile-time information, let's stick with that to avoid confusing variations in terminology. Line 98: /// is stored in ObjectPool 'pool' and returned in 'expr'. 'row_desc' is the tuple row adapt comment (you're still referring to parameter pool) Line 192: /// Assigns indices into the FunctionContext vector in an evaluator to nodes which "functioncontext vector in an evaluator": include reference to exact member var to enhance clarity Line 194: void AssignFnCtxIdx(int* next_fn_ctx_idx); describe param Line 199: /// in 'expr'. 'fn_ctx_idx_ptr' is pointer to the index of the next available there is no more fn_ctx_idx_ptr Line 237: virtual Status OpenEvaluator(RuntimeState* state, ScalarExprEvaluator* evaluator, move in/out params to back Line 247: virtual void CloseEvaluator(RuntimeState* state, ScalarExprEvaluator* evaluator, same here Line 260: llvm::Function* CreateIrFunctionPrototype(LlvmCodeGen* codegen, const std::string& name, same here http://gerrit.cloudera.org:8080/#/c/5483/9/be/src/exprs/scalar-fn-call.cc File be/src/exprs/scalar-fn-call.cc: Line 164: bool need_interp = scalar_fn_wrapper_ == nullptr; 'interpreted' or 'is_interpreted' is a bit less awkward Line 179: // Only evaluate constant arguments at the top level of function contexts. what does 'top level of function contexts' refer to? http://gerrit.cloudera.org:8080/#/c/5483/9/be/src/runtime/data-stream-sender.h File be/src/runtime/data-stream-sender.h: Line 134: /// TODO: Move this to the equivalent of PlanNode class for DataSink. move to class comment, so it's not as easy to overlook. either of this class or of the parent. http://gerrit.cloudera.org:8080/#/c/5483/9/be/src/service/fe-support.cc File be/src/service/fe-support.cc: Line 78: static void SetTColumnValue(void* value, const ColumnType& type, TColumnValue* col_val) { move input params to front const void* value? this is odd, why is this here and not elsewhere? -- 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: 9 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
