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 <[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
