Todd Lipcon has posted comments on this change. Change subject: KUDU-100 make RLE encoder handle 64-bit integer. ......................................................................
Patch Set 1: (6 comments) Are you planning on another patch which will enable the ability to use RLE for the int64 type, with an appropriate test? http://gerrit.cloudera.org:8080/#/c/4822/1/src/kudu/util/bit-stream-utils.inline.h File src/kudu/util/bit-stream-utils.inline.h: Line 28: inline void BitWriter::PutValue(uint64_t v, int num_bits) { I think we should still DCHECK_LE(num_bits, 64) Line 31: v &= BitUtil::ShiftLeft(1ULL, num_bits) - 1; this is introducing an extra branch in this relatively hot function. I think it's equivalent to do: v &= ~0ULL >> (64 - num_bits) which doesn't introduce any extra branch Line 34: buffered_values_ |= BitUtil::ShiftLeft(v, bit_offset_); does this one need the extra safety check? again, it's extra branches which are expensive, and I think the code down below (line 37) makes sure that bit_offset_ is always < 64 Line 111: inline bool BitReader::GetValue(int num_bits, T* v) { same, just change the assertion to <= 64 http://gerrit.cloudera.org:8080/#/c/4822/1/src/kudu/util/bit-util.h File src/kudu/util/bit-util.h: Line 42: static inline uint64_t ShiftLeft(uint64_t v, int num_bits) { can you rename these functions to something like ShiftLeftZeroOnOverflow to clarify the purpose? http://gerrit.cloudera.org:8080/#/c/4822/1/src/kudu/util/rle-test.cc File src/kudu/util/rle-test.cc: Line 304: // Test some random sequences. can you update this comment and test name to indicate a random sequence of bools? -- To view, visit http://gerrit.cloudera.org:8080/4822 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I40974af3a9330cdfe4410c16293f330d0eccd03d Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Haijie Hong <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
