Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12621 )
Change subject: IMPALA-8253: Parquet delta encoding and decoding. ...................................................................... Patch Set 16: (8 comments) I've ran over it and left a few nit comments. http://gerrit.cloudera.org:8080/#/c/12621/16//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12621/16//COMMIT_MSG@17 PS16, Line 17: Could you write something about the benchmark? At least what are the results of the measurements. http://gerrit.cloudera.org:8080/#/c/12621/16/be/src/benchmarks/parquet-delta-benchmark.cc File be/src/benchmarks/parquet-delta-benchmark.cc: http://gerrit.cloudera.org:8080/#/c/12621/16/be/src/benchmarks/parquet-delta-benchmark.cc@436 PS16, Line 436: , nit: line too long http://gerrit.cloudera.org:8080/#/c/12621/16/be/src/benchmarks/parquet-delta-benchmark.cc@480 PS16, Line 480: ride); nit: line too long http://gerrit.cloudera.org:8080/#/c/12621/16/be/src/benchmarks/parquet-delta-benchmark.cc@1058 PS16, Line 1058: 200, 400}; nit: line too long http://gerrit.cloudera.org:8080/#/c/12621/16/be/src/benchmarks/parquet-delta-benchmark.cc@1068 PS16, Line 1068: tride == 100 || config.stride == 400 */) nit: line too long http://gerrit.cloudera.org:8080/#/c/12621/16/be/src/exec/parquet/parquet-delta-decoder.h File be/src/exec/parquet/parquet-delta-decoder.h: http://gerrit.cloudera.org:8080/#/c/12621/16/be/src/exec/parquet/parquet-delta-decoder.h@44 PS16, Line 44: { : } nit: you could put these to L43. http://gerrit.cloudera.org:8080/#/c/12621/2/be/src/exec/parquet/parquet-delta-encoder.h File be/src/exec/parquet/parquet-delta-encoder.h: http://gerrit.cloudera.org:8080/#/c/12621/2/be/src/exec/parquet/parquet-delta-encoder.h@149 PS2, Line 149: er_pos_, output_buffer_len > Checking the code of CountLeadingZeros, I can see it uses __builtin_clz, wh Yeah, CountLeadingZeros should check this. http://gerrit.cloudera.org:8080/#/c/12621/16/be/src/util/bit-packing.cc File be/src/util/bit-packing.cc: http://gerrit.cloudera.org:8080/#/c/12621/16/be/src/util/bit-packing.cc@69 PS16, Line 69: bool* __restrict__ decode_error); nit: fits previous line -- To view, visit http://gerrit.cloudera.org:8080/12621 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie7378ac1a490a6c89a0a4349aae86cbc0fbc80f8 Gerrit-Change-Number: 12621 Gerrit-PatchSet: 16 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabor Kaszab <[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: Mon, 29 Jun 2020 17:10:24 +0000 Gerrit-HasComments: Yes
