Gabriella Gyorgyevics 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) Thanks :) The fixed_len_byte_array tests currently only run on two-directional tests, as creating test data for other sizes would be problematic, but we might add them later. 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 fo There weren't, now I've added them. 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? This was already covered in test.cc starting L153 and L161, but now I added another test (patch 27, test.cc, L171). I think these should cover all cases. -- 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: Fri, 24 Jan 2025 15:36:38 +0000 Gerrit-HasComments: Yes
