andygrove commented on PR #5262:
URL:
https://github.com/apache/datafusion-comet/pull/5262#issuecomment-5457402136
The `-0.0` handling worries me more than the skipped tests suggest.
`array_intersect` and `array_except` already report `Incompatible` from
`getSupportLevel`, so those only reach the native path under
`allowIncompatible`. But `ArrayDistinct` goes through a plain
`CometScalarFunction` and `ArrayUnion` through `CometArrayUnion`, and neither
overrides `getSupportLevel`, so both run natively by default and now return a
different answer from Spark whenever a float or double array contains `-0.0`. I
checked the branch out and ran it against Spark 4.1:
```
SELECT array_distinct(array(0.0, double('-0.0'), 1.0))
Spark: [0.0, -0.0, 1.0]
Comet: [0.0, 1.0]
```
The plan is `CometProject` with no fallback. SPARK-54918 fixes this in Spark
4.2, but we support 3.4 through 4.1 where nothing will. Could we add a
`getSupportLevel` override on the `ArrayDistinct` and `ArrayUnion` serdes
returning `Incompatible` when the element type is `FloatType` or `DoubleType`,
in the same shape `CometArrayExcept` already uses for its unsupported element
types? That keeps the fast path for every other element type and makes the
divergence opt-in rather than silent. I'd also like a Comet issue that the
`ignore(...)` markers can point at, so the skips are findable from our side and
not only via the Spark JIRA.
Related to that: the comment on the literal case says Spark and Comet agree
there, but the query above *is* the literal case and it fails.
`NormalizeFloatingNumbers` only rewrites grouping keys, join keys, window
partition specs and `Distinct`, not a plain `SELECT`, and the optimized plan
keeps the `-0.0` literal intact. The same "literals are fine, only parquet
columns diverge" framing appears in the `array_except`, `array_intersect` and
`array_union` files too. Worth correcting so nobody re-enables these on the
strength of the comment.
Could we also file an issue for the two ignored `CometAggregateSuite` metric
tests and reference it in the TODO? A prose TODO is easy to lose track of, and
`peak_mem_used` changing under apache/datafusion#22710 seems worth tracking on
our side.
On the off-heap bump in `CometTPCHQuerySuite`, is 2g to 4g because DF 55
genuinely needs more memory for these queries, or is it a consequence of that
same `peak_mem_used` accounting change? If it is the former then it seems worth
a changelog note, since anyone who sized their off-heap against TPC-H would be
under-provisioned after upgrading.
Two smaller things in the shuffle and copy paths. `SpillFile::path()`
returning `Option` now folds two different situations into one `None` in
`SpillWriter::path()`: nothing was spilled, and the backend has no local path.
`local_partition_writer.rs` treats both as "skip the copy", so a pathless spill
backend would silently drop spilled data while the recorded offsets keep
advancing. It cannot happen today since `create_tmp_file` always hands back a
`RefCountedTempFile` and that impl always returns `Some`, but `TempFileFactory`
is pluggable. Would it be worth keeping the two cases apart so a future backend
fails loudly instead? Separately, `copy_array` is the one place the
`try_extend` migration goes back to panicking, via `.expect`. Both callers are
already in `Result` context, so making it return `Result<ArrayRef>` looks cheap
and would turn an offset overflow into a task failure rather than a JVM abort,
which is what arrow deprecated `extend` for.
One correction for the description: `spark_sqrt` is not unrelated to the
bump. DF 55 added `validate_sqrt_input`, so `sqrt(-1)` now errors where DF 54
returned NaN. That is precisely why it has to be in this PR, so it is worth
saying.
--
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]
