Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 23 additions & 9 deletions spark/src/main/scala/org/apache/comet/serde/unixtime.scala
Original file line number Diff line number Diff line change
Expand Up @@ -29,21 +29,35 @@ import org.apache.comet.serde.QueryPlanSerde.{exprToProtoInternal, optExprWithFa
// https://github.com/apache/datafusion/issues/16594
object CometFromUnixTime extends CometExpressionSerde[FromUnixTime] with CodegenDispatchFallback {

private val collationReason = DatetimeCollation.reason("from_unixtime")
// Applies even to the default format: Comet executes natively but DataFusion's valid timestamp
// range differs from Spark, so results can differ outside that range.
private val timestampRangeReason =
"DataFusion's valid timestamp range differs from Spark" +
" (https://github.com/apache/datafusion/issues/16594)"

// The native (DataFusion) path covers only the default pattern; documented as an unsupported
// limitation of that path rather than an incompatibility (see #4575).
private val formatReason =
"Only supports the default datetime format pattern `yyyy-MM-dd HH:mm:ss`." +
" DataFusion's valid timestamp range differs from Spark" +
" (https://github.com/apache/datafusion/issues/16594)"
"Only the default datetime format pattern `yyyy-MM-dd HH:mm:ss` is supported"

override def getIncompatibleReasons(): Seq[String] =
Seq(formatReason) ++ DatetimeCollation.incompatibleReasons("from_unixtime")
Seq(timestampRangeReason)

override def getUnsupportedReasons(): Seq[String] =
Seq(formatReason)

// A non-default format has no native (DataFusion) path, so it is `Unsupported`. Because
// `CodegenDispatchFallback` is mixed in, an `Unsupported` result still stays in the Comet
// pipeline via JVM codegen dispatch (Spark's own `doGenCode`) rather than falling back to Spark.
//
// Unlike the other datetime expressions, from_unixtime needs no collation guard: a collation can
// only appear on the format argument, and any collated format is a non-default format, which is
// already `Unsupported` here.
override def getSupportLevel(expr: FromUnixTime): SupportLevel = {
if (DatetimeCollation.hasNonDefaultCollation(expr)) {
Incompatible(Some(collationReason))
if (expr.format != Literal(TimestampFormatter.defaultPattern)) {
Unsupported(Some(formatReason))
} else {
Incompatible(Some(formatReason))
Incompatible(Some(timestampRangeReason))
}
}

Expand All @@ -60,7 +74,7 @@ object CometFromUnixTime extends CometExpressionSerde[FromUnixTime] with Codegen
val timeZone = exprToProtoInternal(Literal(expr.timeZoneId.orNull), inputs, binding)

if (expr.format != Literal(TimestampFormatter.defaultPattern)) {
withFallbackReason(expr, "Datetime pattern format is unsupported")
withFallbackReason(expr, formatReason)
None
} else if (secExpr.isDefined && formatExpr.isDefined) {
val timestampExpr =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1860,6 +1860,13 @@ class CometExpressionSuite extends CometTestBase with AdaptiveSparkPlanHelper {
// After fixing these issues, change checkSparkAnswer to checkSparkAnswerAndOperator
checkSparkAnswer(s"SELECT from_unixtime(_5, 'yyyy') FROM $table $where")
checkSparkAnswer(s"SELECT from_unixtime(_8, 'yyyy') FROM $table $where")
// A non-default format is Unsupported (no native DataFusion path), but at the default
// allowIncompatible=false it stays a Comet operator via CodegenDispatchFallback
// (Spark's own codegen) rather than falling back to Spark. See #4575.
withSQLConf(
CometConf.getExprAllowIncompatConfigKey(classOf[FromUnixTime]) -> "false") {
checkSparkAnswerAndOperator(s"SELECT from_unixtime(_5, 'yyyy') FROM $table $where")
}
withSQLConf(SESSION_LOCAL_TIMEZONE.key -> "Asia/Kathmandu") {
checkSparkAnswerAndOperator(s"SELECT from_unixtime(_5) FROM $table $where")
checkSparkAnswerAndOperator(s"SELECT from_unixtime(_8) FROM $table $where")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -306,9 +306,12 @@ class CometCollationSuite extends CometTestBase {
}

test("from_unixtime rejects non-UTF8_BINARY collated format (issue #4646)") {
// A collation can only appear on the format argument, so a collated format is a non-default
// format. from_unixtime has no native path for non-default formats, so it is reported as
// Unsupported (the format reason) rather than Incompatible (the collation reason).
checkDatetimeFallback(
"SELECT from_unixtime(_10, _6 COLLATE utf8_lcase) FROM datetime_collation_tbl",
"from_unixtime does not support non-UTF8_BINARY collations")
"Only the default datetime format pattern `yyyy-MM-dd HH:mm:ss` is supported")
}

test("make_timestamp rejects non-UTF8_BINARY collated timezone (issue #4646)") {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,9 +127,12 @@ class CometCollationSuite extends CometTestBase {
}

test("from_unixtime rejects non-UTF8_BINARY collated format (issue #4646)") {
// A collation can only appear on the format argument, so a collated format is a non-default
// format. from_unixtime has no native path for non-default formats, so it is reported as
// Unsupported (the format reason) rather than Incompatible (the collation reason).
checkDatetimeFallback(
"SELECT from_unixtime(_10, _6 COLLATE utf8_lcase) FROM datetime_collation_tbl",
"from_unixtime does not support non-UTF8_BINARY collations")
"Only the default datetime format pattern `yyyy-MM-dd HH:mm:ss` is supported")
}

test("make_timestamp rejects non-UTF8_BINARY collated timezone (issue #4646)") {
Expand Down
Loading