Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/24505 )
Change subject: IMPALA-15200: Add Parquet UUID read support for Iceberg tables ...................................................................... Patch Set 6: (9 comments) Thanks for working on this! http://gerrit.cloudera.org:8080/#/c/24505/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24505/6//COMMIT_MSG@26 PS6, Line 26: Testing Now it's also possible to add interop tests with Trino, see * https://github.com/apache/impala/blob/master/tests/custom_cluster/test_trino_interop.py * https://github.com/apache/impala/blob/master/testdata/workloads/functional-query/queries/QueryTest/iceberg-trino-interop-insert.test http://gerrit.cloudera.org:8080/#/c/24505/6/be/src/exec/parquet/parquet-column-readers.cc File be/src/exec/parquet/parquet-column-readers.cc: http://gerrit.cloudera.org:8080/#/c/24505/6/be/src/exec/parquet/parquet-column-readers.cc@325 PS6, Line 325: //TODO: use constexpr ifs when we switch to C++17. Since we switched to C++17 this TODO can be applied. http://gerrit.cloudera.org:8080/#/c/24505/6/be/src/exec/parquet/parquet-column-readers.cc@887 PS6, Line 887: Read 16 raw bytes into a temporary StringValue : // before copying into the inline TYPE_UUID slot. It would be cleaner to introduce a UuidValue object with an internal std::array<uint8_t, 16>, and memcpy directly the bytes directly. We could also set col_chunk_reader_.keep_data_page_pool_ = false; in this case. http://gerrit.cloudera.org:8080/#/c/24505/6/be/src/exec/parquet/parquet-column-readers.cc@895 PS6, Line 895: DCHECK_EQ(fixed_len_size_, UUID_BYTE_LEN); fixed_len_size_ is set on Parquet metadata (node.element->type_length). Which means on invalid Parquet files this DCHECK can fire, and release builds could behave incorrectly. I think we should raise a runtime error when UUID's node.element->type_length is not UUID_BYTE_LEN. http://gerrit.cloudera.org:8080/#/c/24505/6/be/src/exec/parquet/parquet-data-converter.h File be/src/exec/parquet/parquet-data-converter.h: http://gerrit.cloudera.org:8080/#/c/24505/6/be/src/exec/parquet/parquet-data-converter.h@104 PS6, Line 104: if (col_type_->type == TYPE_UUID) { : return true; : } Instead of doing a conversion, we could just copy the bytes directly. UUID isn't like CHAR where we might need to add padding. http://gerrit.cloudera.org:8080/#/c/24505/6/be/src/runtime/raw-value.cc File be/src/runtime/raw-value.cc: http://gerrit.cloudera.org:8080/#/c/24505/6/be/src/runtime/raw-value.cc@86 PS6, Line 86: case TYPE_UUID: : stream->write(chars, type.len); This could be merged with TYPE_CHAR: case TYPE_CHAR: case TYPE_UUID: stream->write(chars, type.len); break; http://gerrit.cloudera.org:8080/#/c/24505/6/be/src/service/fe-support.cc File be/src/service/fe-support.cc: http://gerrit.cloudera.org:8080/#/c/24505/6/be/src/service/fe-support.cc@156 PS6, Line 156: RawValue::PrintValue(value, type, -1, &col_val->string_val); : col_val->__isset.string_val = true; Why is it needed? Please also add a code comment with the reason. http://gerrit.cloudera.org:8080/#/c/24505/6/testdata/data/README File testdata/data/README: http://gerrit.cloudera.org:8080/#/c/24505/6/testdata/data/README@1015 PS6, Line 1015: Iceberg V3 Parquet table for UUID read-path tests. Mention what engine/version you used to create the table. http://gerrit.cloudera.org:8080/#/c/24505/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-uuid-type.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-uuid-type.test: http://gerrit.cloudera.org:8080/#/c/24505/6/testdata/workloads/functional-query/queries/QueryTest/iceberg-uuid-type.test@5 PS6, Line 5: DESCRIBE iceberg_uuid_test Add test for DESCRIBE FORMATTED as well. -- To view, visit http://gerrit.cloudera.org:8080/24505 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4157c002e80677d27d8fd060c7bfa07b95d7c78f Gerrit-Change-Number: 24505 Gerrit-PatchSet: 6 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 28 Jul 2026 18:58:46 +0000 Gerrit-HasComments: Yes
