andygrove commented on PR #5262: URL: https://github.com/apache/datafusion-comet/pull/5262#issuecomment-5552636402
I have merged the latest `main` into this branch and fixed the two Iceberg failures, so the remaining review items are now tracked rather than outstanding. Summary of where things stand. **Upmerge.** `main` at 93c3234 merged in as 12540de. Only two files conflicted, both in the shuffle write path, and both were additive on each side: - `buf_batch_writer.rs`: #5568 added the caller-owned scratch buffer and its debug-only `scratch_addr` identity check, while this branch added `total_bytes_written` because DataFusion 55's `Box<dyn SpillWriter>` is not `Seek`. Both fields kept. - `spill.rs`: `recycled_buffer` is threaded through `write`/`flush`, and the spilled-bytes count comes from `bytes_written()` instead of the `writer_stream_position()` delta DataFusion 55 can no longer take. `shared_buffer_spill_metrics_do_not_depend_on_input_batching` from #5628 covers exactly that accounting and passes, along with the rest of the 118 native shuffle tests. **Iceberg (2d1e7bf).** The 10 failures across the two Iceberg jobs had one root cause: partition metadata was derived in three places that could disagree. `schemaWithRequiredFields` tested whether a required field id was already present using `buildFieldIdMapping`, which walks only `Schema.columns()`. A partition source nested inside a struct therefore looked missing, and `findField`, which does descend, resolved it and appended it a second time as a top-level column. That is the `Multiple entries with same key: 4=nested.b and 4=b` failure in `TestPartitionValues`. Presence is now tested with `findField`. Separately, a partition field whose source column was dropped resolves to Iceberg's `unknown` type. Those were filtered out of the serialized partition type and values, but not out of the spec, which came straight from `PartitionSpecParser.toJson`, nor out of the source ids unioned into the task schema. iceberg-rust rejects a task whose partition value count differs from its spec's field count, and a stale source id resolved out of schema history could reintroduce a historical column that collides by name with a re-added one, which is `testReaddColumnAfterIdentityPartitionDrop` failing with `Invalid schema: multiple fields for name category: 4 and 2`. The spec, partition type, partition values and required source ids now all come from one `IcebergReflection.livePartitionFields` list. The spec is filtered positionally with its spec id preserved, which also subsumes the previous special case for value-less tasks: with no live fields the filter yields the same empty-fields spec that case built by hand. @sunchao, this also resolves the allocation you measured on the unconditional Iceberg helper. The old code built a field-id map for every task even when nothing was required; presence is now tested per required id, so an empty required set does no work. Three regression tests in `CometIcebergNativeSuite` cover it: a table partitioned by a nested source column, a v1 table with a live and a dropped partition field, and a v1 table where a dropped partition source column is re-added under the same name. Each fails with the original error when the fix is reverted. One note in case it saves someone time: Iceberg cannot write new files under a spec whose partition source column is gone, since it has no accessor for it, so the mixed live/dropped case only ever arises when reading files written before the drop, and the test is built that way. **Follow-ups.** Everything else raised in review is filed under #5709 so it is not lost when this merges: - #5701 `array_distinct` / `array_union` and `-0.0` - #5707 nested Parquet cast retention accepting an ambiguous case-insensitive match - #5703 the two ignored `CometAggregateSuite` metric tests - #5704 whether the TPC-H off-heap bump is a real requirement - #5705 `SpillWriter::path()` conflating two cases - #5706 `copy_array` panicking on offset overflow - #5708 slicing arrays before normalizing - #5702 the misleading signed-zero fixture comments Two things on #5701 that change the shape of that fix, since the discussion above assumed a clean version split. SPARK-54918's fix versions are 4.2.0, **4.1.4 and 4.0.5**, so it was backported to maintenance releases. The build pins 4.0.4 and 4.1.3 but Comet runs against any patch in those lines, which means a compile-time shim keyed on `shims.minorVerSrc` cannot express the split and a runtime check is needed. Also, `CometSparkSessionExtensions.isSparkNNPlus` compares `SPARK_VERSION` as a string, so `>= "4.0.5"` would silently break at 4.0.10. Both are written up in the issue. I have also refreshed the PR description, which still described the branch as blocked on unpublished crates and called `spark_sqrt` unrelated to the bump. It is required by it: DataFusion 55 added `validate_sqrt_input`, so `sqrt(-1)` now errors where 54 returned NaN. @ziting-openai's threads were withdrawn as the output of a misconfigured agent. Three are resolved. The fourth, on `CometIcebergNativeScan.scala`, described a real problem that @ajsquared independently confirmed, and is fixed by 2d1e7bf, so it can be resolved now too. CI is running on 2d1e7bf. -- 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]
