peterxcli opened a new pull request, #5854: URL: https://github.com/apache/datafusion-comet/pull/5854
## Which issue does this PR close? Closes #4680. ## Rationale for this change Spark builds every map through `ArrayBasedMapBuilder`, which rejects a `NULL` key outright and resolves duplicate keys by `spark.sql.mapKeyDedupPolicy`. Comet's `map_from_arrays` and `map_from_entries` did neither: a `NULL` element inside the keys array produced a map with a `NULL` key instead of raising `NULL_MAP_KEY`, and `LAST_WIN` fell the whole expression back to Spark. DataFusion 55 supplies the missing half. `datafusion.spark.map_key_dedup_policy` (`datafusion_common::config::SparkOptions`) takes the same `EXCEPTION` / `LAST_WIN` values as the Spark config, and the `datafusion-spark` map kernels follow it. Wiring Comet to it turns the `LAST_WIN` fallback into native execution, and the remaining `ArrayBasedMapBuilder` checks are cheap to add on top rather than declining the expression. ## What changes are included in this PR? **Config plumbing.** `spark.sql.mapKeyDedupPolicy` now crosses JNI explicitly in `CometExecIterator.serializeCometSQLConfs` (`cometSqlConfs` only carries `spark.comet.*`), and `prepare_datafusion_session_context` sets it as `datafusion.spark.map_key_dedup_policy`. `create_scalar_function_expr` was handing every `ScalarFunctionExpr` a fresh `ConfigOptions::default()`, so a kernel reading a session setting could never have seen it. It now passes the session's `ConfigOptions`. **Native wrappers** (`native/spark-expr/src/map_funcs/map_builders.rs`). `SparkMapFromArrays`, `SparkMapFromEntries` and `SparkStrToMap` delegate to the `datafusion-spark` kernels, add the checks those kernels do not perform, and restate their errors as the Spark error classes `SparkErrorConverter` converts back into `QueryExecutionErrors`: - a `NULL` key raises `NULL_MAP_KEY`, ahead of any duplicate-key check, matching the order Spark applies them in; - key and value arrays of different lengths raise `MAP_KEY_VALUE_DIFF_SIZES`; - a duplicate key under `EXCEPTION` raises `DUPLICATED_MAP_KEY` naming the key. `str_to_map` needs only the last of these, since splitting a string cannot produce a `NULL` key. Its `LAST_WIN` case went from erroring to correct once the config flowed. **Serde.** `CometMapFromArrays` emits `map_from_arrays` instead of the generic `map` wrapped in `CaseWhen(IsNotNull(left) AND IsNotNull(right), ...)`; the Spark kernel is null intolerant the same way, so the guard is redundant. The `LAST_WIN` `Incompatible` branch is gone from both serdes. **Known difference.** `ArrayBasedMapBuilder` normalizes a floating-point key before storing it (`-0.0` becomes `+0.0`, every `NaN` collapses to one), while the native builders compare the raw Arrow values, so a map built from both `-0.0` and `+0.0` keeps two entries where Spark reports a duplicate key. Documented as a compatible note, and `spark.comet.exec.strictFloatingPoint` declines a floating-point key type for users who want the guarantee. ## How are these changes tested? | Check | Result | | --- | --- | | `cargo test -p datafusion-comet-spark-expr --lib map_funcs` | 21/21 | | `CometMapExpressionSuite` | 30/30 | | `CometSqlFileTestSuite` (map fixtures) | 23/23 | | `CometSqlFileTestSuite` (all categories) | 487/487 | | `cargo fmt --all -- --check`, `cargo clippy` | clean | New coverage: - 21 native unit tests over the wrappers, including two that pin the upstream duplicate-key message wording the key extraction parses, so an upstream rewording fails there rather than silently downgrading the error to a generic execution failure. - Seven `CometMapExpressionSuite` tests asserting exception type, error class and SQLSTATE parity with Spark for the `NULL` key, duplicate key and length-mismatch cases, and answer parity under `LAST_WIN`. - The two `*_dedup_policy.sql` fixtures asserted the `LAST_WIN` fallback and now assert native execution; `map_from_arrays.sql`, `map_from_entries.sql` and `str_to_map.sql` gained the `EXCEPTION`-mode error cases, and `str_to_map_dedup_policy.sql` is new. This retires the `TODO: Add LAST_WIN policy tests when spark.sql.mapKeyDedupPolicy config is supported` in `str_to_map.sql`. The length-mismatch test uses answer-parity rather than naming the error condition: Spark still reports it through a `_LEGACY_ERROR_TEMP_*` condition whose number moves between versions. `CometTestBase.checkSparkError` is refactored onto a new `checkSparkErrorParity` for that. The full 487-fixture run is the regression net for the `ConfigOptions` change, which touches every scalar function rather than just the map builders. -- 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]
