Tim Armstrong 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?
Reworded to be hopefully clearer.

File be/src/benchmarks/bit-packing-benchmark.cc:

Line 273: #include "common/names.h"
> This isn't needed if using namespace std;, yes?
I think I prefer keeping this and removing the "using namespace" declarations 

File be/src/benchmarks/bswap-benchmark.cc:

Line 123
> Thanks for fixing this. When this is committed, can you file a bug against 
Sure, will do.

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 p
Added cstdlib. I think in practice it's reasonable to assume that cstdlib 
includes the same things as stdlib.h in some form.

The glibc one from the toolchain literally has #include <stdlib.h> in it.

Line 45:  private:

File be/src/util/bit-packing-test.cc:

Line 39: void UnpackSubset(const uint32_t* in, const uint8_t* packed, int 
> I think it would aid clarity to add a short description of what the meaning
Added descriptions and elaborated a little on what the function is doing.

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?
Didn't mean to leave it in - removed.

PS8, Line 84: 8
> Why 8 and not 4?
Used CHAR_BIT and added an intermediate variable to make it a bit clearer.

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;
> remove
Oops, sorry missed this.

To view, visit http://gerrit.cloudera.org:8080/4494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

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

Reply via email to