impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Ho (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4192: Disentangle Expr and ExprContext
Date Wed, 03 May 2017 20:52:25 GMT
Michael Ho has posted comments on this change.

Change subject: IMPALA-4192: Disentangle Expr and ExprContext
......................................................................


Patch Set 8:

(56 comments)

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?
In old-hash-table.cc.


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:   /// ProcessChildBatch().
> rename variables ending in _ctx if they're referring to now-obsolete class 
Done


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.
Done


Line 285:   boost::scoped_ptr<MemTracker> expr_mem_tracker_;
> is there a need to have one per exec node? how about one per fragment insta
There will be one per PlanNode in the future. Currently, it still counts towards the ExecNode's
MemTracker. TODO added.


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 state and implements wrapper functions which
convert
> "runtime state"
Done


Line 55: /// the input TupleRow into AnyVal format expected by UDAF functions defined in AggFn.
> stick to singular or plural
Done


Line 70:       const AggFn& agg_fn, AggFnEvaluator** evaluator) WARN_UNUSED_RESULT;
> why not make agg_fn a const&?
Done


Line 80:   /// and caches all constant input arguments.
> what does agg_fn_ctx mean?
The 'agg_fn_ctx' part of the comment is removed.


Line 88:   /// Avoid the overhead of re-initializing an evaluator (e.g. calling GetConstVal()
> how big exactly is that overhead?
Depends on how number and size of the input expressions.


Line 94:   void Clone(
> do we really need this?
It's used in PAGG::Partition logic for resource isolation.


Line 117:   /// called either to drive the UDA's Update() or Merge() function, depending on
whether
> function, depending
Done


Line 136:   /// merge phases.
> comment on the types of src and dst. also, those aren't great names, they s
Done


Line 159: 
> function comment missing
Done


Line 160:   /// Free local allocations made in UDA functions and input arguments' evaluators.
> why not a const&?
Done


Line 205:   inline const ColumnType& intermediate_type() const;
> long lines
Done


http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exprs/agg-fn.h
File be/src/exprs/agg-fn.h:

Line 21: 
> ..
Done


Line 28: class MemPool;
> the comment about "static states"/compile-time information applies to all e
Done


Line 97:   /// of the output value. Returns error status on failure.
> can intermediate and output slodesc be null?
No. Changed to const ref instead.


Line 101:       WARN_UNUSED_RESULT;
> who owns agg_fn?
It lives in state->obj_pool(). Comments added.


http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exprs/case-expr.h
File be/src/exprs/case-expr.h:

Line 38:  protected:
> this is needed?
Yes so we can call CaseExpr() constructor.


Line 41: 
> it would be nice to keep the fnctx index out of every class. is it possible
Done


http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exprs/expr.h
File be/src/exprs/expr.h:

Line 19: #ifndef IMPALA_EXPRS_EXPR_H
> move directly above class, not sure why the class comment was up here to st
Done


Line 34: using namespace impala_udf;
> explain function context index concept here
The latest patch avoids exposing function context index outside scalar-expr-* so the comments
will be kept in scalar-expr-* instead.


Line 34: using namespace impala_udf;
> also, it would be good to have an explanation of the general class hierarch
Done


Line 81:   /// status on failure.
> why does this not take an Expr**?
'fn_ctx_idx_ptr' removed. It doesn't take Expr** as 'root' is expected to be created by the
caller (constructors of ScalarExpr or AggFn).


Line 113:   /// some ScalarExpr such as ScalarFnCall. NULL if it's not used.
> initialize all pointers to nullptr here, that way you can't forget it in th
Done


Line 134:   /// Creates an expr tree with root 'parent' via depth-first traversal.
> doesn't look like a useful function (if you see it being called you probabl
Removed.


Line 137:   ///   pool: Object pool in which Expr created from nodes are stored
> "to create expr trees for subexpressions"
Done


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 state (e.g. FunctionContexts)
> runtime state
Done


Line 62:   /// FunctionContexts needed during evaluation. Allocations from this evaluator
will
> runtimestate already has an objectpool
There are places in which the evaluator will have shorter life span than the entire fragment
(e.g. expr evaluators in scanner threads or partitions in PAGG). So, it's easier to separate
them.


Line 63:   /// be from 'mem_pool'. The newly created evaluator will be stored in 'pool' and
> why not a const&?
Done


Line 72:       std::vector<ScalarExprEvaluator*>* evaluators) WARN_UNUSED_RESULT;
> the location?
Yes, ideally, that should be done in Expr::Init(). This is being tracked by IMPALA-4743. Will
add a TODO here.


Line 73: 
> this clone business is tricky, would be nice to remove it.
I believe it'd be easier to do it once IMPALA-4743 is fixed. Clone() is a performance optimization
for exec nodes with large expression. Plan to tackle it in a separate change to avoid potential
regression.


Line 111:   /// should only be called after Open() has been called on this expr. Returns an
error if
> can't this be subsumed by GetValue(nullptr)?
There is subtlety in this. "expr" is potentially a sub-expression of root_ (e.g. input expressions
to a ScalarFnCall). That's why it's not the same as GetValue(nullptr) as the latter will evaluate
the root expr.


Line 112:   /// there was an error evaluating the expression or if memory could not be allocated
for
> why pass in expr? how is that different from the expr of Create()?
Please see reply to previous comment. expr is a sub-expression.


Line 179:   friend class ScalarFnCall;
> shouldn't they go into an objectpool?
They do. Comments updated.


Line 208: 
> comment why this is here and not in expr
Done


Line 210:   /// -1 if no scale has been specified (currently the scale is only set for doubles
> reconsider whether this comment is useful
Removed.


http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exprs/scalar-expr.h
File be/src/exprs/scalar-expr.h:

Line 28: #include "common/status.h"
> stick to singular or plural in this and the following sentence.
Done


Line 40:   class Type;
> ..evaluator,
Done


Line 44: namespace impala {
> move comments about evaluator to evaluator class comment.
Done


Line 50: class RowDescriptor;
> ..builder, which inlines ...functions, or...
Done


Line 107: 
> why do we need this and Expr::CreateTree?
Expr::CreateTree() is the shared code between AggFn and ScalarExpr to unpack TExpr. This function
also calls the ScalarExpr's Init() function in addition to creating the Expr tree.


Line 113: 
> could you also add warn_unused_result to all other .h files that you're tou
Will try my best to cover functions (not .h files) I touched. This change is already large
enough to incorporate all these cleanups.


Line 223:   virtual DecimalVal GetDecimalVal(ScalarExprEvaluator*, const TupleRow*) const;
> what is the non-static state in the expr tree?
Done


Line 280:   /// 'fn_ctx_idx_' is the index into the FunctionContext vector in ScalarExprEvaluator
> would be good to have that in the Expr class comment, maybe with some more 
The new patch no longer exposes 'fn_ctx_idx' to expr.*. Will keep all these comments in 'scalar-expr*'
instead.


http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/exprs/scalar-fn-call.cc
File be/src/exprs/scalar-fn-call.cc:

PS7, Line 197: ) evaluator->output_scale_ = scale_arg->va
There is a subtle bug here. This ScalarFnCall isn't necessary evaluator->root_. It's incorrect
to set the output_scale_ in that case.


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
it's used in other subclasses of DataSink.


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.
Done


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?
Used by CreateTupleComparator() below.


Line 207:   vector<ScalarExpr*> ordering_exprs_;
> ?
Comments added.


http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/runtime/descriptors.h
File be/src/runtime/descriptors.h:

Line 288:   const std::vector<TExpr>& thrift_partition_key_exprs_;
> does this todo still apply, given that the descriptor tbl itself moves into
The person making the change to move the descriptor tbl into query state should remove it.


Line 291:   /// are Literals.
> why is there an evaluator in here, instead of the expr? this is shared by a
The evaluator is also shared by all fragment instance executors as it's just a literal.

Please see comments at HdfsPartitionDescriptor::partition_key_value_evaluators()


http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/runtime/sorter.h
File be/src/runtime/sorter.h:

Line 107:   /// Initializes the evaluators for materializing the tuples.
> ?
Done


Line 120:   /// Free any local allocations made when materializing and sorting the tuples.
> ?
Done


http://gerrit.cloudera.org:8080/#/c/5483/7/be/src/service/fe-support.cc
File be/src/service/fe-support.cc:

Line 76: // Serializes expression value 'value' to thrift structure TColumnValue 'col_val'.
> function comment missing
Done


-- 
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: 8
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