Dan Hecht has posted comments on this change. ( http://gerrit.cloudera.org:8080/8025 )
Change subject: IMPALA-5844: use a MemPool for expr local allocations ...................................................................... Patch Set 7: (5 comments) http://gerrit.cloudera.org:8080/#/c/8025/7/be/src/exprs/agg-fn-evaluator.h File be/src/exprs/agg-fn-evaluator.h: PS7: Generally, I think it's hard to understand the two different pools when it comes to aggregates since we don't document which pool different allocations come from. Maybe there is a simple high level rule that can be described? http://gerrit.cloudera.org:8080/#/c/8025/7/be/src/exprs/agg-fn-evaluator.h@156 PS7, Line 156: static void Serialize(const std::vector<AggFnEvaluator*>& evals, Tuple* dst); : static void GetValue(const std::vector<AggFnEvaluator*>& evals, Tuple* src, Tuple* dst); : static void Finalize(const std::vector<AggFnEvaluator*>& evals, Tuple* src, Tuple* dst); which mem pool can dst varlen data live in? i think we need to document this all more clearly to make sense of it. http://gerrit.cloudera.org:8080/#/c/8025/7/be/src/exprs/agg-fn-evaluator.h@162 PS7, Line 162: Serialize(), Finalize() and GetValue() I guess that implies the answer to my previous question, but I think those should be documented explicitly. http://gerrit.cloudera.org:8080/#/c/8025/7/be/src/exprs/agg-fn-evaluator.h@220 PS7, Line 220: Tuple* dst which mem pool? http://gerrit.cloudera.org:8080/#/c/8025/7/be/src/exprs/agg-fn-evaluator.h@225 PS7, Line 225: Note that StringVal result is : /// from local allocation (which will be freed in the next QueryMaintenance()) so it : /// needs to be copied out if it needs to survive beyond QueryMaintenance() (e.g. if : /// 'dst' lives in a row batch). maybe this should be reworded now that you're introducing a mechanism that allows the "local allocation" to be against the right mem-pool. -- To view, visit http://gerrit.cloudera.org:8080/8025 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4ba5a7542ed90a49a4b5586c040b5985a7d45b61 Gerrit-Change-Number: 8025 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Mon, 25 Sep 2017 22:33:10 +0000 Gerrit-HasComments: Yes
