impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taras Bobrovytsky (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4787: Optimize APPX MEDIAN() memory usage
Date Sat, 04 Mar 2017 03:37:15 GMT
Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-4787: Optimize APPX_MEDIAN() memory usage
......................................................................


Patch Set 12:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/6025/12/be/src/exprs/aggregate-functions-ir.cc
File be/src/exprs/aggregate-functions-ir.cc:

Line 985: 
> nit extra newline
Done


PS12, Line 988: capacity_(0),
> the logic around handling 0 seems more complicated than necessary. can you 
Done


PS12, Line 1061:   // Should be called before use after Serialize().
               :   void Deserialize() {
> comment is confusing, seems contradictory? I think we can remove this
Updated the comment. I don't think we can remove this, explained in the comment below.


PS12, Line 1063:     samples_ = reinterpret_cast<ReservoirSample<T>*>(this + 1);
               :     capacity_ = num_samples_;
               :     sample_array_separate_ = false;
> I don't see why this is necessary - can't you set the properties you need b
Serialize gets called, then the resulting buffer gets transferred to a different impalad in
the cluster. The pointer no longer makes sense on the new machine because the location in
memory is different. (Am I thinking about this correctly?)


PS12, Line 1138: 
               :   // True if the array of samples is in a separate memory allocation. This
object is
               :   // responsible for freeing it if true.
               :   bool sample_array_separate_;
> how about calling this samples_inline_ ?
Done


Line 1150:   // Array of ReservoirSamples.
> comment that this may be inline or a pointer to a separately allocated arra
Done


PS12, Line 1153: <
> nit: we don't wrap args with braces in comments
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/6025
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I99adaad574d4fb0a3cf38c6cbad8b2a23df12968
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tbobrovytsky@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message