dwsmith1983 commented on PR #6992: URL: https://github.com/apache/datafusion-comet/pull/6992#issuecomment-6174850125
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. -- 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]
