fix: stop building maps natively when the key type holds a FLOAT or DOUBLE

[andygrove]

Which issue does this PR close?

Part of #6549 and #6385.

Rationale for this change

Native map_from_arrays and map_from_entries tell keys apart by their bits, and they report Compatible for every key type, so they run by default. Spark's ArrayBasedMapBuilder handles keys that hold a FLOAT or DOUBLE differently:

• It finds a duplicate top-level key in a HashMap of boxed values, so NaNs with different bits are one key. For map_from_arrays(array(d, -d), array(1, 2)) over a NaN d, Spark raises DUPLICATED_MAP_KEY and Comet returns a map with two NaN keys.
• It finds a duplicate struct or array key in a TreeMap in Spark's SQL ordering, where -0.0 equals 0.0 and all NaNs are equal. Comet keeps both keys there too.
• From Spark 4.0 it also normalizes a top-level key before the lookup, unless spark.sql.legacy.disableMapKeyNormalization is set. So 0.0 and -0.0 are one key, and map_from_entries, which always rebuilds the map, stores 0.0 for a -0.0 key. Comet keeps -0.0 and accepts both zeros.

The first two also apply on Spark 3.4 and 3.5, which #6549 doesn't describe. I reproduced all three on main with Spark 4.1, and the first two with Spark 3.5.

#6682 implements these rules in the native builders, but it is stacked on #5854. Until then, this falls back for these key types, as #6549 suggests.

What changes are included in this PR?

• MapBuilderKeySupport in maps.scala reports Incompatible when the key type holds a FLOAT or DOUBLE at any depth, on every Spark version, and both CometMapFromArrays and CometMapFromEntries check it. Both then run Spark's code through the codegen dispatcher, as map_from_entries already does for BinaryType keys and LAST_WIN. CometMapFromArrays now mixes in CodegenDispatchFallback for this, so LAST_WIN is dispatched for it too. A float value type still runs natively.
• Docs: a "Map construction" section in the floating-point compatibility guide, the notes for both functions in expressions.md, the contributor guide's rule for map construction, and the map audit, which said 4.0 left their semantics unchanged.

How are these changes tested?

• map_builders_float_key_gate.sql runs on every version. It covers a lone -0.0 key, compared with the running Spark's answer, NaN keys that differ only in the sign bit, and struct and array keys whose floats differ only in sign, plus a float value type that stays native. map_builders_float_key_gate_spark_4_0.sql checks that Spark 4.0+ raises DUPLICATED_MAP_KEY for 0.0 and -0.0. map_builders_float_key_gate_legacy.sql checks that with spark.sql.legacy.disableMapKeyNormalization they stay two keys, which also shows that the dispatcher sees the session's flag.
• map_builders_float_key_last_win.sql runs both builders through the dispatcher under LAST_WIN with a key that really repeats: a string key, NaNs that differ only in the sign bit, and 0.0 with -0.0 in both orders. It compares the kept value and the stored key with Spark's on every version. The two dedup-policy fixtures turn the dispatcher off, so they cover the fallback.
• With the new check disabled, all three gate fixtures fail on Spark 4.1. With CodegenDispatchFallback removed from CometMapFromArrays or from CometMapFromEntries, the LAST_WIN fixture fails on Spark 4.1.
• The 33 CometSqlFileTestSuite tests for the map fixtures pass on Spark 3.4, 3.5, 4.0, 4.1 and 4.2. On 4.1, CometMapExpressionSuite and CometFloatSemanticsSuite pass too (478 tests with the fixtures).

[dwsmith1983]

Hi @andygrove.

The three ArrayBasedMapBuilder behaviors in the description match the Spark source: the HashMap and TreeMap split in 3.5.8, and the keyNormalizer that 4.0.0 added and 4.1.1 still has. Gating on the key type at any depth, on every version, looks right. I also like that the legacy fixture shows the dispatcher picks up the session's spark.sql.legacy.disableMapKeyNormalization.

A few things I'd like to ask about.

Could map_from_arrays use the dispatcher too? In 1.1 a map_from_arrays with a DOUBLE key runs natively. With this PR the whole projection falls back to Spark, while map_from_entries with the same keys stays in the Comet pipeline through the codegen dispatcher. Is there a reason CometMapFromArrays can't mix in CodegenDispatchFallback the way CometMapFromEntries does? The dispatcher never calls the serde's convert, so the CaseWhen null guards shouldn't get in the way, and MapFromArrays.doGenCode goes through mapBuilder.from, so a lone -0.0 key would still come back as -0.0. It would also keep the LAST_WIN case in Comet, which falls back today. I tried it locally on Spark 4.1 with the mixin added. The two expect_fallback queries here and the two in map_from_arrays_dedup_policy.sql pass as expect_dispatch(map_from_arrays), the lone -0.0 key still matches Spark, and all 32 expressions/map/ fixtures pass. The expressions.md row would then read Hybrid.

expect_error_class for the dispatched error queries. The map_from_entries duplicate-key queries raise DUPLICATED_MAP_KEY from Spark's code running inside the dispatcher, so they come from Comet execution. sql-file-tests.md suggests expect_error_class for structured errors from Comet execution, since expect_error also passes when the query falls back and doesn't compare the error class or SQLSTATE. Could the map_from_entries queries in map_builders_float_keys.sql and map_builders_float_keys_spark_4_0.sql use expect_error_class(DUPLICATED_MAP_KEY)? I ran the six of them that way on Spark 4.1 against this branch and they pass. The map_from_arrays ones fall back today, so expect_error fits them unless they move to the dispatcher, in which case they pass as expect_error_class too.

One test checks something other than what its comment says. In map_builders_float_keys.sql, the block under "Struct and array keys compare in Spark's SQL ordering, where -0.0 equals 0.0" ends with map_from_arrays(array(array(d), array(-d)), array(1, 2)) ... WHERE id = 2. Row 2 holds NaN, so that query checks NaN sign bits inside an array key rather than the two zeros. Could it use id = 1 like the others, or get its own comment if NaN is the point?

Backport. On branch-1.1 both serdes report Compatible for these key types, so 1.1 users get a map with two NaN keys where Spark raises DUPLICATED_MAP_KEY. Since this is a correctness fix with a small serde change, I'd suggest backporting it to branch-1.1.

[viirya]

Thanks for tracking down all three ArrayBasedMapBuilder behaviors. I checked them against the 3.4, 3.5, 4.0 and 4.1 sources and they hold, including the from() detail that keeps a lone -0.0 key in map_from_arrays. Gating on the key type at any depth, on every version, is the right call until #6682 lands. The legacy fixture is a nice touch, since it shows the dispatcher sees the session's flag.

+1 to dwsmith's points. The one I'd most like to see addressed is moving CometMapFromArrays onto CodegenDispatchFallback. As it stands, every map_from_arrays with a DOUBLE or FLOAT key drops the whole projection back to Spark on every version, which is a slowdown against 1.1 for queries that never see a NaN or -0.0. The dispatcher would keep those in Comet and make the two builders behave the same. Switching the dispatched error queries to expect_error_class also matters for map_builders_float_keys_spark_4_0.sql in particular. That file only has expect_error queries, so it would still pass if map_from_entries silently fell back to Spark.

I left a few inline comments on top of that. Two are about how this PR and #6682 fit together: both close #6549, and both add fixtures with the same names. The other two are about keeping the float-key explanation in one place, since MapKeySupport in the same file now describes the builder differently.

[viirya]

This PR says Closes #6549, and so does #6682. If this one merges first, #6549 closes while the native builders still compare the keys' bits. This reason string, which ends up on the generated compat page, and the docs would then point users at a closed issue for a difference that is still there. Could the description here say Part of #6549 instead, so the issue stays open until #6682 fixes it natively?

[andygrove]

Changed the PR description to Part of #6549, so the issue stays open until #6682 lands. The reason string keeps its link to #6549, which is still the right place for the open difference.

[viirya]

MapKeySupport at the top of this file already gates float keys with the same containsType check. Its reason at L36-38 and its scaladoc at L54 say Spark normalizes floating-point map keys and that ArrayBasedMapBuilder rewrites -0.0 to +0.0, without saying which versions. Your research in this PR shows that only holds on 4.0+ when spark.sql.legacy.disableMapKeyNormalization is unset. So two objects in the same file now describe the same builder differently. Could you correct that description here? Or fold hasFloatingPointKey and its reason into MapKeySupport, so there is one place that explains how Spark treats float map keys?

[andygrove]

Corrected in ee66473. The MapKeySupport reason and scaladoc no longer say the builder normalizes keys. They now say the lookup uses Spark's SQL ordering, where -0.0 equals 0.0 and all NaNs are equal, on every version, and point at MapBuilderKeySupport for how the builder differs by version. I kept the two objects separate rather than folding them, because one gates lookups and the other gates construction. The builder explanation now lives only in MapBuilderKeySupport.floatingPointReason. Three fixtures and CometMapExpressionSuite matched the old reason text, so I updated those.

[viirya]

Thanks, the new reason reads right. Two spots still carry the old claim. The scaladoc at L51-52 still says the lookup runs over the keys ArrayBasedMapBuilder stored "having first normalized them", and the comment above the first expect_fallback in element_at_map.sql (L59) still says Spark stores -0.0 map keys as +0.0. Both only hold on 4.0+ without the legacy flag. Could you qualify or drop those too?

[andygrove]

Fixed in 2c29974. The MapKeySupport scaladoc now says Spark looks a key up with the interpreted ordering over the map's stored keys, without the normalization claim. The element_at_map.sql comment, and the same sentence in get_map_value.sql and the CometMapExpressionSuite comment, now say only that Spark compares with nanSafeCompareDoubles, so a -0.0 lookup finds the +0.0 key.

[viirya]

This scaladoc repeats the reason string below almost word for word. The same explanation also appears in both audit entries, both floating-point guides, expressions.md, and the headers of the three SQL fixtures. When #6682 lifts this gate, all of them have to change together, and any one that gets missed goes stale. Could the scaladoc just say why the gate lives here and point at floatingPointReason for the details? The new "Map construction" section in the user-guide floating-point.md could also be trimmed to the user-facing outcome, since the reason already feeds the generated compat page for these expressions.

[andygrove]

The scaladoc on MapBuilderKeySupport now says only why the gate lives there and points at floatingPointReason. The "Map construction" section in floating-point.md is down to the user-facing result, with a link to the expression compatibility guide for the reason. The audit entries and the contributor guide rule still describe the builder behavior, since those are the places a reader looks for the audit trail, so they will need updating when #6682 lifts the gate.

[viirya]

#6682 also adds map_builders_float_keys.sql and map_builders_float_keys_legacy.sql in this directory, with different contents. Whichever lands second will collide with the other. Could you rename these, or agree with #6682 on which PR owns the fixtures, so the rebase doesn't silently drop cases from either one?

[andygrove]

Renamed in ee66473 to map_builders_float_key_gate.sql, map_builders_float_key_gate_legacy.sql and map_builders_float_key_gate_spark_4_0.sql, so none of the names match the files in #6682. When #6682 lifts the gate these three should be deleted or folded into its fixtures, since they assert dispatch and the other PR asserts native behavior.

[andygrove]

@dwsmith1983 @viirya, pushed ee66473.

• CometMapFromArrays now mixes in CodegenDispatchFallback, so a FLOAT or DOUBLE key no longer drops the projection to Spark, and LAST_WIN is dispatched too. The expressions.md row now reads Hybrid. map_from_arrays_dedup_policy.sql disables the dispatcher so it still covers a real fallback, as map_from_entries_dedup_policy.sql does.
• The dispatched DUPLICATED_MAP_KEY queries now use expect_error_class, including every query in the Spark 4.0 file. I also added map_from_arrays queries with expect_dispatch to the main and legacy fixtures.
• The array-key zeros query now reads id = 1. The NaN-in-array-key case is its own query with its own comment.
• The 35 map fixtures pass on Spark 3.5, 4.0 and 4.1, and CometMapExpressionSuite passes on 4.1. I did not re-run 3.4, 4.2 or CometFloatSemanticsSuite after this change.
• Fixture names, the Part of #6549 change and the doc trimming are answered in the inline threads.

On the backport to branch-1.1: I'll do it after this merges, per the backporting guide.

[viirya]

Thanks for the quick turnaround. The dispatcher change for map_from_arrays looks right to me. MapFromArrays.doGenCode calls from($keys.copy(), $values.copy()), and the kernel's input ArrayData and struct views are allocated per call and deep-copy into GenericArrayData / GenericInternalRow, so the built map doesn't hold references into the batch's Arrow buffers, and struct or array keys compare as distinct objects. The renamed fixtures, expect_error_class, and the split NaN query cover what dwsmith and I raised. Keeping the lookup gate and the construction gate as separate objects makes sense too. I left two small follow-ups inline.

[viirya]

With the dispatcher off here, nothing now runs the default path for LAST_WIN, which after this PR is the dispatcher, with a key that actually repeats. Could you add an expect_dispatch(map_from_arrays) query under LAST_WIN with the dispatcher on? A DOUBLE key would be the most useful: two NaNs that differ in the sign bit, plus 0.0 and -0.0 gated on 4.0+. A repeated key sends from() through build(), so both the kept value and the stored key get checked. routing_map_legacy_enabled.sql has the same gap for map_from_entries, since its data has no repeated key, so a matching query there would cover both builders.

[andygrove]

Added map_builders_float_key_last_win.sql in 2c29974. It runs map_from_arrays and map_from_entries with the dispatcher on under LAST_WIN and a key that really repeats: a string key, a DOUBLE and a FLOAT pair of NaNs that differ only in the sign bit, and 0.0 with -0.0 in both orders so the stored key is checked too. The zero queries run on every version rather than only 4.0+, since the dispatcher runs Spark's builder either way and the result has to match what the running Spark does. I put it in its own file instead of routing_map_legacy_enabled.sql so one fixture covers both builders, and the two dedup-policy fixtures now point at it. It fails on 4.1 when CodegenDispatchFallback is dropped from either serde, and the map fixtures pass on 3.4, 3.5, 4.0, 4.1 and 4.2.

[viirya]

Thanks for the follow-ups. map_builders_float_key_last_win.sql covers the dispatched LAST_WIN path for both builders with keys that really repeat, and checking 0.0 / -0.0 in both orders against the running Spark pins the stored key on every version. The MapKeySupport scaladoc and the lookup fixtures no longer claim the builder normalizes keys. LGTM once CI is green, and thanks for planning the branch-1.1 backport.