Marcel Kornacker has posted comments on this change. Change subject: IMPALA-4192: Disentangle Expr and ExprContext ......................................................................
Patch Set 7: (25 comments) some more comments. i haven't looked at the exec nodes in detail yet. http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exec/aggregation-node.h File be/src/exec/aggregation-node.h: Line 84: std::vector<ScalarExpr*> grouping_exprs_; where are the evaluators for these? http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exec/analytic-eval-node.h File be/src/exec/analytic-eval-node.h: Line 181: bool PrevRowCompare(ScalarExprEvaluator* pred_ctx); rename variables ending in _ctx if they're referring to now-obsolete class names. http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exprs/agg-fn-evaluator.h File be/src/exprs/agg-fn-evaluator.h: Line 54: /// AggFnEvaluator contains runtime states and implements wrapper functions which "runtime state" Line 55: /// converts the input TupleRow into AnyVal format expected by UDAF functions defined stick to singular or plural apply to this class comment and all other comments. Line 70: const AggFn* agg_fn, AggFnEvaluator** evaluator) WARN_UNUSED_RESULT; why not make agg_fn a const&? Line 80: /// and caches constant input arguments in 'agg_fn_ctx'. what does agg_fn_ctx mean? is the caching behavior relevant for the caller? Line 88: /// Avoid the overhead of re-initializing an evaluator (e.g. calling GetConstVal() how big exactly is that overhead? Line 94: void Clone( do we really need this? Line 117: /// called either to drive the UDA's Update() or Merge() function depending on whether function, depending Line 136: void Finalize(Tuple* src, Tuple* dst); comment on the types of src and dst. also, those aren't great names, they sound like you're referring to Add() Line 159: void FreeLocalAllocations(); function comment missing Line 160: static void FreeLocalAllocations(std::vector<AggFnEvaluator*>& evaluators); why not a const&? Line 205: /// The interpreted path for the UDA's Update() function. It sets up the arguments to call long lines http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exprs/expr.h File be/src/exprs/expr.h: Line 34: /// Please see the headers of ScalarExpr and AggFn for details. > explain function context index concept here also, it would be good to have an explanation of the general class hierarchy somewhere. this description is also unclear on what differentiates the classes in this hierarchy. exprs and their subclasses contain compile-time information and the code to evaluate the expr (represented by the specific class). evaluators contain the general runtime state needed for the actual evaluation, but they don't need to be subclassed, because the expr-specific code sits in the expr subclasses. "embodies" doesn't really mean anything, that could also be applied to the analyzer (java) classes. http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exprs/scalar-expr-evaluator.h File be/src/exprs/scalar-expr-evaluator.h: Line 40: /// reference to the root of a ScalarExpr tree, runtime states (e.g. FunctionContexts) runtime state apply elsewhere Line 62: static Status Create(ObjectPool* pool, RuntimeState* state, MemPool* mem_pool, runtimestate already has an objectpool Line 63: const ScalarExpr* expr, ScalarExprEvaluator** evaluator) WARN_UNUSED_RESULT; why not a const&? Line 72: /// also the location in which constant arguments to functions are computed. Does not the location? do you mean "this also computes constant function arguments"? why here and not in expr::init()? it sounds like static setup. Line 73: /// need to be called on clones. Idempotent (this allows exprs to be opened multiple this clone business is tricky, would be nice to remove it. Line 111: Status GetConstValue( can't this be subsumed by GetValue(nullptr)? Line 112: RuntimeState* state, const ScalarExpr* expr, AnyVal** const_val) WARN_UNUSED_RESULT; why pass in expr? how is that different from the expr of Create()? Line 179: /// and owned by this ScalarExprEvaluator. shouldn't they go into an objectpool? Line 208: int output_scale_; comment why this is here and not in expr Line 210: /// Call Create() instead. reconsider whether this comment is useful http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exprs/scalar-expr.h File be/src/exprs/scalar-expr.h: Line 113: std::vector<ScalarExpr*>* exprs); could you also add warn_unused_result to all other .h files that you're touching? -- 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 <k...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
