impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <>
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:


initial comments
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?
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?
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?)
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"
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.


Line 40: /// in FunctionContext in ScalarExprEvaluator which is the interface to evaluate

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.
File be/src/runtime/

Line 379:     const vector<TExpr>& thrift_output_exprs) {
unused parameter
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.
File be/src/runtime/

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

Line 207:   vector<ScalarExpr*> ordering_exprs_;
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.
File be/src/runtime/sorter.h:

Line 107:   ///

Line 120:   /// XXX
File be/src/service/

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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Iefdc9aeeba033355cb9497e3a5d2363627dcf2f3
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <>
Gerrit-Reviewer: Dan Hecht <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Michael Ho <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message