Jim Apple has posted comments on this change. Change subject: IMPALA-4123: Fast bit unpacking ......................................................................
Patch Set 8: (14 comments) http://gerrit.cloudera.org:8080/#/c/4494/8//COMMIT_MSG 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? http://gerrit.cloudera.org:8080/#/c/4494/8/be/src/benchmarks/bit-packing-benchmark.cc File be/src/benchmarks/bit-packing-benchmark.cc: Line 273: #include "common/names.h" This isn't needed if using namespace std;, yes? http://gerrit.cloudera.org:8080/#/c/4494/8/be/src/benchmarks/bswap-benchmark.cc File be/src/benchmarks/bswap-benchmark.cc: Line 123 Thanks for fixing this. When this is committed, can you file a bug against me to fix the other posix_memalign locations? http://gerrit.cloudera.org:8080/#/c/4494/8/be/src/testutil/mem-util.h 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: DISALLOW_COPY_AND_ASSIGN http://gerrit.cloudera.org:8080/#/c/4494/7/be/src/util/bit-packing-test.cc File be/src/util/bit-packing-test.cc: 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' http://gerrit.cloudera.org:8080/#/c/4494/8/be/src/util/bit-packing-test.cc File be/src/util/bit-packing-test.cc: 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 'packed' 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()? http://gerrit.cloudera.org:8080/#/c/4494/7/be/src/util/bit-packing.inline.h 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? http://gerrit.cloudera.org:8080/#/c/4494/8/be/src/util/bit-packing.inline.h 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 -- 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 <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
