Andrew Sherman 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 1: (5 comments) Another patch should be coming soon http://gerrit.cloudera.org:8080/#/c/11582/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11582/1//COMMIT_MSG@31 PS1, Line 31: All unit tests pass > I assume that you ran the e2e tests, and also that you checked that there i I did run e2e tests, and now I have also checked that they encode values of bit width 1 and 2. Bit width 1 is used when writing definition levels (i.e. when a value is null or not). Bit width 2 is used when encoding a dictionary with 3 or 4 entries. http://gerrit.cloudera.org:8080/#/c/11582/1/be/src/util/rle-encoding.h File be/src/util/rle-encoding.h: http://gerrit.cloudera.org:8080/#/c/11582/1/be/src/util/rle-encoding.h@208 PS1, Line 208: min_repeated_run_length > mention that it must be a multiple of 8, less than MAX_RUN_LENGTH_BUFFER Done http://gerrit.cloudera.org:8080/#/c/11582/1/be/src/util/rle-encoding.h@209 PS1, Line 209: which is the best choice > seems like from your DCHECK(TestInfo::is_test()) that this is the required Done http://gerrit.cloudera.org:8080/#/c/11582/1/be/src/util/rle-encoding.h@242 PS1, Line 242: MinRepeatedRunLength(bit_width) > should this be min_repeated_run_length_, in case its set to a non-default v I've changed this function again after thinking it through more carefully, and it no longer uses MinRepeatedRunLength() in this way. http://gerrit.cloudera.org:8080/#/c/11582/1/be/src/util/rle-encoding.h@380 PS1, Line 380: run_length > min_repeated_run_length_? Done -- 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: 1 Gerrit-Owner: Andrew Sherman <[email protected]> Gerrit-Reviewer: Andrew Sherman <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Mon, 15 Oct 2018 23:52:21 +0000 Gerrit-HasComments: Yes
