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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
