Will Berkeley has posted comments on this change. Change subject: WIP: KUDU-1848. in-memory dictionary for binary columns ......................................................................
Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/5780/1/src/kudu/tablet/memrowset.cc File src/kudu/tablet/memrowset.cc: PS1, Line 119: arraysize(slice_dict_) Can this just be kSliceDictSize? PS1, Line 154: hash & (DICT_SIZE - 1) This works correctly only if DICT_SIZE is a power of 2, right? Could you add a quick comment or check that DICT_SIZE is a power of two, just in case? PS1, Line 176: // <24 bits length> : // <48 bits pointer> Isn't 24 + 48 = 72, so there's 1 too many bytes to store in 64 bits? PS1, Line 190: kMaxSize = (1 << 24) - 1; If my comment above isn't mistaken, max size should be (1 << 16) - 1 -- To view, visit http://gerrit.cloudera.org:8080/5780 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic03ef0473383b1dcf22ebefee029ff29d2dae813 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <wdberke...@gmail.com> Gerrit-HasComments: Yes
