Tim Armstrong has posted comments on this change. Change subject: IMPALA-4123: Fast bit unpacking ......................................................................
Patch Set 8: (11 comments) http://gerrit.cloudera.org:8080/#/c/4494/8//COMMIT_MSG Commit Message: PS8, Line 17: 64 > 32, now? Done 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. 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? I think I prefer keeping this and removing the "using namespace" declarations below. 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 Sure, will do. 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 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: > DISALLOW_COPY_AND_ASSIGN Done 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 meaning Added descriptions and elaborated a little on what the function is doing. PS8, Line 45: uint32_t > const uint32_t * in Done 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. 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 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 <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
