andygrove opened a new issue, #6080:
URL: https://github.com/apache/datafusion-comet/issues/6080
### Describe the bug
`CometToUnixTimestamp` and `CometMakeTimestamp` both extend
`CometCodegenDispatch`, whose `convert` unconditionally emits
`CometScalaUDF.emitJvmCodegenDispatch`
(`spark/src/main/scala/org/apache/comet/serde/CometScalaUDF.scala:202-211`).
Neither expression has a native kernel at all. Both nevertheless return
`Incompatible` when an argument carries a non-default collation:
- `CometMakeTimestamp`,
`spark/src/main/scala/org/apache/comet/serde/datetime.scala:867-883`
- `CometToUnixTimestamp`,
`spark/src/main/scala/org/apache/comet/serde/datetime.scala:895-911`
That `Incompatible` has two user-visible effects, and both of them promise a
native implementation that does not exist.
In `EXPLAIN`, the `Incompatible` arm of `QueryPlanSerde.exprToProtoInternal`
tags the node with `withInfo(expr, NativeOptIn.message(...))` before routing it
through the dispatcher, so the user is told to set a flag that changes nothing.
In the generated compatibility guide, `getIncompatibleReasons()` is rendered
under the `nativeOptIn` header from `GenerateDocs.renderExpressionCompatNotes`:
"By default, `X` is evaluated in the JVM using Spark's own code-generated
implementation (run inside the Comet pipeline), which matches Spark exactly.
Set `...allowIncompatible=true` to opt into Comet's native implementation
instead, which has the following differences from Spark:". There is no native
implementation to opt into.
### Steps to reproduce
On Spark 4.0+, over a Parquet table `t(_1 string, _2 string, _3 string)`
holding `('2024-06-15', 'yyyy-MM-dd', 'America/Los_Angeles')`, with
`ConstantFolding` excluded, run each query twice, once with the expression's
`allowIncompatible` at `false` and once at `true`:
```sql
SELECT to_unix_timestamp(_1, _2 COLLATE UTF8_LCASE) FROM t
SELECT make_timestamp(2024, 6, 15, 12, 0, 0, _3 COLLATE UTF8_LCASE) FROM t
```
The routing is byte-identical across the two settings. Only the hint
disappears. On Spark 4.1.3:
```
ToUnixTimestamp allowIncompatible=false native=[] dispatched=[collate,
resolvedcollation, to_unix_timestamp]
CometProject [COMET-INFO: A native implementation of ToUnixTimestamp is
available.
Set spark.comet.expression.ToUnixTimestamp.allowIncompatible=true to
enable it. ...]
Accelerated expressions: 0 native, 3 codegen dispatch.
ToUnixTimestamp allowIncompatible=true native=[] dispatched=[collate,
resolvedcollation, to_unix_timestamp]
CometProject
Accelerated expressions: 0 native, 3 codegen dispatch.
MakeTimestamp allowIncompatible=false native=[] dispatched=[cast,
collate, make_timestamp, resolvedcollation]
CometProject [COMET-INFO: A native implementation of MakeTimestamp is
available.
Set spark.comet.expression.MakeTimestamp.allowIncompatible=true to
enable it. ...]
Accelerated expressions: 0 native, 4 codegen dispatch.
MakeTimestamp allowIncompatible=true native=[] dispatched=[cast,
collate, make_timestamp, resolvedcollation]
CometProject
Accelerated expressions: 0 native, 4 codegen dispatch.
```
### Expected behavior
Results are correct in every case, so this is about diagnostics and
documentation rather than answers.
A collated argument on an expression with no native path should not report
`Incompatible`, because `Incompatible` is the level that means "Comet has a
native implementation that differs from Spark, and you can opt into it". The
natural fix is to drop the collation gate from these two serdes entirely and
let them report `Compatible`, since `CometCodegenDispatch` always runs Spark's
own generated code and therefore already honours collation exactly. Reporting
`Unsupported` instead would also silence the hint, but it would misdescribe the
situation, because these expressions are fully supported.
Either way, `getIncompatibleReasons()` should go too, so the compatibility
page stops advertising a native opt-in for these two.
### Additional context
This surfaced while reviewing #5789, which makes the same correction for
`unix_timestamp`. That PR removes the collation gate from `CometUnixTimestamp`
on the grounds that Spark's `ToTimestamp` only reads the format argument in the
`StringType` branch, so the format's collation cannot change the answer for
date and timestamp inputs. `ToUnixTimestamp` is the same `ToTimestamp`
subclass, and neither it nor `MakeTimestamp` has a native path for the gate to
protect in the first place.
These are the only two serdes in the tree that combine
`CometCodegenDispatch` with a non-`Compatible` support level, so the fix is
contained. `CometMakeInterval` also reports `Incompatible`, but it is a plain
`CometExpressionSerde` with a real native path, so its opt-in hint is genuine
and should stay.
Existing coverage to keep green: the `to_unix_timestamp` and
`make_timestamp` cases in `routing_datetime_collation_enabled.sql` and
`routing_datetime_collation_disabled.sql` (added by #5950) assert the dispatch
and fallback routes, and `CometCollationSuite` on Spark 4.0 and 4.1 has a
`make_timestamp rejects non-UTF8_BINARY collated timezone` and a
`to_unix_timestamp rejects non-UTF8_BINARY collated format` test, both
asserting the fallback reason text that would go away.
--
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]
