impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4192: Disentangle Expr and ExprContext
Date Mon, 22 May 2017 22:25:42 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4192: Disentangle Expr and ExprContext

Patch Set 14:


First batch of comments - made it through exec/kudu*
File be/src/exec/

Line 93:     RETURN_IF_ERROR(build_expr->Init(*(intermediate_row_desc_.get()), state));
build_expr is leaked if this returns.
File be/src/exec/

This all works out a lot cleaner - nice!

Line 124:     RETURN_IF_ERROR(AggFn::Create(analytic_node.analytic_functions[i],
I think there's a potential resource leak if the expr Init() function fails in certain ways.
E.g. if it successfully got a lib cache entry, but failed later. We don't add it to 'analytic_fns_'
so I don't think it can be closed.

Line 166:   fn_pool_.reset(new MemPool(expr_mem_tracker()));;
extra semicolon
File be/src/exec/

Line 181:   // TODO: codegen table sink
I'm not sure what this means. Is it something to be fixed in this patch? Otherwise could be
just remove the TODO and file a JIRA if it's something that needs to be tracked.
File be/src/exec/data-sink.h:

Line 126:   /// Not used in DataStreamSender and PHJBuilder.
Or NljBuilder. Maybe easier to just say that they're not used in all subclasses.
File be/src/exec/

PS14, Line 451: inline
Is the inline needed here?

Line 465: void ExecNode::FreeLocalAllocations() {
This helper function doesn't seem to help that much - consider inlining it.
File be/src/exec/exec-node.h:

Line 261:   /// Conjuncts and their evaluators in this node.
Maybe mention who owns the pointers?
File be/src/exec/

PS14, Line 86: expr_evaluator->root()
Is the plan to eventually pass around the Expr itself and avoid going via the evaluator, or
will we continue using this pattern? I don't feel that strongly either way, just curious.
File be/src/exec/filter-context.h:

Line 120:   FilterContext() { }
nit: can just remove this and rely on the default constructor
File be/src/exec/hash-table.h:

Line 115:   /// computing the size of a scratch row.
Maybe mention what 'pool' is for

Line 415:   Status Init(ObjectPool* pool, RuntimeState* state, int num_build_tuples);
What is pool used for?

Line 513:   MemPool* mem_pool_;
Mention that it's not owned?
File be/src/exec/

PS14, Line 294: 	  

Line 733:     vector<ScalarExprEvaluator*>& output_expr_evaluators)
Can't this be a const& still? The vector isn't being modified is it?
File be/src/exec/

Line 26: #include "exprs/scalar-expr.h"
Maybe not needed?
File be/src/exec/

Line 147:     // TODO: Move this to Prepare()
Is this TODO in this patch, or does it depend on another change?

Line 165:         new RowDescriptor(min_max_tuple_desc_, /* is_nullable */ false));
This is a little inconsistent - above on line 121 we create the RowDescriptor on the stack
but here it's heap-allocated. I think the stack allocation pattern is probably simpler.

PS14, Line 186: conjunct_evaluators_map_[entry.first]
[] has a side-effect of creating the element - maybe use find() instead for the DCHECK.
File be/src/exec/hdfs-scan-node-base.h:

Line 146:   const std::vector<ScalarExprEvaluator*> min_max_conjunct_evaluators() const
Shouldn't this return a const&, rather than copying the vector?

Line 393:   /// the per-scanner ScannerContext..
Add something like "These correspond to the exprs in 'filter_exprs_'"
File be/src/exec/

Line 362:   MemPool filter_mem_pool(expr_mem_tracker());
This is a little subtle - could do with a comment.

Line 388:             for (auto& ctx: filter_ctxs) {
Maybe use a goto and an exit block at the bottom of the function to enforce that the cleanup
happens on both paths.
File be/src/exec/kudu-scanner.h:

Line 95:   boost::scoped_ptr<MemPool> expr_mem_pool_;
Put it in 'obj_pool_'?
File be/src/exprs/expr-context.h:

This can be deleted right?

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Iefdc9aeeba033355cb9497e3a5d2363627dcf2f3
Gerrit-PatchSet: 14
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