Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/13737 )
Change subject: IMPALA-8710: Increase allowed bit width to 64 for bit packing ...................................................................... Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/13737/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13737/2//COMMIT_MSG@14 PS2, Line 14: > Performance effects could be mentioned + Done http://gerrit.cloudera.org:8080/#/c/13737/2//COMMIT_MSG@16 PS2, Line 16: > nit: wrap at 72 Done http://gerrit.cloudera.org:8080/#/c/13737/2/be/src/util/bit-packing.inline.h File be/src/util/bit-packing.inline.h: http://gerrit.cloudera.org:8080/#/c/13737/2/be/src/util/bit-packing.inline.h@177 PS2, Line 177: const uint32_t* const in = re > I would move this to the very beginning to make it clear that the rest of t Done http://gerrit.cloudera.org:8080/#/c/13737/2/be/src/util/bit-packing.inline.h@181 PS2, Line 181: p > nit: extra space Done http://gerrit.cloudera.org:8080/#/c/13737/2/be/src/util/bit-packing.inline.h@184 PS2, Line 184: == 1 > Can you move this to a constexpr to make it clear that this happens at comp Done http://gerrit.cloudera.org:8080/#/c/13737/2/be/src/util/bit-stream-utils-test.cc File be/src/util/bit-stream-utils-test.cc: http://gerrit.cloudera.org:8080/#/c/13737/2/be/src/util/bit-stream-utils-test.cc@294 PS2, Line 294: TestZigZagEncode<int64_t>(1629267541664, {0xc0, 0xfa, 0xcd, 0xfe, 0xea, 0x5e}); > nit:extra line Done http://gerrit.cloudera.org:8080/#/c/13737/2/be/src/util/bit-stream-utils-test.cc@315 PS2, Line 315: TestZigZagDecode<int64_t>(-9223372036854775808U, // Most negative int64_t. > nit:extra line Done http://gerrit.cloudera.org:8080/#/c/13737/2/be/src/util/bit-stream-utils-test.cc@320 PS2, Line 320: TEST(VLQInt, TestMaxVlqByteLen) { > nit:extra line Done http://gerrit.cloudera.org:8080/#/c/13737/2/be/src/util/bit-stream-utils.inline.h File be/src/util/bit-stream-utils.inline.h: http://gerrit.cloudera.org:8080/#/c/13737/2/be/src/util/bit-stream-utils.inline.h@44 PS2, Line 44: un > Can you expand the abbreviation? It took me a few seconds to understand it. Done -- To view, visit http://gerrit.cloudera.org:8080/13737 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3ff3b387cbe8e41dd78c454777711ef54de4afa8 Gerrit-Change-Number: 13737 Gerrit-PatchSet: 3 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Fri, 28 Jun 2019 12:11:36 +0000 Gerrit-HasComments: Yes
