Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8025 )
Change subject: IMPALA-5844: use a MemPool for expr result allocations ...................................................................... Patch Set 7: (5 comments) I think I addressed all the comments - I was in the process of renaming and updating comments so some of the comments commented on changed substantially. http://gerrit.cloudera.org:8080/#/c/8025/6/be/src/exec/exec-node.h File be/src/exec/exec-node.h: http://gerrit.cloudera.org:8080/#/c/8025/6/be/src/exec/exec-node.h@324 PS6, Line 324: expr-managed > Okay, keeping this specific to exprs, at least for now is fine, so that we Done http://gerrit.cloudera.org:8080/#/c/8025/6/be/src/exec/exec-node.h@333 PS6, Line 333: expr_tmp_mem_pool_ > let's go with expr_results_mem_pool_. And then I think we should explain it Done. I started doing the rename and ended up realising that we could go the whole way and remove the "Local" terminology entirely because it is not exposed in udf.h http://gerrit.cloudera.org:8080/#/c/8025/7/be/src/exprs/agg-fn-evaluator.h File be/src/exprs/agg-fn-evaluator.h: 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 thi I documented each of the functions in this header and tweaked the description in udf.h, which was vague. http://gerrit.cloudera.org:8080/#/c/8025/7/be/src/exprs/agg-fn-evaluator.h@220 PS7, Line 220: Tuple* dst > which mem pool? Done 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 Done -- 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 23:52:39 +0000 Gerrit-HasComments: Yes
