Thomas Marshall 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) 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 is an existing e2e tests that involve using RleEncoder to write values of bit width 1 and 2 (eg. in tests/query_tests/test_insert_parquet.py or similar) and if not could you add some? 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 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 choice for non-test code 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 value? http://gerrit.cloudera.org:8080/#/c/11582/1/be/src/util/rle-encoding.h@380 PS1, Line 380: run_length min_repeated_run_length_? -- 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: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Thomas Marshall <[email protected]> Gerrit-Comment-Date: Mon, 08 Oct 2018 22:28:08 +0000 Gerrit-HasComments: Yes
