Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/24973 )
Change subject: IMPALA-15365: Iceberg UUID partition transform support ...................................................................... Patch Set 1: (9 comments) Left a few small comments, otherwise LGTM! http://gerrit.cloudera.org:8080/#/c/24973/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24973/1//COMMIT_MSG@9 PS1, Line 9: Allow only the identity, bucket(N, col) and void partition transforms Please mention it is not an Impala limitation, but the spec only allows these. http://gerrit.cloudera.org:8080/#/c/24973/1//COMMIT_MSG@13 PS1, Line 13: Iceberg compares UUIDs as signed longs UUIDs use unsigned byte-wise comparison: https://github.com/apache/iceberg/blob/7e3fe2f935540ed4c4a8dc8715d7236a64e62ee3/format/expressions-spec.md?plain=1#L176 http://gerrit.cloudera.org:8080/#/c/24973/1//COMMIT_MSG@14 PS1, Line 14: so Iceberg can skip data files that contain : matching rows Please mention that this is a bug in the Iceberg Java library: https://github.com/apache/iceberg/pull/14500 http://gerrit.cloudera.org:8080/#/c/24973/1/fe/src/main/java/org/apache/impala/common/IcebergPartitionPredicateConverter.java File fe/src/main/java/org/apache/impala/common/IcebergPartitionPredicateConverter.java: http://gerrit.cloudera.org:8080/#/c/24973/1/fe/src/main/java/org/apache/impala/common/IcebergPartitionPredicateConverter.java@71 PS1, Line 71: Manifest partition summaries are built with Iceberg's own signed comparator. Holds for writers that use the Java library (Impala, Trino, Spark). A writer that orders UUIDs unsigned (e.g. PyIceberg's min()/max()) would produce summaries that make ManifestEvaluator skip manifests. Please state this assumption in the comment. http://gerrit.cloudera.org:8080/#/c/24973/1/fe/src/main/java/org/apache/impala/common/IcebergPartitionPredicateConverter.java@73 PS1, Line 73: op == Operation.EQ || op == Operation.IN) != and NOT_IN could be allowed as well. http://gerrit.cloudera.org:8080/#/c/24973/1/fe/src/main/java/org/apache/impala/common/IcebergPartitionPredicateConverter.java@74 PS1, Line 74: canUsePartitionKeyScan Instead of using canUsePartitionKeyScan(), consider extracting logic to a new function: isIdentityPartitionedInAllSpecs(table, column) and calling it from both places. http://gerrit.cloudera.org:8080/#/c/24973/1/fe/src/main/java/org/apache/impala/common/IcebergPredicateConverter.java File fe/src/main/java/org/apache/impala/common/IcebergPredicateConverter.java: http://gerrit.cloudera.org:8080/#/c/24973/1/fe/src/main/java/org/apache/impala/common/IcebergPredicateConverter.java@475 PS1, Line 475: Iceberg compares UUIDs as signed longs Same as in commit message: it is a bug in the Iceberg Java Library, not in the Iceberg spec. http://gerrit.cloudera.org:8080/#/c/24973/1/fe/src/main/java/org/apache/impala/common/IcebergPredicateConverter.java@476 PS1, Line 476: // byte order, so Iceberg could skip data files that contain matching rows. When UUID is part of identity-partitioning, we could still push down = and IN, right? And we could push down bucket(u, N) = <hash> which is even more useful, since hash partitioning is probably the common case for UUID-columns. Not a blocker of this patch, it could be done in a follow-up patch. http://gerrit.cloudera.org:8080/#/c/24973/1/testdata/data/README File testdata/data/README: http://gerrit.cloudera.org:8080/#/c/24973/1/testdata/data/README@1035 PS1, Line 1035: iceberg_uuid_test_part Each of the 4 appends writes a single-file manifest, so the partition summaries never span the sign bit. The comment's claim that summaries are built with Iceberg's signed comparator is only covered by iceberg-trino-interop-uuid.test, which runs in exhaustive mode with the Trino container. Could the fixture be regenerated with one append, so a single manifest has a uuid_identity summary of signed [ffff..., 1234...]? Then the DROP PARTITION / SHOW FILES / SHOW PARTITIONS tests cover it in core. -- To view, visit http://gerrit.cloudera.org:8080/24973 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1dc3af4418285d6ea1d489912419be470e668f29 Gerrit-Change-Number: 24973 Gerrit-PatchSet: 1 Gerrit-Owner: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Arnab Karmakar <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Thu, 01 Oct 2026 15:19:46 +0000 Gerrit-HasComments: Yes
