tlfua opened a new pull request, #7507: URL: https://github.com/apache/datafusion-comet/pull/7507
## Which issue does this PR close? Closes #7240. ## Rationale for this change Iceberg 1.12 removed the 2-arg `ManifestFiles.read(ManifestFile, FileIO)`. `IcebergReflection` resolves that overload in the executor-side reflection probe and in `readDataFilesFromManifest`. On 1.12 the probe now fails with `NoSuchMethodException`, so every native Iceberg write falls back to the JVM writer. In every release that has it (1.5.2 through 1.11), the 2-arg overload just delegates to `read(ManifestFile, FileIO, Map<Integer, PartitionSpec>)` with a `null` map. The 3-arg overload is present in all supported versions, 1.5.2 through 1.12.0. ## What changes are included in this PR? - `IcebergReflection` adds a private `manifestFilesReadMethod()` that resolves the 3-arg `ManifestFiles.read`. The probe and `readDataFilesFromManifest` both use it. The decode passes `null` for the specs map, the same call the 2-arg overload made, so behavior on every currently pinned Iceberg version (1.5.2, 1.8.1, 1.10.0, 1.11.0) is unchanged. - The scaladoc records why the map is `null`: the reader must use the spec embedded in the manifest, which `manifest_partition_spec` in `iceberg_write.rs` writes and which for a void-only V1 spec has no fields. It also records that the `null` path is deprecated upstream and slated for removal in Iceberg 2.0. - Two `IcebergReflectionSuite` tests write a manifest with Iceberg's own `ManifestFiles.write` and decode it through `decodeManifestToDataFiles`. The first uses a partitioned spec with a non-zero spec id; the second uses a spec with no fields. Both run in every existing profile. On the deprecation warning: with a `null` map, Iceberg 1.11 already logs `ManifestReader` "...deprecated and will be removed in the 1.12.0 release". 1.12 logs the same warning but names 2.0.0. The default 1.11 profile hits this path today, so this PR adds no new warning on the pinned versions. Follow-ups, not in this PR: - Before Iceberg 2.0, pass a specs map that agrees with the spec the native writer embeds, instead of `null`. - Add Iceberg 1.12 CI coverage. Today no profile pins 1.12, and Comet's test sources do not compile against it (`CometIcebergWriteDetectionSuite` `getConf` override and `RESTCatalogAdapter` REST API changes), so that needs its own PR. ## How are these changes tested? **Unit tests.** The local command below runs on Spark 4.1 with Iceberg 1.11: ``` ./mvnw -B -Pspark-4.1 test -Dtest=none -Dsuites="org.apache.comet.iceberg.IcebergReflectionSuite" ``` Result: 33 tests succeeded, 0 failed. `spotless:check` passes. **Pinned versions unchanged, fork CI.** All jobs passed in both runs: | Run | Coverage | Result | |---|---|---| | [PR tier](https://github.com/tlfua/datafusion-comet/actions/runs/41974471081) | Default `ci.yml` PR tier | success | | [Iceberg verification](https://github.com/tlfua/datafusion-comet/actions/runs/41974474284) | 12 Comet Iceberg suites on Spark 3.4, 3.5, 4.0, 4.1, 4.2 (Iceberg 1.5.2–1.11.0) | success | | same run | Iceberg Spark SQL tests on Iceberg 1.8, 1.9, 1.10, 1.11 (`iceberg_spark_test_reusable.yml`) | success (32/32 jobs) | The verification workflow lives only on a fork branch and is not part of this PR. **Iceberg 1.12, end to end, fork CI.** The same run has a job per leg. Each leg pins Iceberg 1.12.0 (javap-checked on the runtime jar), compiles Comet main sources, and runs a Spark 4.1 session with Comet native Iceberg writes enabled. It inserts into two tables: - a V2 table evolved to partition spec 1; - a V1 table whose spec 1 is void-only. It then checks the plan, rows, a pruned read, and the `files` metadata (`spec_id`, partitions). The `base` leg runs the same check on `main` (b82f5eaff) without this change. | | `fix` | `base` (without this PR) | |---|---|---| | executor probe | `None` | `NoSuchMethodException: ManifestFiles.read(ManifestFile,FileIO)` | | V2 evolved spec insert | native (`CometIcebergWrite`) | falls back to JVM | | V1 void-only spec insert | native | falls back to JVM | | rows / pruned read / `spec_id` / partitions | correct | correct | javap on `iceberg-spark-runtime-4.1_2.13-1.12.0.jar` lists only: ``` public static ManifestReader<DataFile> read(ManifestFile, FileIO, Map<Integer, PartitionSpec>); static ManifestReader<DataFile> read(ManifestFile, FileIO, Map<Integer, PartitionSpec>, boolean); ``` The issue comment linked from #7240 has the original reproduction. -- 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]
