andygrove opened a new pull request, #6682:
URL: https://github.com/apache/datafusion-comet/pull/6682
**Stacked on #5854.** The first commits are #5854's; review 41ce5c61a. I'll
rebase onto main and mark this ready once #5854 lands. cc @peterxcli, since
this builds on your #5854.
## Which issue does this PR close?
Closes #6549.
## Rationale for this change
Spark's `ArrayBasedMapBuilder` finds duplicate keys in a `HashMap` of boxed
keys. `Double.equals` and `Float.equals` compare `doubleToLongBits` and
`floatToIntBits`, so every NaN is one key while `-0.0` and `0.0` are two, and
the map keeps each key as it first occurred. From Spark 4.0 the builder also
normalizes a `FLOAT` or `DOUBLE` key before it looks it up, unless
`spark.sql.legacy.disableMapKeyNormalization` is set, and stores the normalized
key. `ArrayBasedMapBuilder.from` is the one exception to the storing: when no
key repeats, it returns its input arrays untouched, so `map_from_arrays` keeps
a `-0.0` key and `map_from_entries` does not. I checked `ArrayBasedMapBuilder`
at 3.4.3, 3.5.9, 4.0.4, 4.1.3 and 4.2.0. 3.4 and 3.5 are identical, and 4.0
through 4.2 differ only in how they compare the policy string.
The `datafusion-spark` kernels behind #5854's wrappers find duplicates by
comparing `ScalarValue`s, which compare floats by their bits. So on every Spark
version they kept two NaNs with different bits apart, where Spark raises
`DUPLICATED_MAP_KEY` or keeps one entry under `LAST_WIN`. On 4.0+ they also
kept `0.0` and `-0.0` apart, and `map_from_entries` returned `-0.0` where Spark
returns `0.0`. #5854 documents this as a compatibility note, and this PR
removes the difference.
## What changes are included in this PR?
- **Native:** `SparkMapFromArrays` and `SparkMapFromEntries` take a
`MapFloatKeys` rule, `Boxed` or `Normalized`, registered under their own names.
For a `FLOAT` or `DOUBLE` key, the wrapper hands the kernel the keys as Spark
compares them: `canonicalize_nans`, new in `float_semantics` next to
`normalize_floats`, or `normalize_floats`. It then puts back the keys Spark
stores. Under `Boxed` that is each key as it first occurred, bits included.
Under `Normalized` it is the normalized key, except for a `map_from_arrays` row
with no repeated key. A `DUPLICATED_MAP_KEY` names the repeated key with
`Double.toString` / `Float.toString` (`-0.0`, `1.0`), as Spark does, where the
kernel wrote `1`. All of this applies only to float key types. When no key's
bits change (no non-canonical NaN, and no `-0.0` under normalization), the cost
is one pass over the keys and a byte comparison per batch.
- **Serde:** `MapBuilderSupport.nativeFunction` keeps the version policy in
Scala, as #6385 proposes. It picks `map_from_arrays_normalized_keys` /
`map_from_entries_normalized_keys` on Spark 4.0+ unless the legacy flag is set,
and the plain names otherwise.
- **Strict mode:** a top-level float key no longer makes the builders
`Incompatible` under `spark.comet.exec.strictFloatingPoint`, since nothing
differs any more. A struct or array key holding a float still declines, as in
#5854.
- **Docs:** the map_funcs audit and `expressions.md`.
`map(k1, v1, ...)` (`CreateMap`) and `map_concat` already run through the
codegen dispatcher, so they follow Spark and are unchanged.
## How are these changes tested?
- **Rust:** 10 new unit tests in `map_builders.rs` cover the following:
- NaNs merging and zeros staying apart under `Boxed`, with the first
occurrence's bits kept;
- `0.0`/`-0.0` collisions under `Normalized`, named `-0.0`;
- a batch whose `map_from_arrays` rows keep or normalize their keys
depending on whether a key repeats;
- `map_from_entries` storage under both rules;
- NULL rows, a sliced list, an all-scalar call, and the duplicate naming
for `DOUBLE` and `FLOAT`, including one reported ahead of a `NULL` key.
- **SQL fixtures:** four new files. `map_builders_float_keys.sql` and
`map_builders_float_keys_last_win.sql` are gated on 4.0+. They cover the #6549
repros (`map_from_entries(arrays_zip(k, v))` and the `explode(map_keys(...))`
one), `DUPLICATED_MAP_KEY` for both builders, with the key pinned in the
message, and a NaN with the sign bit set from negating a stored NaN.
`map_builders_float_keys_legacy.sql` and
`map_builders_float_keys_legacy_last_win.sql` set the legacy flag. They run on
every version, since 3.4 and 3.5 have no such flag and behave the same way
without it. `map_builders_strict_fp.sql` now expects all four queries to run
natively.
- **`CometFloatSemanticsSuite`:** #5854's two #6549 known gaps are gone.
They are strict xfails, so their ten cases now have to match.
- **Mutation:** with the native rewrite disabled, all four new fixtures and
those ten cases fail.
- **Results:**
- Spark 4.1: `CometFloatSemanticsSuite`, `CometMapExpressionSuite` and the
`expressions/map/` fixtures passed 505 tests.
- Spark 3.5: the same suites passed 500. The 5 canceled are existing Spark
4-only cases.
- Rust: all 1127 `spark-expr` tests passed, workspace clippy
(`--all-targets -D warnings`) passed, and `cargo fmt` is clean.
- 3.4, 4.0 and 4.2 were not run locally. I'll add `run-all-spark-profiles`
when this comes out of draft.
--
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]
