Arnab Karmakar 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 11: (4 comments) http://gerrit.cloudera.org:8080/#/c/24505/10/be/src/runtime/uuid-value.h File be/src/runtime/uuid-value.h: http://gerrit.cloudera.org:8080/#/c/24505/10/be/src/runtime/uuid-value.h@32 PS10, Line 32: void Assign(const void* src, int len) { : DCHECK_EQ(len, BYTE_SIZE); > Unused? Yes, it was extra AI code. Removed them both. http://gerrit.cloudera.org:8080/#/c/24505/10/be/src/service/fe-support.cc File be/src/service/fe-support.cc: http://gerrit.cloudera.org:8080/#/c/24505/10/be/src/service/fe-support.cc@153 PS10, Line 153: // string_val holds the canonical UUID text for the FE to read back. : RawValue::PrintValue(value, type, -1, &col_val->string_val); : col_val->__isset.string_val = true; > Is it needed? Nice catch. Dropped it since the raw bytes are not being used anywhere. http://gerrit.cloudera.org:8080/#/c/24505/10/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/24505/10/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@676 PS10, Line 676: if (!(table instanceof FeIcebergTable)) return; > We should only call validateUuidReadSupported if we actually read UUID valu Done, we are now validating reads only when uuid columns are materialized. http://gerrit.cloudera.org:8080/#/c/24505/10/testdata/workloads/functional-query/queries/QueryTest/iceberg-trino-interop-uuid.test File testdata/workloads/functional-query/queries/QueryTest/iceberg-trino-interop-uuid.test: http://gerrit.cloudera.org:8080/#/c/24505/10/testdata/workloads/functional-query/queries/QueryTest/iceberg-trino-interop-uuid.test@103 PS10, Line 103: STRING,STRING,STRING,STRING > You could also add tests for UUIDs nested in complex types. Done. Added comprehensive coverage for complex types now: * ROW(uuid_col UUID, name VARCHAR) * ARRAY<UUID> * MAP<UUID, VARCHAR> * MAP<VARCHAR, UUID> * ARRAY<ROW(uuid_col UUID, name VARCHAR)> * ROW(ids ARRAY<UUID>, label VARCHAR) -- 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: 11 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: Michael Smith <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Tue, 25 Aug 2026 11:42:45 +0000 Gerrit-HasComments: Yes
