Dan Hecht 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 9: (11 comments) http://gerrit.cloudera.org:8080/#/c/8025/9/be/src/exprs/agg-fn-evaluator.h File be/src/exprs/agg-fn-evaluator.h: http://gerrit.cloudera.org:8080/#/c/8025/9/be/src/exprs/agg-fn-evaluator.h@65 PS9, Line 65: Evaluator-managed let's use same terminology as in scalar-expr-evaluator.h (permanent) http://gerrit.cloudera.org:8080/#/c/8025/9/be/src/exprs/agg-fn-evaluator.h@93 PS9, Line 93: all input values what do we mean by that? http://gerrit.cloudera.org:8080/#/c/8025/9/be/src/exprs/agg-fn-evaluator.h@96 PS9, Line 96: /// So, it's not safe to use cloned evaluators concurrently. does expr_*_pool need to be unique, or can they be shared with another evaluator? http://gerrit.cloudera.org:8080/#/c/8025/9/be/src/exprs/agg-fn-evaluator.h@117 PS9, Line 117: expr-managed permanent? or i think we should somewhere explain what expr-managed really means and that it comes from the permanent pool. http://gerrit.cloudera.org:8080/#/c/8025/9/be/src/exprs/agg-fn-evaluator.h@142 PS9, Line 142: 'results_pool' that's not actually a variable, is it? maybe say "results mem pool"? http://gerrit.cloudera.org:8080/#/c/8025/9/be/src/exprs/agg-fn-evaluator.h@175 PS9, Line 175: /// This should generally be used via ScopedResultsPool instead of directly. I'm not sure I follow why we need this as opposed to always just requiring that the right mem pool is provided in the first place (maybe GetValue() and friends should just take the mem pool they should use?) Perhaps that's too intrusive to do now. But I guess I'm wondering is are there cases where we won't want to use the specific result pool and fallback to the original "generic" result pool? http://gerrit.cloudera.org:8080/#/c/8025/9/be/src/exprs/scalar-expr-evaluator.h File be/src/exprs/scalar-expr-evaluator.h: http://gerrit.cloudera.org:8080/#/c/8025/9/be/src/exprs/scalar-expr-evaluator.h@217 PS9, Line 217: fn_ctxs_' missing opening ' http://gerrit.cloudera.org:8080/#/c/8025/9/be/src/exprs/scalar-expr-evaluator.h@219 PS9, Line 219: Allocations made from this pool must live until after the evaluator is closed. this is great information (and similar in expr_results_pool_), but since agg-fn-evaluator doesn't have these fields itself, we don't have these comments there. Maybe some part of these comments are better on the Create() methods since the caller of Create() has to abide by this rule? http://gerrit.cloudera.org:8080/#/c/8025/9/be/src/exprs/scalar-expr-evaluator.h@225 PS9, Line 225: except when the evaluator is in the : /// middle of evaluating the expression. that's kind of already implied since these things being used by a single thread, right? i'm fine with that statement, just want to make sure i'm interpreting it the right way. http://gerrit.cloudera.org:8080/#/c/8025/9/be/src/exprs/scalar-expr-evaluator.h@227 PS9, Line 227: MemPool* const expr_results_pool_; what's the distinction between these pools and the pair that live in the FunctionContextImpl? why do scalar evaluators have these here, but the agg fn evaluator relies on the one in the function context? http://gerrit.cloudera.org:8080/#/c/8025/9/be/src/udf/udf-internal.h File be/src/udf/udf-internal.h: http://gerrit.cloudera.org:8080/#/c/8025/9/be/src/udf/udf-internal.h@101 PS9, Line 101: AllocateResults AllocateForResults -- 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: 9 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 28 Sep 2017 22:23:23 +0000 Gerrit-HasComments: Yes
