Zoltan Borok-Nagy 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 5: Code-Review+1 (4 comments) http://gerrit.cloudera.org:8080/#/c/13737/5/be/src/util/bit-packing.inline.h File be/src/util/bit-packing.inline.h: http://gerrit.cloudera.org:8080/#/c/13737/5/be/src/util/bit-packing.inline.h@172 PS5, Line 172: constexpr int NO_WORDS = LAST_WORD_IDX - FIRST_WORD_IDX; nit: You don't use abbreviations at other places, so at first it reads like "no words". Maybe WORDS_TO_READ? http://gerrit.cloudera.org:8080/#/c/13737/5/be/src/util/bit-packing.inline.h@179 PS5, Line 179: constexpr bool CAN_READ_64 = FIRST_BIT_IDX - FIRST_BIT_OFFSET + 64 <= BIT_WIDTH * 32; nit: This is about not to read past the end of the buffer, right? Can you add comment for it? http://gerrit.cloudera.org:8080/#/c/13737/5/be/src/util/bit-stream-utils-test.cc File be/src/util/bit-stream-utils-test.cc: http://gerrit.cloudera.org:8080/#/c/13737/5/be/src/util/bit-stream-utils-test.cc@158 PS5, Line 158: int64_t nit: uint64_t? http://gerrit.cloudera.org:8080/#/c/13737/5/be/src/util/bit-stream-utils-test.cc@158 PS5, Line 158: 0LL very nit: sometimes in the code there is only L or UL, and sometimes LL, I think UL is the choice for most places. It doesn't really matter, but makes the reader to think for a sec why this specific suffix is used here. -- 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: 5 Gerrit-Owner: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Zoltan Borok-Nagy <borokna...@cloudera.com> Gerrit-Comment-Date: Tue, 02 Jul 2019 15:48:48 +0000 Gerrit-HasComments: Yes
