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 11: (4 comments) Remaining comments. 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: if (++i % 1024 == 0) is that even worth it? 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@97 PS11, Line 97: (returns true if lhs < rhs) not sure that still makes sense. what returns true? http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/runtime/sorter.h@202 PS11, Line 202: intermediate not just the intermediate results, but also the final results of expression evaluation, no? 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 intermediate 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 other places, we have indirection (i.e. scoped_ptr) for these fields. is there a reason we need that indirection in those places but not here? -- 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: 11 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: Tue, 03 Oct 2017 21:52:52 +0000 Gerrit-HasComments: Yes
