andygrove commented on issue #6399: URL: https://github.com/apache/datafusion-comet/issues/6399#issuecomment-5936722786
Phase 3, dependency upgrades: I reviewed the upstream changes between the versions in 1.0.0 and 1.1.0-rc1, starting from the code Comet calls on default paths. That covers DataFusion and datafusion-spark 54.1 to 55.1, arrow and parquet 58.4 to 59.3, opendal 0.57 to 0.58.2, and the 154 iceberg-rust commits from `3d84c81` to `bb1e4a4`. The reproducers were run on 1.0.0 and rc1 builds. This completes Phase 3. Two regressions that ship in 1.1.0 are confirmed, both from DataFusion 55, and #6402 has the details: - #5701: `array_distinct` and `array_union` now treat `-0.0` and `0.0` as one value and return `0.0`. Spark versions without SPARK-54918 keep them apart: 3.4, 3.5, 4.0 before 4.0.5 and 4.1 before 4.1.4. 1.0.0 matched Spark. - #6254: a native final hash aggregate that has spilled can't spill again while it reads the spill back, so a task near its memory limit fails with `Failed to acquire N bytes`. At 96m of off-heap memory, rc1 failed every run and 1.0.0 passed every run. Both were already filed, so I added the `regression` label and the evidence to each. Their workarounds are in the draft release notes (#6469). No regression was found in arrow, parquet, opendal or iceberg-rust. A few upstream changes improve on 1.0.0. Comparisons now treat `-0.0` as equal to `0.0`, as Spark does. `map_from_entries` raises Spark's `DUPLICATED_MAP_KEY` error. A Date32 to `TIMESTAMP_NTZ` overflow now errors as it does in Spark. The DataFusion review wasn't exhaustive: it didn't cover the sort-merge join and sort spill changes, row-based group values, or several string kernels. One gap is worth checking before rc2. Native Iceberg reads from S3, GCS or Azure now depend on opendal 0.58 installing its HTTP transport when the native library loads. That works in local runs, but no CI job reads from a real object store, so a quick S3 smoke test on the rc2 build would cover it. Why review missed these: - #5262 switched the `array_distinct` and `array_union` signed-zero tests to `ignore` to get the upgrade through, so a behavior change from 1.0.0 went in as a skipped test rather than a fallback. - #6254 needs a final aggregate that spills under a tight budget, and no CI test runs one. -- 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]
