impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Apple (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4123: Fast bit unpacking
Date Tue, 11 Oct 2016 21:28:49 GMT
Jim Apple has posted comments on this change.

Change subject: IMPALA-4123: Fast bit unpacking

Patch Set 8:

Commit Message:

PS8, Line 17: 64
32, now?

PS8, Line 18: ors at 64-bit boundaries
What does it mean to have an or at a k-bit boundary?
File be/src/benchmarks/

Line 273: #include "common/names.h"
This isn't needed if using namespace std;, yes?
File be/src/benchmarks/

Line 123
Thanks for fixing this. When this is committed, can you file a bug against me to fix the other
posix_memalign locations?
File be/src/testutil/mem-util.h:

PS8, Line 31: posix_memalign
#include stdlib.h. I'd say cstdlib, but posix_memalign doesn't seem to be part of the C standard,
so I don't know if it is technically in that header.

Line 45:  private:
File be/src/util/

PS7, Line 42: values' va
> In practice it should be 64-bit aligned with TCMalloc beyond a certain allo
I think it would help the reader here to spell out that misaligned is with respect to 64.

PS7, Line 132: 
> Other ones should work but I don't think improve coverage - can't see a sce
Still, if we're only allowing those two you could switch it to a bool 'misaligned'
File be/src/util/

Line 39: void UnpackSubset(const uint32_t* in, const uint8_t* packed, int num_in_values,
I think it would aid clarity to add a short description of what the meanings are of 'in' and

PS8, Line 45: uint32_t
const uint32_t * in

PS8, Line 72: INFO
Was this just for debugging, or did you mean to leave it in?

PS8, Line 84: 8
Why 8 and not 4?

Can you add a more exact ASSERT here about num_to_unpack and it's relationship with writer.bytes_written()?
File be/src/util/bit-packing.inline.h:

PS7, Line 69:   // First unpack as many full batches as possible.
> Yep, that was a bug.
Should there be an additional test that would demonstrate that bug?
File be/src/util/bit-packing.inline.h:

PS8, Line 64: //LOG(INFO) << "bit_width " << BIT_WIDTH << "\n"
            :   //  << "in_bytes " << in_bytes << " num_values " <<
num_values << " max_input_values " << max_input_values << "\n"
            :    // << "values_to_read " << values_to_read << " batches_to_read
" << batches_to_read << "\n"
            :    // << "remainder_values " << remainder_values;

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <>
Gerrit-Reviewer: Jim Apple <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-HasComments: Yes

View raw message