jordepic commented on PR #5361: URL: https://github.com/apache/datafusion-comet/pull/5361#issuecomment-5479675952
Thanks Andy — all five points addressed. Point by point: **1. Divergence classification + stats safety** Restructured the divergence section of `iceberg-writes.md` into three explicitly classified groups: *physical file layout only* (footer metadata, schema element naming, `created_by`, page CRCs/header stats, dictionary labeling, row-group/rolling cadence, compressed bytes — no reader bases a decision on any of them), *manifest metadata visible to later readers*, and one *operational caveat* (unescaped partition paths). Your suspicion about the stats is right, and the doc now says so explicitly: the manifest-visible class is deliberately almost empty because `DataFile` metrics are not taken from the native writer at all — they are re-derived on the JVM from each written file's parquet footer through iceberg-java's own `ParquetUtil.footerMetrics` + `MetricsConfig.forTable`, so bound truncation, null-count conventions, and metrics modes are iceberg-java's code making iceberg-java's decisions (and the parity suite compares committed manifests against JVM-written ones). Exactly two footer-derived values can differ from what iceberg-java's *writer-tracked* state would have recorded, and both are analyzed in the doc: zero-sign bound normalization (a strictly conservative widening, cannot change pruning) and the Iceberg 1.10+ float/double-under-nullable-struct counts (value and null counts inflate by the same amount, so null-based pruning is unchanged). **2. Reflection resolution** Tested against Iceberg 1.5.2 (spark-3.4 profile), 1.8.1 (spark-3.5), 1.10.0 (spark-4.0), and 1.11.0 (spark-4.1) — the version-sensitive spots (two ambiguous 6-arg `FieldMetrics` constructors on 1.10+, the `GenericManifestFile` constructor arity change at 1.6, `sort_order_id` stamping change at 1.11) are each handled and pinned by tests. Your worry was legitimate, though: the plan-time gate resolved everything *it* used, but the commit-message assembly that runs on executors (manifest decode, the footer-metrics rebuild, `TaskCommit` construction) resolved its reflection lazily on first use — a method moved there would have been a task failure after the data files were already written. Fixed in this pass: the eligibility gate now eagerly probes the entire executor-side reflection surface (every class, method, and constructor) on the driver at plan time, so an Iceberg release that moves any of it becomes a clean fallback with a reported reason. A new test in `IcebergReflectionSuite` pins that the probe resolves on every CI profile, so an Iceberg version bump that breaks the surface fails loudly in CI rather than silently falling every write back. I don't have permission to apply the `run-iceberg-tests` label — could you add it? **3. Performance numbers** Added to the description. Summary (2M rows x 100 mixed-type columns, ~1.5 GB, Spark 4.0.1 / Iceberg 1.10.0, local[*], medians of 3): an unpartitioned `INSERT INTO ... SELECT` goes 9.5 s → 3.3 s when the native writer replaces the Java writer under an identical Comet scan (2.9x on the write side); a `bucket(8, int)` partitioned append with fanout goes 24.1 s → 4.4 s (5.5x). One honest caveat is in the description: with Iceberg's default hash distribution, the exchange partitions by Iceberg's `BucketFunction`, which Comet doesn't run natively, so a default-distribution bucketed append currently declines (fully-Comet-native-input rule) — the partitioned numbers use `write.distribution-mode=none` + fanout. **4. Failure mode** Now stated in a "Failure handling" section in `iceberg-writes.md`. The short version is close to your suggested answer, with one honest caveat: - Eligibility is decided entirely at plan time; the physical plan is fixed, so a failed task retries through the native writer again (never the Java writer). Retries cannot collide — the task attempt id is embedded in file names. - Partial results are never committed: the commit set is exactly the commit messages of successful tasks, and a job failure aborts before commit. - The caveat: data files finalized by a failed attempt are *not* deleted by that task — iceberg-java's `DataWriter.abort` deletes them, and the native path has no abort hook yet. They're invisible to all readers (files resolve through committed manifests) and are reclaimed by the normal `remove_orphan_files` maintenance. A task-abort cleanup hook is a reasonable follow-up. - Driver-side commit failure behaves exactly as stock: the commit messages are genuine `SparkWrite$TaskCommit`s, so `SparkWrite.abort`'s cleanup applies unchanged. **5. #5318 cross-reference** Done — the CoW MERGE fallback pin now points at #5318 and says what to flip when it lands. -- 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]
