andygrove commented on PR #5302:
URL:
https://github.com/apache/datafusion-comet/pull/5302#issuecomment-5441663110
> **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 analysis in the description is excellent. "The right answer fell out of
an accident of pattern matching rather than a check anyone wrote" is exactly
the situation, and moving the check above the `fromType == toType` shortcut so
identity casts on collated types are caught too is the detail that makes this
actually complete. Adding the new suite to both the Linux and macOS workflows
is the right thing to do.
Two things I would like to understand.
**Where does the cast actually go now?**
`Unsupported` routes through the JVM codegen dispatcher rather than straight
to Spark row execution. The dispatcher builds Arrow vectors for its inputs and
outputs, and Arrow has no notion of Spark collation either. If a collated
`STRING` reaches the dispatcher, does it hit the same silent metadata loss this
PR is fixing, just one layer down?
`CometBatchKernelCodegen.canHandle` would be the place to check. If collated
strings are already rejected there, a sentence in the comment saying so would
close the loop. If they are not, this fix moves the problem rather than
removing it, and the dispatcher needs the same guard.
**The fallback reason is generic**
`unsupported(fromType, toType)` produces the standard "Cast from X to Y is
not supported" message. For a user whose query slowed down because one column
has `COLLATE UTF8_LCASE`, that message gives no hint that collation is the
cause, and both types will print as `string` in the message, making it look
like a nonsensical rejection of a string-to-string cast.
Could this return `Unsupported(Some("Cast involving a non-default string
collation is not supported
(https://github.com/apache/datafusion-comet/issues/4489)"))` instead? That is
the string users will see in `EXPLAIN` output, and it is the difference between
a confusing fallback and an actionable one.
**One smaller note**
`hasNonDefaultStringCollation` now walks nested element, key, value, and
field types on every cast support check, for every cast in every plan, on every
Spark version including 3.x where it always returns false. Is the 3.x shim
short-circuiting before the walk, or does it recurse and then return false?
Planning-time cost is usually irrelevant, but `isSupported` is called a lot and
deeply nested schemas are not rare.
--
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]
