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 12: Code-Review+2 (5 comments) http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/exec-node.h File be/src/exec/exec-node.h: http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/exec-node.h@379 PS11, Line 379: Status QueryMaintenance(RuntimeState* state) WARN_UNUSED_RESULT; > Yeah I think this is another step towards IMPALA-2399 since there's less en Yup. Mind dropping a TODO: IMPALA-2399 remove this. in here to remind people reading the code that this is not the way we want to do things in the future? Also, you could list IMPALA-2399 in the commit message since this is a very nice step toward that. http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/partitioned-aggregation-node.cc File be/src/exec/partitioned-aggregation-node.cc: http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/partitioned-aggregation-node.cc@600 PS11, Line 600: > Probably not Cool. Alternatively, if it were worth it, then maybe it'd be better for Clear() (or a wrapper) to do a quick first check e.g. like check total_allocated_bytes_ and bail if that's under some threshold. But if Clear() is cheap enough to just run through, let's do that. http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/scanner-context.h File be/src/exec/scanner-context.h: http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/scanner-context.h@337 PS11, Line 337: /// single-threaded scan node implementation. > Mentioned the two cases, since the only reason this is this complicated is Once we can get rid of the multi-threaded scanner case, this seems worth revisiting. Do you agree? If so, I think we should add a subtask about it to the overall MT jira. http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/runtime/sorter.h File be/src/runtime/sorter.h: http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/runtime/sorter.h@199 PS11, Line 199: /// MemPool for allocating data structures used by expression evaluators in the sorter. : MemPool expr_perm_pool_; : : /// MemPool for allocations that hold results of expression evaluation in the sorter. : /// Cleared periodically during sorting to prevent memory accumulating. : MemPool expr_results_pool_; : : /// In memory sorter and less-than comparator. : TupleRowComparator compare_less_than_; > In TopNNode and ExchangeNode the comparator can't be instantiated until Pre Was also asking about the MemPool (e.g. in ExecNode we have indirection - is that actually needed?). http://gerrit.cloudera.org:8080/#/c/8025/12/be/src/udf/udf.h File be/src/udf/udf.h: http://gerrit.cloudera.org:8080/#/c/8025/12/be/src/udf/udf.h@352 PS12, Line 352: Serialize() and Finalize()? or is it suppose to just say Serialize() is? -- 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: 12 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 04 Oct 2017 16:40:51 +0000 Gerrit-HasComments: Yes
