Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/12621 )
Change subject: IMPALA-8253: Parquet delta encoding and decoding. ...................................................................... Patch Set 19: (3 comments) http://gerrit.cloudera.org:8080/#/c/12621/19/be/src/exec/parquet/parquet-delta-decoder.cc File be/src/exec/parquet/parquet-delta-decoder.cc: http://gerrit.cloudera.org:8080/#/c/12621/19/be/src/exec/parquet/parquet-delta-decoder.cc@298 PS19, Line 298: template int ParquetDeltaDecoder<int32_t>::NextValuesConverted<int8_t>(int num_values, I'm not sure it is a good idea to explicitly instantiate NextValuesConverted() here because there are too many possibilities and the compile time of this file is very long. Maybe we should move the NextValuesConverted() implementation back to the header. 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: > Yeah, CountLeadingZeros should check this. I opened IMPALA-12086 for 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: > nit: fits previous line Done -- 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: 19 Gerrit-Owner: Daniel Becker <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 26 Apr 2023 09:28:18 +0000 Gerrit-HasComments: Yes
