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 20:26:10 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:

(12 comments)

This is looking very good. Todo for me:
partitioned-aggregation-node.cc
sorter.{h,cc}

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;
I still think we should get rid of this but okay to leave in this change.


http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/hash-table.h
File be/src/exec/hash-table.h:

http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/hash-table.h@396
PS11, Line 396: Cloes
Close


http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/hash-table.h@525
PS11, Line 525:   /// results of expr evaluation.
Not owned? Which also means the exec node will do the maintenance on them, right?


http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/partitioned-aggregation-node.h
File be/src/exec/partitioned-aggregation-node.h:

http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/partitioned-aggregation-node.h@214
PS11, Line 214: if this is a non-grouping aggregation used
              :   /// for the non-grouping cases
did that get garbled?


http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/partitioned-aggregation-node.h@219
PS11, Line 219:   /// are allocated from 'expr_perm_pool_' and 'expr_results_pool_' respectively.
and what about for grouping case?


http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/partitioned-aggregation-node.h@419
PS11, Line 419: expr_managed
should this be talking about permanent?


http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/partitioned-aggregation-node.h@420
PS11, Line 420: per partition
I don't get that comment, given this is a single pool.


http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/exec/partitioned-aggregation-node.h@421
PS11, Line 421: Local allocations
results


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:   /// evaluation to prevent memory from accumulating.
who owns this?
and why does this make sense in ScannerContext? (reading the class comment it's not clear
why a lot of this stuff is here).


http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/udf/udf.h
File be/src/udf/udf.h:

http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/udf/udf.h@350
PS11, Line 350: For allocations that back the intermediate results of aggregation
              : /// returned by Init(), Update() and Merge() should be allocated by
a bit garbled


http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/udf/udf.h@616
PS11, Line 616: temporary
this is kind of new terminology. above i think you said "temporary results". maybe that, or
just "results"?


http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/util/tuple-row-compare.h
File be/src/util/tuple-row-compare.h:

http://gerrit.cloudera.org:8080/#/c/8025/11/be/src/util/tuple-row-compare.h@77
PS11, Line 77: Any
             :   /// the evaluators with use
garbled



-- 
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 20:26:10 +0000
Gerrit-HasComments: Yes

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