Jim Apple has posted comments on this change. Change subject: IMPALA-4123: Fast bit unpacking ......................................................................
Patch Set 11: (5 comments) http://gerrit.cloudera.org:8080/#/c/4494/11/be/src/util/bit-packing-test.cc File be/src/util/bit-packing-test.cc: Line 19: #include <stdlib.h> cstd... Line 132: std::generate(std::begin(in), std::end(in), rand); rand() is only guaranteed to have 15 non-zero bits - the high 17 bits may all be 0 each time. More modern rand, and also seeded to prevent flaky tests: std::mt19937 pseudorandom(0); std::uniform_int_distribution<int32_t> uniform; std::generate(std::begin(in), std::end(in), [&pseudorandom, &uniform]() { return uniform(pseudorandom); }); http://gerrit.cloudera.org:8080/#/c/4494/11/be/src/util/bit-packing.h File be/src/util/bit-packing.h: PS11, Line 74: /// Templated versions of UnpackValues() that can be used if BIT_WIDTH is a : /// compile-time constant. Did you check to see that this benchmarks to be actually faster than if BIT_WIDTH is not a template parameter? http://gerrit.cloudera.org:8080/#/c/4494/11/be/src/util/bit-stream-utils.h File be/src/util/bit-stream-utils.h: Line 100: /// 'buffer' is the buffer to read from. The buffer's length is 'buffer_len'. While you're here, can you add "Does not take ownership of the pointer"? Line 142: const uint8_t* buffer_; While you're here, can you DISALLOW_COPY_AND_ASSIGN? -- 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: 11 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
