impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5844: use a MemPool for expr result allocations
Date Tue, 03 Oct 2017 21:52:52 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Michael Ho <kwho@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Tue, 03 Oct 2017 21:52:52 +0000
Gerrit-HasComments: Yes

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