andygrove commented on code in PR #5595:
URL: https://github.com/apache/datafusion-comet/pull/5595#discussion_r3905759083
##########
spark/src/main/scala/org/apache/comet/serde/datetime.scala:
##########
@@ -324,12 +323,7 @@ object CometUnixTimestamp extends
CometExpressionSerde[UnixTimestamp] {
expr: UnixTimestamp,
inputs: Seq[Attribute],
binding: Boolean): Option[ExprOuterClass.Expr] = {
- if (!isSupportedInputType(expr)) {
- val inputType = expr.children.head.dataType
- withFallbackReason(expr, s"unix_timestamp does not support input type:
$inputType")
- return None
- }
-
+ // getSupportLevel reports an unsupported input type before reaching here,
so no re-check.
Review Comment:
Good catch, and it reproduces. `getSupportLevel` checked collation first, so
`unix_timestamp(s, 'yyyy-MM-dd HH:mm:ss' COLLATE UTF8_LCASE)` over a plain
`STRING` column returned `Incompatible` before it ever looked at the input
type, and with `allowIncompatible=true` that goes straight to `convert` — which
no longer has the guard. Running it on this branch gives `CometNativeException:
unix_timestamp does not support input type: Utf8` instead of a fallback.
Fixed in 559c0f9 by screening the input type ahead of the collation check: a
non-date/timestamp input has no native path at all, so it should report
`Unsupported` regardless of collation, and `allowIncompatible` should never be
able to reach it. Collation still reports `Incompatible` for the input types
that do have a native path, so the opt-in behavior there is unchanged. Added
the live-string/collated-format case to `CometTemporalExpressionSuite` — it
fails on the parent commit with the native error above and passes with the fix.
--
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]
