Csaba Ringhofer 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 2: Code-Review+1 (10 comments) I found only nits. 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 + https://gerrit.cloudera.org/#/c/12621/ as it contains the benchmarks that were used to verify that there is no performance regression. http://gerrit.cloudera.org:8080/#/c/13737/2//COMMIT_MSG@16 PS2, Line 16: 64 bits. nit: wrap at 72 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: if (BIT_WIDTH == 0) return 0; I would move this to the very beginning to make it clear that the rest of the logic does not have to handle this case. http://gerrit.cloudera.org:8080/#/c/13737/2/be/src/util/bit-packing.inline.h@181 PS2, Line 181: nit: extra space http://gerrit.cloudera.org:8080/#/c/13737/2/be/src/util/bit-packing.inline.h@184 PS2, Line 184: BitUtil::IsPowerOf2(BIT_WIDTH) Can you move this to a constexpr to make it clear that this happens at compile time? 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: nit:extra line http://gerrit.cloudera.org:8080/#/c/13737/2/be/src/util/bit-stream-utils-test.cc@315 PS2, Line 315: nit:extra line http://gerrit.cloudera.org:8080/#/c/13737/2/be/src/util/bit-stream-utils-test.cc@320 PS2, Line 320: nit:extra line 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: UB Can you expand the abbreviation? It took me a few seconds to understand it. http://gerrit.cloudera.org:8080/#/c/13737/2/be/src/util/bit-stream-utils.inline.h@196 PS2, Line 196: UB Same as line 44. -- 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: 2 Gerrit-Owner: Daniel Becker <daniel.bec...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Fri, 28 Jun 2019 11:59:55 +0000 Gerrit-HasComments: Yes
