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 Tue, 09 May 2017 00:22:00 GMT
Tim Armstrong has posted comments on this change.

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


Patch Set 9:

(10 comments)

Still trying to come to grips with it all but did an initial pass over the query execution
changes.

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

Line 122:   /// Output expressions to convert row batches onto output values.
It's confusing that these aren't used in all subclasses of DataSink (e.g. DataStreamSender,
JoinBuildSink). Clarify that not all sinks use these?


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

PS9, Line 69: expr_mem_pool
Does this mean that multiple scanner threads share a single MemPool? That doesn't seem safe.


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

PS9, Line 68: materialize_tuple_
materialize_tuple_ doesn't seem to be defined. I think we always need these exprs anyway,
right?


Line 69:   std::vector<ScalarExpr*> slot_materialize_exprs_;
See my comment about the name in sorter.h.


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

Line 65:   /// tree rooted at 'node'.
* and do any initialization that is required as a result of setting the subplan.


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

PS9, Line 78: slot_materialize_exprs_
output_tuple_exprs_? Usually when we have exprs materializing tuples it's named after the
tuple it's materializing.


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

PS9, Line 94: Clone
Maybe this should be called ShallowClone() to make it more explicit. Without reading the function
comment, I would assume it did a deep clone.


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

PS9, Line 182: slot_materialize_exprs_
Maybe something like 'sort_tuple_exprs_'? I think the important thing is that there is one
expr per slot in the sort tuple.


http://gerrit.cloudera.org:8080/#/c/5483/9/be/src/util/tuple-row-compare.cc
File be/src/util/tuple-row-compare.cc:

Line 49:   ScalarExprEvaluator::Close(key_expr_evaluators_lhs_, state);
opened_ = false?


http://gerrit.cloudera.org:8080/#/c/5483/9/be/src/util/tuple-row-compare.h
File be/src/util/tuple-row-compare.h:

PS9, Line 141: key_expr_evaluators_lhs_
ordering_expr_evaluators_lhs_ to match the expr name?


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