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 10: Code-Review+1 (4 comments) Just a few small issues, otherwise LGTM! 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: const uint8_t* data() const { return bytes_.data(); } : uint8_t* mutable_data() { return bytes_.data(); } Unused? 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: tmp.assign(reinterpret_cast<const char*>(value), type.len); : col_val->binary_val.swap(tmp); : col_val->__isset.binary_val = true; Is it needed? 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 values: boolean readsUuid = false; for (SlotDescriptor slot : desc_.getSlots()) { if (slot.isMaterialized() && slot.getType().containsUuid()) { readsUuid = true; break; } } if (!readsUuid) return; Then the column check could be dropped from validateUuidReadSupported(). 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: ==== You could also add tests for UUIDs nested in complex types. -- 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: 10 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: Mon, 24 Aug 2026 16:36:53 +0000 Gerrit-HasComments: Yes
