impala-reviews mailing list archives

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

File be/src/exprs/

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

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I99adaad574d4fb0a3cf38c6cbad8b2a23df12968
Gerrit-PatchSet: 8
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