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