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-5844: use a MemPool for expr result allocations
Date Mon, 25 Sep 2017 23:52:39 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Mon, 25 Sep 2017 23:52:39 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message