impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Taras Bobrovytsky (Code Review)" <>
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:

File be/src/exprs/

Line 985: 
> nit extra newline

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

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

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

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

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I99adaad574d4fb0a3cf38c6cbad8b2a23df12968
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Mostafa Mokhtar <>
Gerrit-Reviewer: Taras Bobrovytsky <>
Gerrit-HasComments: Yes

View raw message