Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11582 )
Change subject: IMPALA-6658: improve Parquet RLE for low bit widths ...................................................................... Patch Set 8: Code-Review+1 (4 comments) Some typos and optional changes, lgtm otherwise. http://gerrit.cloudera.org:8080/#/c/11582/8/be/src/util/rle-encoding.h File be/src/util/rle-encoding.h: http://gerrit.cloudera.org:8080/#/c/11582/8/be/src/util/rle-encoding.h@259 PS8, Line 259: MaxBufferSize I am not 100% sure here, but I think that this could be expressed in an easier to understand way: - calculate num_bytes (or num_bytes_plain) - calculate the overhead depending on bit_width: on byte/MAX_VALUES_PER_LITERAL_RUN or 1 byte / 2 or 3 bytes for bit width 2 and 1 - return std::max(MinBufferSize(bit_width), num_bytes + overhead); http://gerrit.cloudera.org:8080/#/c/11582/8/be/src/util/rle-encoding.h@261 PS8, Line 261: encoded Maybe repeated would be better here? http://gerrit.cloudera.org:8080/#/c/11582/8/be/src/util/rle-test.cc File be/src/util/rle-test.cc: http://gerrit.cloudera.org:8080/#/c/11582/8/be/src/util/rle-test.cc@636 PS8, Line 636: buffer_len I think that this var could have a more descriptive name like expected_max_buffer_len. http://gerrit.cloudera.org:8080/#/c/11582/8/be/src/util/rle-test.cc@642 PS8, Line 642: bug typo: big :) -- To view, visit http://gerrit.cloudera.org:8080/11582 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I191a581d3f699b6669e48ac9dc39c76ed77c4a76 Gerrit-Change-Number: 11582 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Andrew Sherman <asher...@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Comment-Date: Mon, 05 Nov 2018 15:59:53 +0000 Gerrit-HasComments: Yes
