Noemi Pap-Takacs has posted comments on this change. ( http://gerrit.cloudera.org:8080/22165 )
Change subject: IMPALA-13648: Implement a decoder and an encoder for Byte Stream Split encoding ...................................................................... Patch Set 26: (2 comments) Nice patch! I went through the tests and noticed these 2 cases which I could not find coverage for, but maybe I just missed it. Could you please confirm if they are covered? http://gerrit.cloudera.org:8080/#/c/22165/26/be/src/exec/parquet/parquet-byte-stream-split-decoder.h File be/src/exec/parquet/parquet-byte-stream-split-decoder.h: http://gerrit.cloudera.org:8080/#/c/22165/26/be/src/exec/parquet/parquet-byte-stream-split-decoder.h@45 PS26, Line 45: This would usually be used : // for fixed_len_byte_array types. Do we have test coverage for this code path now since there are no tests for fixed_len_byte_array? http://gerrit.cloudera.org:8080/#/c/22165/26/be/src/exec/parquet/parquet-byte-stream-split-decoder.cc File be/src/exec/parquet/parquet-byte-stream-split-decoder.cc: http://gerrit.cloudera.org:8080/#/c/22165/26/be/src/exec/parquet/parquet-byte-stream-split-decoder.cc@54 PS26, Line 54: // If reading num_values number of values would overstep the total values stored, : // then decrease its value to read until the last value possible. Could you please cover this scenario in tests? -- To view, visit http://gerrit.cloudera.org:8080/22165 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icea60894ae22b8ddb7616aeda6d69012cc69972c Gerrit-Change-Number: 22165 Gerrit-PatchSet: 26 Gerrit-Owner: Gabriella Gyorgyevics <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Daniel Becker <[email protected]> Gerrit-Reviewer: Gabriella Gyorgyevics <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Noemi Pap-Takacs <[email protected]> Gerrit-Comment-Date: Thu, 23 Jan 2025 11:50:48 +0000 Gerrit-HasComments: Yes
