Skip to content

to_unix_timestamp and make_timestamp advertise an allowIncompatible native opt-in that does not exist #6080

Description

@andygrove

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:

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions