impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5844: use a MemPool for expr result allocations
Date Wed, 04 Oct 2017 19:24:50 GMT
Tim Armstrong 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:

(6 comments)

Rebase, then addressed 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;
> Yup. Mind dropping a TODO: IMPALA-2399 remove this. 
Done


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: 
> Cool. Alternatively, if it were worth it, then maybe it'd be better for Cle
Clear() looks fairly cheap (it just iterates over the vector<ChunkInfo>) and I don't
think this class is super perf critical - it's only called to clean up partitions that weren't
fully processed.


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.
> Once we can get rid of the multi-threaded scanner case, this seems worth re
IMPALA-6015


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_;
> Was also asking about the MemPool (e.g. in ExecNode we have indirection - i
I think the reasoning there is similar - the MemTracker* is a constructor parameter and the
MemTrackers are not set up until Prepare(). Another advantage is that we can forward-declare
MemPool instead of pulling in the full header.


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?
Done


http://gerrit.cloudera.org:8080/#/c/8025/12/be/src/udf/udf.h@356
PS12, Line 356: tringVal(FunctionContext*, int) ctor or StringVal::CopyFrom(FunctionContext*,
              : /// const uint8_t*, size_t).
StringVal::Resize() also should be listed 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: 12
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: Wed, 04 Oct 2017 19:24:50 +0000
Gerrit-HasComments: Yes

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