impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4787: Optimize APPX MEDIAN() memory usage
Date Thu, 23 Feb 2017 02:41:55 GMT
Marcel Kornacker has posted comments on this change.

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


Patch Set 8:

(9 comments)

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

Line 126: // TODO: this file should be cross compiled and then all of the builtin
is this done?


Line 961: struct ReservoirSampleState {
i'd say this really turned into a class


Line 967:   // resize occurs, this needs to be updated from the outside.
what does 'from the outside' mean?


Line 977:   ReservoirSampleState(int init_capacity) :
use standard formatting


Line 1016:     // The array of ReservoirSamples starts right after ReservoirSampleState, so
we use
that's often done by putting an array of size 1 at the end of the header struct:

ReservoirSample<T> samples[1];

and then you can do things like state.samples[5] = x;

it's convenient and makes it explicit that you have a trailing var-len array.


Line 1025:   int64_t GetNext64(int64_t max) {
while you're at it, this deserves a comment


Line 1033:   // Given a buffer that contains a ReservoirSampleState, resize the buffer so
that it's
its


Line 1040:   if (new_capacity * 2 >= MAX_CAPACITY) new_capacity = MAX_CAPACITY;
if state->capacity is 10 and max_capacity is 40, this line sets new_capacity to 40.


Line 1062:   // If the array gets filled due to updates or merges, we reallocate a larger
buffer to
you should put this (= a brief description of what you're doing) somewhere at the beginning
of the reservoir sample-related code.


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