Tim Armstrong has posted comments on this change. Change subject: IMPALA-4123: Fast bit unpacking ......................................................................
Patch Set 7: (3 comments) Sorry about that - I wish there was a good way to see whether I've responded to all of the comments on different patchset verisons. 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: misaligned > I think it would help the reader here to spell out that misaligned is with Done PS7, Line 132: min(length, 1) > Still, if we're only allowing those two you could switch it to a bool 'misa Made the change, then switched to 'aligned' to avoid having to think about a double negative. 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: in_bytes -= batch_size * (BIT_WIDTH / CHAR_BIT); > Should there be an additional test that would demonstrate that bug? Running the test under ASAN caught it once I fixed the bug in the test code. The bug was exactly the kind of bug I was trying to catch with the test so we do have the coverage. It seems weird (although not entirely without merit) to add a regression test for a test bug. -- 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: 7 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
