tlfua commented on issue #7240: URL: https://github.com/apache/datafusion-comet/issues/7240#issuecomment-6624579835
Reproduced on current `main` (0f7f8e453) in CI on my fork. Notes below, plus two related 1.12 findings and a proposed fix for discussion before I open a PR. ### Setup - Workflow: [`repro_7240.yml`](https://github.com/tlfua/datafusion-comet/blob/tlfua/ci/7240-iceberg-1.12-repro/.github/workflows/repro_7240.yml), run [41787526836](https://github.com/tlfua/datafusion-comet/actions/runs/41787526836) - `ubuntu-24.04`, container `amd64/rust`, the repo's `setup-builder` action (Zulu JDK 17) - `-Pspark-4.1` (Spark 4.1, Scala 2.13). The only change is the profile's `iceberg-spark-runtime-4.1_2.13` version, set by `sed` in CI: `1.11.0` (control) or `1.12.0` - `./mvnw -B -Pspark-4.1 -Dmaven.test.skip=true compile`, then a small Java main calls `IcebergReflection.executorReflectionUnresolved()`, the probe the native write gate uses. No native library is involved. - `javap` on the resolved runtime jar lists the `ManifestFiles.read` overloads. ### Results | Iceberg | public `ManifestFiles.read` overloads | `executorReflectionUnresolved()` | |---|---|---| | 1.11.0 | `(ManifestFile, FileIO)`, `(ManifestFile, FileIO, Map<Integer, PartitionSpec>)` | `None` | | 1.12.0 | `(ManifestFile, FileIO, Map<Integer, PartitionSpec>)` only | `Some(executor-side Iceberg reflection did not resolve: NoSuchMethodException: org.apache.iceberg.ManifestFiles.read(org.apache.iceberg.ManifestFile,org.apache.iceberg.io.FileIO))` | So on 1.12 the gate declines every native write at plan time and the write falls back to iceberg-java, as reported. ### Related 1.12 findings (test sources only) I first tried running `IcebergReflectionSuite` with the 1.12 pin, but the spark module's test sources do not compile against 1.12: 1. `CometIcebergWriteDetectionSuite.scala:1878`: 1.12 adds `default Configuration getConf()` to `HadoopConfigurable`, so `DetectionDelegatingFileIO.getConf` now needs `override`. It was left without `override` on purpose for versions where the member is absent. 2. `spark/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java` uses REST APIs that changed in 1.12: `RESTUtil.NAMESPACE_SPLITTER` is gone, `CatalogHandlers.loadTable` now takes a `SnapshotMode`, and `RESTUtil.decodeNamespace` now takes 2 args. Because of these, adding a 1.12 CI profile needs more than a version bump. I'd keep that separate from this fix. ### Proposed fix Both call sites (`probeExecutorReflection` and `readDataFilesFromManifest`) would resolve the 3-arg `read` through one shared helper, so the probe and the real call can't diverge. The open question is what to pass as `specsById`. **A. `null`.** On 1.5.2 through 1.11.0 the 2-arg overload is just `return read(manifest, io, null);`, so this is the same behavior as today on every supported version. With `null`, `ManifestReader` reads the spec embedded in the manifest's Avro metadata. That is what `manifest_partition_spec` in `iceberg_write.rs` relies on: void-only V1 specs are written with a field-less spec of the same id (#5691, #5693, from #5729). Downsides: - `ManifestReader` logs a deprecation WARN for a `null` map, once per task. 1.11 already logs it today through the 2-arg path. 1.10 and earlier don't log it. - 1.12 says the `null` path will be removed in 2.0.0. In 1.11 it said 1.12.0, so it has already been pushed back once. **B. `Map(specId -> outputSpec)`.** The issue suggests this. It avoids the deprecated path and the WARN. But the JVM would then read the void-only manifests with the table's real spec, which no longer matches the field-less spec the native side wrote, and for #5693 resolving that spec can fail once the void field's source column has been dropped. Making B safe means changing how the native side encodes void-only specs, or special-casing them on the JVM side. **I'd go with A here**: it is provably the same behavior on every supported version, and it fixes 1.12. The comment would note the dependency on the deprecated path, and I'd open a follow-up to move to B before Iceberg 2.0, together with the void-only spec question. Two points I'd like input on: - **Validation.** No CI profile covers 1.12, and the test sources don't compile against it, so the PR would carry this kind of out-of-band evidence. Is that acceptable, or would you rather see the test-source fixes and a 1.12 job first? - **Log noise.** With A, users on 1.12 get one deprecation WARN per write task. Is that acceptable for now? Does A sound right? -- 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]
