Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/24579 )
Change subject: IMPALA-15139: Support DELTA_LENGTH_BYTE_ARRAY Parquet encoding ...................................................................... Patch Set 8: (10 comments) most of my comments are kind of nits, one thing that I think is critical is to improve test data, see my comment in data generator - this is the reason why I removed the +2 from Zoltan http://gerrit.cloudera.org:8080/#/c/24579/8//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24579/8//COMMIT_MSG@20 PS8, Line 20: Changes: nit: this looks unnecessarily detailed to me, some of these are self-evident (e.g. CMakeLists.txt) http://gerrit.cloudera.org:8080/#/c/24579/8//COMMIT_MSG@51 PS8, Line 51: Testing: looks too detailed to me http://gerrit.cloudera.org:8080/#/c/24579/8/be/src/exec/parquet/parquet-column-readers.cc File be/src/exec/parquet/parquet-column-readers.cc: http://gerrit.cloudera.org:8080/#/c/24579/8/be/src/exec/parquet/parquet-column-readers.cc@944 PS8, Line 944: SetPlainDecodeError(); This looks misleading, this is not a plain encoded page. http://gerrit.cloudera.org:8080/#/c/24579/8/be/src/exec/parquet/parquet-column-readers.cc@1052 PS8, Line 1052: SetPlainDecodeError(); Same as line 944 http://gerrit.cloudera.org:8080/#/c/24579/8/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.cc File be/src/exec/parquet/parquet-delta-length-byte-array-decoder.cc: http://gerrit.cloudera.org:8080/#/c/24579/8/be/src/exec/parquet/parquet-delta-length-byte-array-decoder.cc@105 PS8, Line 105: *sv = StringValue(reinterpret_cast<char*>(const_cast<uint8_t*>(data_)), len); we should try to smallify the string here http://gerrit.cloudera.org:8080/#/c/24579/8/be/src/exec/parquet/parquet-metadata-utils.cc File be/src/exec/parquet/parquet-metadata-utils.cc: http://gerrit.cloudera.org:8080/#/c/24579/8/be/src/exec/parquet/parquet-metadata-utils.cc@335 PS8, Line 335: // DELTA_BINARY_PACKED is listed in column chunk metadata by some writers when pass schema_element.type to IsEncodingSupported and check there? http://gerrit.cloudera.org:8080/#/c/24579/8/testdata/parquet_delta_length_byte_array_encoding/README File testdata/parquet_delta_length_byte_array_encoding/README: http://gerrit.cloudera.org:8080/#/c/24579/8/testdata/parquet_delta_length_byte_array_encoding/README@1 PS8, Line 1: The mixed_types_delta_length_byte_array.parquet file was generated with the nit: new data files in the repo are mainly added in https://github.com/apache/impala/tree/master/testdata/data http://gerrit.cloudera.org:8080/#/c/24579/8/testdata/parquet_delta_length_byte_array_encoding/parquet_files_generator.py File testdata/parquet_delta_length_byte_array_encoding/parquet_files_generator.py: http://gerrit.cloudera.org:8080/#/c/24579/8/testdata/parquet_delta_length_byte_array_encoding/parquet_files_generator.py@39 PS8, Line 39: str_vals = ["hello", "world", "", "impala", None, "delta"] : bin_vals = [b"foo", b"bar", b"", b"baz", None, b"qux"] These strings are too small IMO, all can be small string optimized. there should be a few larger ones. Also, this won't exercise the batch execution path due to the enbedded null http://gerrit.cloudera.org:8080/#/c/24579/8/tests/query_test/test_parquet_delta_length_byte_array_encoding.py File tests/query_test/test_parquet_delta_length_byte_array_encoding.py: http://gerrit.cloudera.org:8080/#/c/24579/8/tests/query_test/test_parquet_delta_length_byte_array_encoding.py@24 PS8, Line 24: TestParquetDeltaLengthByteArrayEncoding not sure if this needs its own file, many similar tests exist in test_scanners.py http://gerrit.cloudera.org:8080/#/c/24579/8/tests/query_test/test_parquet_delta_length_byte_array_encoding.py@53 PS8, Line 53: _create_test_table creating the table multiple times looks a bit wasteful - could be merged to one test also, .test files are much more readable for the kinds of test -- To view, visit http://gerrit.cloudera.org:8080/24579 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e4395fa30cd7fb2c89ef5a2b16b043d4de60b52 Gerrit-Change-Number: 24579 Gerrit-PatchSet: 8 Gerrit-Owner: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Balazs Hevele <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Mihaly Szjatinya <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Sat, 25 Jul 2026 13:24:06 +0000 Gerrit-HasComments: Yes
