Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/12172 )
Change subject: IMPALA-7979: Enhance decoders to support value-skipping ...................................................................... Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/12172/1/be/src/exec/parquet/parquet-bool-decoder-test.cc File be/src/exec/parquet/parquet-bool-decoder-test.cc: http://gerrit.cloudera.org:8080/#/c/12172/1/be/src/exec/parquet/parquet-bool-decoder-test.cc@75 PS1, Line 75: 100 falses, 100 trues, 100 falses > Changed to write 300 values. Sorry, I am still not 100% ok with the testdata, as it will only use repeated runs in the RLE case. A run could be added that adds alternating data (i % 2 == 0). http://gerrit.cloudera.org:8080/#/c/12172/3/be/src/exec/parquet/parquet-bool-decoder-test.cc File be/src/exec/parquet/parquet-bool-decoder-test.cc: http://gerrit.cloudera.org:8080/#/c/12172/3/be/src/exec/parquet/parquet-bool-decoder-test.cc@73 PS3, Line 73: TestPlainSkip The name seems a bit misleading, as the test checks both plain and RLE encoded booleans. http://gerrit.cloudera.org:8080/#/c/12172/3/be/src/util/rle-test.cc File be/src/util/rle-test.cc: http://gerrit.cloudera.org:8080/#/c/12172/3/be/src/util/rle-test.cc@301 PS3, Line 301: ValidateRleSkip(seq, bit_width, min_run_length, 0, 7); optional: these random looking numbers suggests to me that it would make sense to actually run random tests. These may bump to edge cases that the current test avoid at the moment. A few fix tests could be kept to guarantee code coverage. -- To view, visit http://gerrit.cloudera.org:8080/12172 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib848f1bd71735fe84e8064daf700417b32589f57 Gerrit-Change-Number: 12172 Gerrit-PatchSet: 3 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Wed, 16 Jan 2019 16:40:29 +0000 Gerrit-HasComments: Yes
