andygrove commented on PR #4647:
URL:
https://github.com/apache/datafusion-comet/pull/4647#issuecomment-5441800683
> **Note on this review:** this was generated by an LLM (Claude Code) at my
request while I worked through a review backlog. I have not verified the
individual findings myself. Please treat everything below as suggestions to
evaluate rather than as authoritative review feedback, and push back on
anything that is wrong or already handled.
The rendering matches Spark's `{k -> v, ...}` and reusing `cast_array` for
the keys and values so nested types compose is the right structure. Flipping a
good number of `expect_fallback` fixtures back to plain `query` is a satisfying
diff to read.
Four things.
**Does this honor `spark.sql.legacy.castComplexTypesToString.enabled`?**
`cast_map_to_string` hardcodes `{`, `}`, and `" -> "`. When that legacy
config is on, Spark renders maps with `[` and `]` and omits NULL elements
instead of writing the literal `null`. `CometCast.scala` already has a comment
about only supporting the non-legacy behavior for structs and arrays, so
presumably the same guard needs to cover `MapType -> StringType`.
Is the map case gated by that check today? If not, a user with the legacy
config gets a silently different string. If it is gated, a test with the config
on asserting a fallback would pin it.
**Hidden entries outside a slice or under a null row**
The function casts the entire `keys()` and `values()` children up front,
then indexes them with the row offsets. For a sliced `MapArray`, or one with
null rows, the children contain entries that the visible rows never reference.
Under a throwing eval mode, an ANSI cast failure on one of those hidden values
would fail the query even though Spark never evaluates it.
This is the same class of problem #5457 is fixing for nested casts
generally. Is it reachable here? If the fix lands in
`prepare_nested_cast_input` at the top of `cast_array`, it may cover this for
free, but it would be worth confirming rather than assuming.
**`NullType` is now admitted to the codegen dispatcher globally**
```scala
def isSupportedDataType(dt: DataType): Boolean = dt match {
case NullType => true
```
That is in `CometBatchKernelCodegen`, so it admits `NullType` for every
dispatched expression, not just this cast. The supporting changes generate the
literal `null` from `elementGetterCall` and `emitSpecializedGetterExpr`, which
compiles where an `Object` is expected and does not where a primitive is.
Is there an expression where a `NullType` child would land in a primitive
context and produce uncompilable Java? A test that dispatches some other
expression over a `NullType` column would be worth having, since the failure
mode is a Janino compile error at runtime rather than something CI catches.
**The propagation test was inverted**
`cast MapType propagates Unsupported from nested value cast` existed
specifically to prove that an unsupported nested cast is not silently swallowed
by the outer `Map` arm. It has been rewritten to assert `Compatible` for the
now-supported case.
That is correct, but it means the original property is no longer covered.
Could you add back a case with a genuinely unsupported nested type, for example
`Map<Int, Binary>` to string, asserting that the `Unsupported` still propagates
through the outer arm? Otherwise a future change that swallows the inner result
would go unnoticed.
--
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]
