[sam-1112]

Which issue does this PR close?

Partially addresses #6959 (UUID writes).

Rationale for this change

Spark represents Iceberg UUID columns as strings. The native writer currently declines these columns because Arrow casting does not provide Java-compatible UUID parsing.

This change enables native UUID writes while preserving iceberg-java’s value, metrics and failure-cleanup semantics.

What changes are included in this PR?

• Add Java-compatible UUID parsing, including shortened groups, leading plus signs, Unicode digits, group masking and overflow rejection.
• Convert UUID fields recursively through structs, lists and map keys/values, using Iceberg field IDs to distinguish UUID from fixed(16).
• Skip invisible values under null parents and outside sliced collection ranges. Preserve the existing cast path for UUID-free subtrees.
• Allow UUID data columns while retaining fallback for UUID partition/sort sources and unsupported v3 types.
• Add reproducible Java oracle fixtures, regression coverage, documentation and dedicated UUID benchmark workloads.

Native UUID files use FIXED_LEN_BYTE_ARRAY(16) with Iceberg field IDs and big-endian UUID bytes. The pinned iceberg-rust writer omits the Parquet UUID logical annotation. This physical-layout difference is documented and tested: Iceberg readers use the table schema, while standalone Parquet readers relying on the annotation see fixed-length binary.

How are these changes tested?

Latest local validation:

• 9 Rust UUID tests passed, including multi-level null visibility, nonzero map offsets, string representations and fixed(16) isolation.
• 14 Spark 4.1 UUID tests passed, covering native/Java row and metrics parity, footer metadata, writer modes, AQE, eligibility and failure cleanup for flat UUIDs, list elements and map values.
• Java oracle fixtures matched on JDK 17 and 21: 2,516 UUID inputs plus BMP digit classification.
• Clippy with warnings denied, compilation and formatting checks passed.

Earlier regression runs passed 174 native Iceberg tests and 211 tests across the complete Spark 4.1 detection/action suites. UUID-focused runs also passed on Spark 3.4/3.5/4.0/4.1 with JDK 17 and Spark 4.0/4.1 with JDK 21. These runs preceded the final test-only coverage additions.

Local benchmarks used an Apple M4 (16 GiB), JDK 17, Spark 4.1.3 and Iceberg 1.11.0, with a release native build and local storage. Each workload processed 4,194,304 rows, with at least five measured iterations per arm.

Writer factor = median time of Comet scan + Java writer / median time of Comet scan + native writer.

UUID workload	Run 1 writer factor	Run 2 writer factor
Canonical UUIDs	1.77×	1.99×
Canonical UUIDs with approximately 1/8 nulls	1.53×	1.08×
Java-compatible shortened UUIDs	1.46×	1.21×

All arms passed native-plan and checksum validation. The null workload showed substantial variability: native write times ranged from 206.78 to 715.45 ms in the second run. A non-UUID control was also measured, but no matched pre-change baseline was collected, so it does not establish the absence of a non-UUID performance regression.

[andygrove]

Thanks for this, @sam-1112. The Java oracle fixtures (including every BMP code unit against Character.digit) are exactly the evidence a parity claim needs. The visibility masking for null parents and sliced list and map windows is carefully tested, and the native versus iceberg-java comparison covers rows, counts and bounds in the unpartitioned, clustered and fanout modes with AQE on and off. The gate also fails closed, since an unresolvable spec becomes Unsupported.

I compared the touched paths against branch-1.2. Behavior only changes when spark.comet.write.iceberg.enabled is on, and then only for tables with UUID data columns, which used to fall back to iceberg-java. Non-UUID writes take the same cast_with_options path with safe: false as before.

I added run-iceberg-tests and approved the pending workflow runs, so CI should report on the Iceberg suites shortly. The gate is now wider, so I'd like to see that result.

The one thing I'd like to discuss is the missing UUID logical annotation. The Iceberg spec maps uuid to fixed_len_byte_array(16) with the UUID logical type, and these native files will not carry it. The PR documents this, which I appreciate. The guide says Iceberg readers use the table schema and recover the same values, but the readback evidence is from the Spark and iceberg-java reader. Did you look at whether parquet can write the annotation, for example by marking the Arrow field with the arrow.uuid extension and enabling the arrow_canonical_extension_types feature in native/Cargo.toml? If that does not work, have you confirmed that other engines that read Iceberg tables, such as Trino and Flink, read these files back as UUID? I would like the "no reader is affected" claim in iceberg.md to rest on more than one reader.

Two smaller things to fix in this PR:

• The comment at CometIcebergNativeWrite.scala:964 still refers to decorate_batch_with_field_ids, which this PR removes. Could you point it at UuidAlignment::convert? The same name appears in .ai/skills/review-comet-iceberg-write-pr/SKILL.md.
• contributor-guide/iceberg-writes.md has a test table and a list of native adaptations. Could you add iceberg_uuid.rs and the new UUID action and detection tests there?

[andygrove]

A follow-up on my question about the missing UUID annotation, since I looked into how other engines treat these files and into what Comet can do about it.

The Iceberg Spark readers (row and vectorized) pick the UUID reader from the Iceberg field type, not from the Parquet annotation. The Flink reader returns raw bytes for any fixed-length binary. Trino reads unannotated FIXED_LEN_BYTE_ARRAY(16) as UUID on purpose, because Iceberg 0.11.1 wrote UUID that way (see ColumnReaderFactory in trino-parquet). So the engines I checked cope with these files. DuckDB's Parquet reader only produces a UUID type when the annotation is present, so tools that read the Parquet schema directly do see a difference.

My earlier suggestion to set the arrow.uuid extension on our side won't work. iceberg-rust builds the AsyncArrowWriter schema from the Iceberg schema in parquet_writer.rs, not from the batches, so Comet can't inject it. The fix is upstream. apache/iceberg-rust#2930 (merged 2026-10-15, after our pin at 1f3bc34) tags UUID fields with arrow.uuid, and crates/iceberg already enables the arrow_canonical_extension_types feature on parquet. I haven't tried a bump, but I'd expect a pin that includes #2930 to write the annotation.

I don't think this PR should bump the pin. A pin bump is a writer change that needs its own review and test run, and #6934 is already open for the next one. What I'd suggest here is that the divergence note in iceberg.md says the annotation arrives with an iceberg-rust pin that includes #2930, so the next person to bump the pin knows to remove it. The assertion leaf.getLogicalTypeAnnotation == null in the new test will fail on that bump, which is a useful tripwire. Maybe a short comment next to it saying so would help.

[comphead]

Porting UUID.fromString keeps the result identical to iceberg-java's UUIDWriter, and the Java oracle fixtures back that up. A local replay of uuid-java.tsv through UUID.fromString on JDK 11 and 17 showed no differences. The pinned iceberg-rust maps Uuid to FixedSizeBinary(16) and handles its statistics. andygrove's asks from 2026-10-27 are still open at this head and not repeated here. This conflicts with #7100 in four files, including iceberg_write.rs, where this PR deletes decorate_batch_with_field_ids and #7100 edits it. Four comments.

[comphead]

#7100 makes a top-level unknown column eligible by dropping Null columns in decorate_batch_with_field_ids before this count check, and this PR deletes that function. Whichever lands second needs UuidAlignment::convert to skip Null columns before comparing batch.num_columns() with self.0.len(), or a table with both a uuid and an unknown column fails with column count mismatch. fall-back: uuid does not make an unknown column eligible would then also start failing, since that table becomes eligible. Could the two of you coordinate, and turn that test into a native-write parity test for a uuid plus unknown table? This is from reading both diffs, not from a merged run. Separately, the old message carried "arrow batch has N columns but schema has M", which helped with the earlier pruning and ReplaceData projection bugs. Could the counts stay?

[comphead]

batch row {row} is an index inside one Comet batch, so a user cannot map it back to their data. Java's UUID.fromString error names the offending value. Could this message include the value, truncated, next to the field name?

[comphead]

child_visibility allocates a Vec<bool> the length of the child values for every list and map on every batch. parquet_support.rs already has repeated_visibility, which handles entries outside a slice and under null parents and returns None when nothing is hidden, so the common case allocates nothing. Could it be shared, and the struct active mask skipped when there are no nulls and visible is None? The signatures differ (offsets plus a parent NullBuffer against Option<&[bool]>), so this needs a check that it fits.

[comphead]

The gate now declines a UUID partition or sort source, but this row still says "any, except a void field ...", and the rule appears only in the paragraph further down. Could the row, or a sort-order row, list it so the table shows every fallback? #7100 edits the same table.