comphead opened a new pull request, #6211:
URL: https://github.com/apache/datafusion-comet/pull/6211
## Which issue does this PR close?
Closes #4680.
## Rationale for this change
Spark builds `map_from_arrays` and `map_from_entries` through
`ArrayBasedMapBuilder`, which checks each row in order: the key and value
counts, then each key for NULL (`NULL_MAP_KEY`) or a repeat
(`DUPLICATED_MAP_KEY` under the default
`spark.sql.mapKeyDedupPolicy=EXCEPTION`). On `main`:
- `map_from_arrays` runs DataFusion's `map`, which fails on NULL and
duplicate keys with its own messages ("map key cannot be null", "map key must
be unique ...") rather than Spark's exceptions, and compares key and value
lengths per batch rather than per row.
- `map_from_entries` runs `datafusion-spark`'s kernel, which never checks
for a NULL key (Arrow then rejects the non-nullable key field), reports
duplicates as an untyped DataFusion error, and misreads a sliced input. It
builds its key mask from `keys_offsets[0]` but applies it from index 0 of the
unsliced child, so an earlier row's keys can be paired with a later row's
values.
- Keys that are equal only under a non-default collation are not detected as
duplicates, so on Spark 4 the `UTF8_LCASE` keys `a` and `A` build two entries
where Spark raises `DUPLICATED_MAP_KEY`.
Related open PRs touch the same code. #5854 also closes #4680, by making
`LAST_WIN` native through `datafusion.spark.map_key_dedup_policy`. #5846 adds
per-row length checks for `map_from_arrays`, and #5844 and #5867 change how
`map_from_arrays` is routed. This PR keeps `LAST_WIN` on its existing path, see
below.
## What changes are included in this PR?
- `native/spark-expr/src/map_funcs/map_builder.rs` adds `SparkMapFromArrays`
and `SparkMapFromEntries`, which share `build_map`.
- Each non-NULL row is checked in Spark's order and fails with
`SparkError::{MapKeyValueDiffSizes, NullMapKey, DuplicatedMapKey}`, which the
existing shims turn into Spark's own exceptions.
- Keys are compared by their Arrow row encoding in a per-row hash set.
- A row holding a NULL entry makes the `map_from_entries` result NULL
before its keys are checked, as in Spark.
- The key and value children are reused zero-copy unless a NULL row spans
some of their entries or their C Data export would carry an offset. Arrow
Java's import ignores a nested child's offset, which a sliced boolean child
keeps (#2051), so those children are copied with `take`.
- `CometMapFromArrays` now calls `map_from_arrays` inside its existing
`CaseWhen` null guard. The `datafusion-spark` `MapFromEntries` registration is
replaced by the Comet UDF of the same name.
- `MapBuilderSupport` in `maps.scala` is the shared gate.
- `LAST_WIN` stays `Incompatible`, so `map_from_arrays` falls back to
Spark and `map_from_entries` uses the codegen dispatcher, as before.
- Keys with a non-default collation become `Incompatible`.
- Floating-point keys become `Incompatible` under
`spark.comet.exec.strictFloatingPoint=true`.
- The compatibility note saying NULL keys were not detected is removed.
- Docs: a "Map keys" section in the floating-point compatibility guide,
notes in `expressions.md`, and corrected audit entries in `map_funcs.md`. Comet
compares floating-point keys by their bits. Spark treats every NaN key as one
key, and `-0.0` as `0.0` in nested keys and, from Spark 4.0, in top-level keys.
Left out on purpose, possible follow-ups:
- Native `LAST_WIN`. The `CaseWhen` guard evaluates its children twice
(#5781), and `LAST_WIN` currently falls back correctly.
- Spark-style normalization of floating-point keys. The existing
`normalize_nested_floats` could cover nested keys.
- `str_to_map` under `LAST_WIN`, which reads DataFusion's default policy
(inferred from the code, not run).
- Normalizing nested child offsets once at the JNI export boundary instead
of in each kernel.
## How are these changes tested?
- SQL fixtures:
- `map_from_arrays.sql` and `map_from_entries.sql` cover distinct keys of
many types (int boundaries, doubles with NaN and infinities, multibyte strings,
structs with NULL fields, boolean, tinyint, smallint, bigint, float,
decimal(38, 18), date, timestamp, timestamp_ntz, binary and array keys), NULL
arrays and NULL entries, `NULL_MAP_KEY` and `DUPLICATED_MAP_KEY` in both check
orders, per-row length mismatches whose totals add up across rows, and repeated
struct keys.
- `map_from_arrays_collation.sql` and `map_from_arrays_strict_fp.sql`
cover the new gates.
- `routing_maps_{enabled,disabled,opt_in}.sql` and
`routing_collection_collation_{enabled,disabled}.sql` cover `map_from_entries`
routing for floating-point keys under strict mode and for collated keys.
- `CometMapExpressionSuite` adds `map constructors on a sliced batch read
the visible rows`, which feeds both constructors a batch sliced by a native
`OFFSET`, with boolean children. It is a Scala test because
`CometSqlFileTestSuite` excludes `ConstantFolding`, which leaves `LIMIT ...
OFFSET` unplannable.
- A Rust unit test in `map_builder.rs` covers a NULL values row spanning
keys on a sliced input with boolean values. Comet's serde never passes such a
row, so SQL cannot reach it.
Ran locally on the default Spark 4.1 profile: `CometSqlFileTestSuite
expressions/map/` (30 passed), the new `CometMapExpressionSuite` test, and
`CometSqlFileTestSuite routing_collection_collation` (2 passed). The other
Spark profiles and the Rust unit test were not run locally.
--
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]
