You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Smallfu666 (via GitHub)" <gi...@apache.org> on 2026/09/24 10:40:06 UTC
[PR] fix: fall back to Spark for a TRY cast of a map key that can fail [datafusion-comet]
Smallfu666 opened a new pull request, #6179:
URL: https://github.com/apache/datafusion-comet/pull/6179
## Which issue does this PR close?
Closes #5995.
Stacked on #6178, which has to land first. Until it merges this branch also shows its commit.
## Rationale for this change
`TRY_CAST` between map types whose key cast can fail fails in Comet with
`Found unmasked nulls for non-nullable StructArray field "key"`, where Spark returns a result.
`CometCast.isSupported` decided map support by recursing into the key and value casts, so the
plan was marked `Compatible`.
## What changes are included in this PR?
- Report `Unsupported` for a TRY map cast unless its key cast is on the list of casts that cannot
throw, the same list #6178 adds for the dispatcher. Arrow's map format requires a non-nullable
key field, so no Comet path can hold the result. Spark's own result is not consistent between
readers either: its map holds a null key internally, `map_keys` shows it, and `collect`,
`element_at` and a cast to string read it as `0`.
- The codegen dispatcher then refuses the tree, through #6178, and the plan falls back to Spark.
- Widening key casts, and plain `CAST` in legacy and ANSI mode, stay native.
Without #6178, `Unsupported` would route the cast through the dispatcher, which wrote the null
key as `0` and turned today's error into a wrong result.
One known limitation, which is effectively unreachable. The struct arm of `isSupported` returns
the first field that is not `Compatible`, so a struct whose earlier field is `Incompatible` would
report `Incompatible` even when a later field is this map cast. The only `Incompatible` cast today
is a negative-scale decimal to string with the legacy flag off, and such a decimal cannot be
created in that state. Even then it only matters with `spark.comet.expr.allowIncompatible=true`.
By default an `Incompatible` cast goes to the codegen dispatcher, which refuses the tree through
#6178 and falls back. With the flag set it would run natively and fail with the error above
rather than return a wrong result.
## How are these changes tested?
- `try_cast_map_null_key.sql`: the direct `try_cast`, `map_keys` and `element_at` over a narrowing
key cast, and `map_keys` over a date to timestamp key cast, all fall back. A legacy `CAST` and a
widening TRY cast stay native, asserted with `expect_native`. The direct cast fails on #6178
alone with the error above.
- A unit test in `CometNativeCastSuite` pins the support level across eval modes, for a date key,
and for a map nested in an array and in a map value.
- `CometNativeCastSuite` and `CometCodegenSuite` pass, and `test-compile` passes on the spark-3.4,
3.5, 4.0, 4.1 and 4.2 profiles.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for a TRY cast of a map key that can fail [datafusion-comet]
Posted by "sunchao (via GitHub)" <gi...@apache.org>.
sunchao commented on code in PR #6179:
URL: https://github.com/apache/datafusion-comet/pull/6179#discussion_r4367212296
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -131,6 +131,10 @@ object CometCast
Compatible()
} else if (CometTimeZone.nativeId(cast.timeZoneId).isEmpty) {
CometTimeZone.supportLevel(cast.timeZoneId)
+ } else if (canProduceNullMapKey(cast)) {
Review Comment:
[P2] Preserve native execution for map-key conversions that always succeed. For a `MAP<BOOLEAN,INT>` column, this guard rejects `TRY_CAST(b AS MAP<INT,INT>)`, although both possible keys convert to `0` or `1`. It also rejects default `STRING` to `BINARY` and `ARRAY<INT>` to `ARRAY<BIGINT>` keys. All three ran natively on the base. In the Boolean case, the fallback moved both aggregate stages to Spark and required converting every input row. Extend `keyCastCannotFail` for these proven-safe cases and pin their routing with `expect_native(try_cast)`.
Evidence: On Spark 4.1.3 with `local[1]`, AQE disabled, and one shuffle partition, write `SELECT map(false, CAST(id + 1 AS INT), true, CAST(id AS INT)) AS b FROM range(262144)` to Parquet. Read it as `t` and run `SELECT sum(element_at(try_cast(b AS map<int,int>), 1)) FROM t`. Both SHAs returned `34359607296`. The base used `CometHashAggregate` and converted one result row. The head used Spark `HashAggregate` and converted 262,144 input rows in 32 batches. Across three executions after two warmups, median executor task-thread allocation rose from 4,261,536 to 58,994,104 bytes. The string and array cases reproduced the same plan change. Scalar `INT`-to-`BIGINT` key widening converted one result row on both SHAs.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for a TRY cast of a map key that can fail [datafusion-comet]
Posted by "rich7420 (via GitHub)" <gi...@apache.org>.
rich7420 commented on code in PR #6179:
URL: https://github.com/apache/datafusion-comet/pull/6179#discussion_r4404580448
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -131,6 +131,10 @@ object CometCast
Compatible()
} else if (CometTimeZone.nativeId(cast.timeZoneId).isEmpty) {
CometTimeZone.supportLevel(cast.timeZoneId)
+ } else if (canProduceNullMapKey(cast)) {
Review Comment:
Keep numeric to BOOLEAN and TIMESTAMP to BIGINT map casts native. Neither conversion can produce NULL.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for a TRY cast of a map key that can fail [datafusion-comet]
Posted by "Smallfu666 (via GitHub)" <gi...@apache.org>.
Smallfu666 commented on PR #6179:
URL: https://github.com/apache/datafusion-comet/pull/6179#issuecomment-6395543376
Thanks! These can't fail, so I added boolean to numeric, string and integral to binary, and array or struct keys to `keyCastCannotFail`. A failing element inside an array or struct key only becomes a null element, so the key itself is never null. The boolean, binary and array cases now have `expect_native(try_cast)` coverage.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for a TRY cast of a map key that can fail [datafusion-comet]
Posted by "Smallfu666 (via GitHub)" <gi...@apache.org>.
Smallfu666 commented on code in PR #6179:
URL: https://github.com/apache/datafusion-comet/pull/6179#discussion_r4419343768
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -131,6 +131,10 @@ object CometCast
Compatible()
} else if (CometTimeZone.nativeId(cast.timeZoneId).isEmpty) {
CometTimeZone.supportLevel(cast.timeZoneId)
+ } else if (canProduceNullMapKey(cast)) {
Review Comment:
Thanks, added both, plus timestamp to boolean.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for a TRY cast of a map key that can fail [datafusion-comet]
Posted by "Smallfu666 (via GitHub)" <gi...@apache.org>.
Smallfu666 commented on PR #6179:
URL: https://github.com/apache/datafusion-comet/pull/6179#issuecomment-6383627408
Thanks, good idea! Moved the check into `getSupportLevel` with `canProduceNullMapKey` and dropped the new arm. The test now also covers an earlier `Incompatible` struct field. #6200 itself still applies to other casts. The other `isSupported` callers are `GenerateDocs` (primitive types only) and `CometToPrettyString` (casts to string), so neither can hit a map to map cast.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: fall back to Spark for a TRY cast of a map key that can fail [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #6179:
URL: https://github.com/apache/datafusion-comet/pull/6179#discussion_r4361444300
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -308,6 +308,16 @@ object CometCast
}
}
Compatible()
+ case (from_map: MapType, to_map: MapType)
+ if evalMode == CometEvalMode.TRY && !keyCastCannotFail(
+ from_map.keyType,
+ to_map.keyType) =>
+ // Under TRY a key cast that throws becomes a null key, and Spark keeps the row. Arrow's map
+ // format requires a non-nullable key field, so neither the native cast nor the codegen
+ // dispatcher can hold that result, and `canHandle` refuses the dispatch. Key casts that
+ // cannot throw stay on the native path. Maps nested in the value are checked when the
+ // value cast recurses back into this arm.
+ Unsupported(Some(tryCastNullMapKeyReason))
Review Comment:
`canProduceNullMapKey(cast)` from #6178 already walks map, array and struct types and tests `keyCastCannotFail` at every depth, so this arm adds a second traversal that decides the same question through `isSupported`'s recursion. That recursion is also why #6200 exists: the struct arm returns `other` as soon as a field is not `Compatible`, and the general map arm returns the key's `Incompatible` before it looks at the value, so an earlier `Incompatible` can hide this `Unsupported`. Would it be simpler to check the helper once at the top of `getSupportLevel`, next to the literal and timezone checks, for example `else if (canProduceNullMapKey(cast)) Unsupported(Some(tryCastNullMapKeyReason))` before the `isSupported` branch? That would drop the new arm, keep one definition of "can put a null key in a map", and might remove the masking limitation in #6200. I have not run it and have not checked that every caller of `isSupported` goes through `getSupportLevel`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org