andygrove opened a new pull request, #5729:
URL: https://github.com/apache/datafusion-comet/pull/5729
## Which issue does this PR close?
Closes #5691.
Closes #5694.
Closes #5693 (same root cause as #5691, one step further along).
## Rationale for this change
Two native Iceberg write panics, both crossing the JNI boundary as a
`CometNativeException`
rather than surfacing as an error Comet could fall back on or report cleanly.
**#5691 / #5693 — evolved partition spec.** iceberg-java's
`UpdatePartitionSpec` keeps a dropped
partition field in a format-version-1 spec as a `void` transform so its
field id survives, and
`PartitionSpec#isUnpartitioned` means "every field is `void`", not "no
fields" — on the Rust side
too. The next write therefore routes through `UnpartitionedWriter`, which
stamps every data file
with an empty partition struct, while `ManifestWriter` derives one partition
summary per spec
field and `zip_eq`s the two:
```
itertools: .zip_eq() reached end of one iterator before the other
at
<iceberg::spec::manifest::writer::ManifestWriter>::construct_partition_summaries
```
Dropping the `void` field's source column afterwards then broke the
manifest's `partition_type`
resolution as well (`No column with source column id 2 in schema`), which is
#5693.
**#5694 — `timestamptz` partition path.** iceberg-rust's
`microseconds_to_datetimetz` takes
`micros % 1_000_000` — negative for a pre-1970 value — casts it to `u32` and
multiplies by 1000,
then unwraps the `None` that `DateTime::from_timestamp` returns for the
resulting out-of-range
nanosecond count. `RandomData` in Iceberg's own tests generates timestamps as
`random.nextLong() % FIFTY_YEARS_IN_MICROS`, so the value reaching the
conversion is a legitimate
pre-epoch timestamp; the conversion is simply wrong for negative inputs.
Still present on
iceberg-rust `main`, so there is nothing to bump the pin to.
## What changes are included in this PR?
**#5691 / #5693.** `encode_data_files_as_manifest` now encodes the per-task
transport manifest
against a field-less spec of the same spec id whenever `is_unpartitioned()`
holds (a no-op in the
common already-field-less case), so the manifest's partition arity matches
the empty partition
struct the writer produced. Nothing downstream loses information: the JVM
re-reads this manifest
with the spec embedded in its own Avro metadata, then rebuilds each
`DataFile` against the real
output spec, whose `DataFiles.Builder` drops partition data outright for an
unpartitioned spec —
so the manifest that reaches storage carries exactly what iceberg-java's own
writer would have
committed. Dropping the fields also skips the `partition_type` resolution
that #5693 tripped over.
**#5694.** New `CometLocationGenerator`
(`native/core/src/execution/operators/iceberg_partition_path.rs`)
replaces iceberg-rust's `DefaultLocationGenerator`. It lays files out
identically but renders the
partition path itself, mirroring iceberg-java's
`PartitionSpec#partitionToPath`. Per-field it
delegates to `Transform::to_human_string` and overrides only the arms where
iceberg-rust disagrees
with iceberg-java:
| Iceberg type | iceberg-java | iceberg-rust |
| --- | --- | --- |
| `timestamp` | `1969-12-31T23:59:58.5` | `1969-12-31 23:59:58.500` |
| `timestamptz` | `1969-12-31T23:59:58.5+00:00` | panics on a negative value
with a sub-second part; otherwise `1969-12-31 23:59:58.500 UTC` |
| `binary` / `fixed` | base64 | uppercase hex |
I widened it past `timestamptz` because a function whose stated job is
iceberg-java parity should
not knowingly leave two adjacent arms wrong, and Iceberg's own
`TestSparkDataFile` SPEC (the
#5694 reproducer) partitions on `binary` and `timestamp` as well as
`timestamptz`. The name/value
escaping is unchanged: `form_urlencoded` leaves exactly the byte set
`URLEncoder.encode(s, UTF_8)`
leaves.
The generator resolves the partition type once per task instead of per file,
which turns a spec it
could not render into an error at task start rather than a panic inside the
infallible
`LocationGenerator::generate_location`.
One deliberate gap, documented in the code and in `iceberg-writes.md`:
`float` and `double` still
delegate, so a float partition directory reads `f=1` where iceberg-java
writes `f=1.0`. Matching
Java means porting `Float.toString`/`Double.toString` — the same port Comet's
`cast(float as string)` needs — which is much more than a partition
directory name warrants, and
unlike `timestamptz` it does not panic. Distinct partition values still get
distinct directories,
and no reader parses these names.
## How are these changes tested?
New tests, all of which I confirmed **fail against the pre-fix native
library with the exact
panics from the issues** (`zip_eq() reached end of one iterator before the
other` for #5691;
`attempt to multiply with overflow` for #5694 — the debug-build face of the
release-build
`Option::unwrap()` in the issue) and pass with it:
- `CometIcebergWriteActionSuite`, two end-to-end parity tests that write the
same data twice, once
natively and once through iceberg-java's writer, and compare:
- `timestamptz and binary partition paths match iceberg-java` — the full
set of committed
partition directories must be equal between the two tables, plus pinned
expectations for the
pre-epoch sub-second value, the epoch, and a microsecond-precision value.
- `writes after a V1 partition field is dropped match iceberg-java` —
walks the exact sequence
from #5691 (`ADD PARTITION FIELD` → write → `DROP PARTITION FIELD` →
write → `DROP COLUMN` →
write) and compares rows and the committed manifests'
`partition_spec_id` /
`partition_summaries`.
- `iceberg_write.rs`:
`void_only_spec_write_round_trips_through_the_manifest`,
`void_field_with_a_dropped_source_column_still_writes`, and
`pre_epoch_timestamptz_partition_gets_a_java_shaped_directory` drive the
real iceberg-rust
writer stack.
- `iceberg_partition_path.rs`: unit tests pinning each rendering against the
strings the Java
source produces, including `ISO_LOCAL_DATE_TIME`'s always-print-seconds and
strip-trailing-zeros-fraction behaviour (`.5`, not `.500000`),
`EXCEEDS_PAD` year formatting,
and timestamps past chrono's year-262143 calendar ceiling (which `i64`
micros can reach and
iceberg-java has no ceiling for, hence the hand-rolled `civil_from_days`).
Full runs: the whole native workspace (`cargo test --workspace`) and all
seven Comet Iceberg
suites (233 tests) pass.
Not verified here: Iceberg's own gradle suites
(`TestAlterTablePartitionFields`,
`TestSparkDataFile`). Gradle cannot start in my environment — it fails
binding its
`FileLockContentionHandler` socket. Two notes for reviewers on that:
1. The `iceberg_spark_test_1.11` workflow will not catch these either,
because
`dev/diffs/iceberg/1.11.0.diff` does not set
`spark.comet.iceberg.write.enabled` — the native
write path is only reachable in those suites once #5677 turns it on. That
is why I put the
equivalent iceberg-java-parity coverage in Comet's own suite instead.
2. The panic in #5694 is upstream in iceberg-rust, unfixed on `main`; this
PR routes around it
rather than fixing it there. Happy to file the upstream issue and add the
reference here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
