impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
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:

(26 comments)

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

http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/aggregation-node.cc
File be/src/exec/aggregation-node.cc:

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


http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/analytic-eval-node.cc
File be/src/exec/analytic-eval-node.cc:

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


http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/data-sink.cc
File be/src/exec/data-sink.cc:

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.


http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/data-sink.h
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.


http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/exec-node.cc
File be/src/exec/exec-node.cc:

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.


http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/exec-node.h
File be/src/exec/exec-node.h:

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


http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/filter-context.cc
File be/src/exec/filter-context.cc:

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.


http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/filter-context.h
File be/src/exec/filter-context.h:

Line 120:   FilterContext() { }
nit: can just remove this and rely on the default constructor


http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/hash-table.h
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?


http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/hdfs-parquet-table-writer.cc
File be/src/exec/hdfs-parquet-table-writer.cc:

PS14, Line 294: 	  
tab?


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


http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/hdfs-rcfile-scanner.cc
File be/src/exec/hdfs-rcfile-scanner.cc:

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


http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/hdfs-scan-node-base.cc
File be/src/exec/hdfs-scan-node-base.cc:

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.


http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/hdfs-scan-node-base.h
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_'"


http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:

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.


http://gerrit.cloudera.org:8080/#/c/5483/14/be/src/exec/kudu-scanner.h
File be/src/exec/kudu-scanner.h:

Line 95:   boost::scoped_ptr<MemPool> expr_mem_pool_;
Put it in 'obj_pool_'?


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

This can be deleted right?


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