andygrove opened a new pull request, #5759:
URL: https://github.com/apache/datafusion-comet/pull/5759
## Which issue does this PR close?
Closes #5758.
## Rationale for this change
Iceberg CI is red on `main`:
`TestForwardCompatibility.testSparkCanReadUnknownTransform` fails on the
Iceberg 1.8.1 legs with
```
org.apache.comet.CometNativeException: General execution error with reason:
Invalid Iceberg scan task: DataInvalid => Non-empty FileScanTask partition
requires a partition spec.
```
The table is partitioned by a `zero` transform that no Iceberg release
defines, standing in for one written by a newer Iceberg. Reading it is supposed
to work — that is Iceberg's forward-compatibility contract, and every data
column is present in the file.
Iceberg Java parses an unrecognized transform into an `UnknownTransform`
whose `toString()` is the original name, and whose `getResultType()` is
`StringType`, **not** `UnknownType`. So the "drop partition fields with an
unknown type" filter in `serializePartitionData` does not drop the field, and
the spec reaches native as `"transform":"zero"` with a real partition value
beside it. `serde_json::from_str::<PartitionSpec>` rejects `zero`, leaving the
task with `partition = Some(..)` and `partition_spec = None` — the one pair
iceberg-rust's `FileScanTask::validate()` rejects.
That validation is new: #5262 bumped iceberg-rust to a revision where
`FileScanTask`'s fields are private and construction goes through a validating
builder. Before it, the task was built by struct literal and nothing in this
query's read path consumed the missing spec, so it happened to work.
`Transform::Unknown` is iceberg-rust's model of the same situation and is
conservative in the right direction — no partition constants, no pruning — and
its result type is `string`, matching `UnknownTransform.getResultType`, so the
partition type Comet already serializes alongside the spec stays in agreement
with it.
## What changes are included in this PR?
- `IcebergReflection.Transforms.forNative` maps a transform name that
iceberg-rust's `Transform::from_str` would reject onto `unknown`.
`CometIcebergNativeScan` runs the spec's transform names through it when
building the spec JSON.
This is safe because the transform name reaches nothing in the native read
but the identity test that builds the partition constants map (`_spec_id` uses
the spec id; `_partition` matches partition values by field id). `identity` is
matched exactly and so is never rewritten, and every other transform
contributes no constants either way.
- Corrected the comments on the native side that described the old
(mistaken) understanding — that an unknown transform's field is filtered out
Scala-side as an unknown type, and that the spec-id-from-JSON fallback exists
for forward-compatibility tables. That fallback is now purely defensive.
The write side needs no equivalent change: Iceberg Java refuses to write
through an unknown transform before Comet is involved, which
`testSparkWriteFailsUnknownTransform` asserts.
## How are these changes tested?
Three layers, plus the Iceberg Java suites this fixes.
**End-to-end** (`CometIcebergNativeSuite`, new test): writes an
identity-partitioned table, rewrites its spec's transform to `zero`, and reads
it back through the native scan. With the fix reverted it fails with the CI
message verbatim; with the fix it passes on `spark-3.4`/Scala 2.12 (Iceberg
1.5.2), `spark-3.5` (1.8.1), and `spark-4.0` (1.10.0).
It is `assume`-skipped on 4.1, where Spark validates a V2 relation's
metadata columns on every read — forcing `SparkTable.metadataColumns()` ->
`Partitioning.partitionType()`, which rejects an unknown transform in the
analyzer with or without Comet. Iceberg disabled its own copy of the test there
for the same reason (SPARK-55626), which is also why only the 1.8.1 legs are
failing.
**Native** (`planner.rs`, new test): pins both directions — a spec carrying
`unknown` plus partition data builds a task that keeps its spec, its
`Transform::Unknown`, its partition value and a string-typed unified partition
type; the same input spelled `zero` still fails with the reported error.
**Scala** (`IcebergReflectionSuite`, two new tests): checks `forNative`
against real Iceberg `Transform` objects. The eight transforms iceberg-rust
understands round-trip unchanged (the round-trip assertion matters as much as
the answer — `forNative` matches on `Transform.toString`, so a version that
renders one differently from its JSON spelling would silently start rewriting
it), `zero` rewrites to `unknown`, and `partitionType()` for it is `string`.
Green on Iceberg 1.5.2, 1.8.1 and 1.11.0.
Also run: full `CometIcebergNativeSuite` (100 on 4.1, 95 on 3.5),
`CometIcebergNativeScanSuite` + `CometIcebergWriteDetectionSuite` +
`CometIcebergRewriteActionSuite` (60), all 275 native core tests, and
clippy/rustfmt/spotless/scalastyle.
--
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]
