You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "0lai0 (via GitHub)" <gi...@apache.org> on 2026/06/19 18:33:24 UTC
[PR] fix: gate non-default collations for Spark 4 datetime expressions [datafusion-comet]
0lai0 opened a new pull request, #4693:
URL: https://github.com/apache/datafusion-comet/pull/4693
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #4646.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
Spark 4.0 adds `StringTypeWithCollation` support to several datetime expression string inputs. Comet previously did not distinguish non-default collations for these inputs, so affected expressions could be planned as compatible even though native execution does not honor collation semantics.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
- Adds a shared datetime collation guard for Spark 4 datetime expression serdes.
- Marks non-default collated string inputs as Incompatible(Some(...)) for: `convert_timezone, date_format, date_trunc, from_unixtime, make_timestamp, next_day, to_unix_timestamp, trunc, and unix_timestamp.`
- Updates Spark 4.0 and 4.1 collation tests to verify fallback reasons and codegen-dispatch behavior without `LocalTableScan `masking the expression fallback.
## How are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
- `./mvnw compile test-compile -Pspark-4.0 -DskipTests`
- `./mvnw test -Pspark-4.0 -Dtest=none -Dsuites=org.apache.spark.sql.CometCollationSuite`
- `./mvnw compile test-compile -Pspark-4.1 -DskipTests`
- `./mvnw test -Pspark-4.1 -Dtest=none -Dsuites=org.apache.spark.sql.CometCollationSuite`
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: gate non-default collations for Spark 4 datetime expressions [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #4693:
URL: https://github.com/apache/datafusion-comet/pull/4693#discussion_r3475026778
##########
spark/src/main/scala/org/apache/comet/serde/datetime.scala:
##########
@@ -711,9 +777,10 @@ object CometDateFormat extends CometExpressionSerde[DateFormatClass] {
case _ => None
}
- val canUseNative = nativeFormat.isDefined && {
- isUtc || CometConf.isExprAllowIncompat(getExprConfigName(expr))
- }
+ val canUseNative = nativeFormat.isDefined &&
+ !expr.children.exists(c => hasNonDefaultStringCollation(c.dataType)) && {
Review Comment:
This seems incorrect, or is at least inconsistent with other date/time functions. It seems there is no way to enable the native incompatible expression now due to the `hasNonDefaultStringCollation` check introduced here?
PR looks good other than this issue.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: gate non-default collations for Spark 4 datetime expressions [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on code in PR #4693:
URL: https://github.com/apache/datafusion-comet/pull/4693#discussion_r3500134670
##########
spark/src/test/spark-4.1/org/apache/spark/sql/CometCollationSuite.scala:
##########
@@ -172,6 +172,18 @@ class CometCollationSuite extends CometTestBase {
"date_format does not support non-UTF8_BINARY collations")
}
+ test("date_format uses native path with collated format when allowIncompatible is enabled") {
Review Comment:
Thanks for the review! Consolidating these into the 4.1+ shim also needs `add-test-source` in spark/pom.xml to pick up `minorPlusVerSrc` (right now only the main-source `add-source` does), and the suite has to be moved rather than copied to avoid a duplicate `CometCollationSuite` class on the 4.1 classpath.
I'll do that in a follow-up PR.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: gate non-default collations for Spark 4 datetime expressions [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on code in PR #4693:
URL: https://github.com/apache/datafusion-comet/pull/4693#discussion_r3475213673
##########
spark/src/main/scala/org/apache/comet/serde/datetime.scala:
##########
@@ -711,9 +777,10 @@ object CometDateFormat extends CometExpressionSerde[DateFormatClass] {
case _ => None
}
- val canUseNative = nativeFormat.isDefined && {
- isUtc || CometConf.isExprAllowIncompat(getExprConfigName(expr))
- }
+ val canUseNative = nativeFormat.isDefined &&
+ !expr.children.exists(c => hasNonDefaultStringCollation(c.dataType)) && {
Review Comment:
Thanks for catching this!!
I missed it. The collation check in `canUseNative` was incorrectly blocking the native path even when `DateFormatClass.allowIncompatible=true`.
I have also added regression coverage in `CometCollationSuite` for Spark 4.0 and 4.1 to handle cases involving `date_format` with a collated literal format and `DateFormatClass.allowIncompatible=true`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: gate non-default collations for Spark 4 datetime expressions [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on code in PR #4693:
URL: https://github.com/apache/datafusion-comet/pull/4693#discussion_r3445796284
##########
spark/src/main/scala/org/apache/comet/serde/datetime.scala:
##########
@@ -289,13 +290,26 @@ object CometSecond extends CometExpressionSerde[Second] with CodegenDispatchFall
}
}
+private[serde] object DatetimeCollation extends CometTypeShim {
+ def reason(functionName: String): String =
+ s"$functionName does not support non-UTF8_BINARY collations " +
+ "(https://github.com/apache/datafusion-comet/issues/4646)"
+
+ def hasNonDefaultCollation(expr: Expression): Boolean =
+ expr.children.exists(c => hasNonDefaultStringCollation(c.dataType))
+}
+
object CometUnixTimestamp extends CometExpressionSerde[UnixTimestamp] {
+ private val collationReason = DatetimeCollation.reason("unix_timestamp")
+
override def getUnsupportedReasons(): Seq[String] = Seq(
"Only `TimestampType` and `DateType` inputs are supported." +
" `TimestampNTZType` is not supported because Comet incorrectly applies timezone" +
" conversion to TimestampNTZ values.")
+ override def getIncompatibleReasons(): Seq[String] = Seq(collationReason)
Review Comment:
Sure, makes sense! I will move it into `CometTypeShim` so the reason appears only on 4.x, just as `hasNonDefaultStringCollation` is already shimmed.
I'll push an update shortly. Thanks for review
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: gate non-default collations for Spark 4 datetime expressions [datafusion-comet]
Posted by "0lai0 (via GitHub)" <gi...@apache.org>.
0lai0 commented on code in PR #4693:
URL: https://github.com/apache/datafusion-comet/pull/4693#discussion_r3475213673
##########
spark/src/main/scala/org/apache/comet/serde/datetime.scala:
##########
@@ -711,9 +777,10 @@ object CometDateFormat extends CometExpressionSerde[DateFormatClass] {
case _ => None
}
- val canUseNative = nativeFormat.isDefined && {
- isUtc || CometConf.isExprAllowIncompat(getExprConfigName(expr))
- }
+ val canUseNative = nativeFormat.isDefined &&
+ !expr.children.exists(c => hasNonDefaultStringCollation(c.dataType)) && {
Review Comment:
Thanks for catching this!!
I missed it. The collation check in `canUseNative` was incorrectly blocking the native path even when `DateFormatClass.allowIncompatible=true`.
I removed the guard so that the non-default collation case is now handled by `getSupportLevel()` as *Incompatible*. When `allowIncompatible` is false, the request still routes through the dispatcher/fallback, but when it is true, the native path can be used.
I have also added regression coverage in `CometCollationSuite` for Spark 4.0 and 4.1 to handle cases involving `date_format` with a collated literal format and `DateFormatClass.allowIncompatible=true`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: gate non-default collations for Spark 4 datetime expressions [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #4693:
URL: https://github.com/apache/datafusion-comet/pull/4693
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: gate non-default collations for Spark 4 datetime expressions [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #4693:
URL: https://github.com/apache/datafusion-comet/pull/4693#discussion_r3444122643
##########
spark/src/main/scala/org/apache/comet/serde/datetime.scala:
##########
@@ -289,13 +290,26 @@ object CometSecond extends CometExpressionSerde[Second] with CodegenDispatchFall
}
}
+private[serde] object DatetimeCollation extends CometTypeShim {
+ def reason(functionName: String): String =
+ s"$functionName does not support non-UTF8_BINARY collations " +
+ "(https://github.com/apache/datafusion-comet/issues/4646)"
+
+ def hasNonDefaultCollation(expr: Expression): Boolean =
+ expr.children.exists(c => hasNonDefaultStringCollation(c.dataType))
+}
+
object CometUnixTimestamp extends CometExpressionSerde[UnixTimestamp] {
+ private val collationReason = DatetimeCollation.reason("unix_timestamp")
+
override def getUnsupportedReasons(): Seq[String] = Seq(
"Only `TimestampType` and `DateType` inputs are supported." +
" `TimestampNTZType` is not supported because Comet incorrectly applies timezone" +
" conversion to TimestampNTZ values.")
+ override def getIncompatibleReasons(): Seq[String] = Seq(collationReason)
Review Comment:
minor nit: this incompatibility will get documented for all Spark versions even though it is specific to Spark 4.x
I wonder if we can shim `getIncompatibleReasons` so it only applies for 4.x?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: gate non-default collations for Spark 4 datetime expressions [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #4693:
URL: https://github.com/apache/datafusion-comet/pull/4693#discussion_r3481806406
##########
spark/src/test/spark-4.1/org/apache/spark/sql/CometCollationSuite.scala:
##########
@@ -172,6 +172,18 @@ class CometCollationSuite extends CometTestBase {
"date_format does not support non-UTF8_BINARY collations")
}
+ test("date_format uses native path with collated format when allowIncompatible is enabled") {
Review Comment:
minor nit: this test is duplicated between 4.1 and 4.2 shims and could move to the 4.1+ shim, which was recently introduced. We can do this later.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org