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 Thu, 20 Apr 2017 00:31:29 GMT
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 <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