Matthew Jacobs 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 PS12, Line 988: capacity_(0), the logic around handling 0 seems more complicated than necessary. can you just use INIT_CAPACITY and do the allocation in the constructor? PS12, Line 1061: // Should be called before use after Serialize(). : void Deserialize() { comment is confusing, seems contradictory? I think we can remove this 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 before returning a copy in Serialize? 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_ ? also can you put this right above samples_ to keep parameters grouped together. Line 1150: // Array of ReservoirSamples. comment that this may be inline or a pointer to a separately allocated array PS12, Line 1153: < nit: we don't wrap args with braces in comments -- 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 <tbobrovyt...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Marcel Kornacker <mar...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Mostafa Mokhtar <mmokh...@cloudera.com> Gerrit-Reviewer: Taras Bobrovytsky <tbobrovyt...@cloudera.com> Gerrit-HasComments: Yes
