fix: Fall back to Spark for `trunc` / `date_trunc` functions when format string is unsupported, or is not a literal value by andygrove · Pull Request #2634 · apache/datafusion-comet

andygrove · 2025-10-22T22:30:52Z

Which issue does this PR close?

There is a follow-up issue #2649 for another bug discovered while working on this PR.

Rationale for this change

Both the trunc and date_trunc functions accept a format string parameter, such as year or month.

My understanding is that most users would specify a literal string for this parameter. However, it is possible to specify a column, or any other valid Spark expression. Unfortunately, Comet is not compatible with Spark for this use case if the expression evaluates to a format string that is not supported. The query will fail with an error such as:

Unsupported format: "foo" for function 'date_trunc'

The correct behavior would be to return NULL instead.

scala> spark.sql("select now(), a, date_trunc(a, now()) from t1").show
+--------------------+-----+--------------------+
|               now()|    a|date_trunc(a, now())|
+--------------------+-----+--------------------+
|2025-10-23 15:57:...| year| 2025-01-01 00:00:00|
|2025-10-23 15:57:...|month| 2025-10-01 00:00:00|
|2025-10-23 15:57:...|  foo|                NULL|
+--------------------+-----+--------------------+

The same is true for literal format strings:

scala> spark.sql("select now(), date_trunc('foo', now())").show
+--------------------+----------------------+
|               now()|date_trunc(foo, now())|
+--------------------+----------------------+
|2025-10-23 15:59:...|                  NULL|
+--------------------+----------------------+

What changes are included in this PR?

Fall back to Spark for invalid literal format string
Fall back to Spark for non-literal format string
Implement an improved test
Remove tests specific to the format being an array, which is no longer supported

How are these changes tested?

codecov-commenter · 2025-10-23T21:26:14Z

Codecov Report

❌ Patch coverage is 91.17647% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.29%. Comparing base (f09f8af) to head (8d15e51).
⚠️ Report is 644 commits behind head on main.

Files with missing lines	Patch %	Lines
...c/main/scala/org/apache/comet/serde/datetime.scala	93.93%	0 Missing and 2 partials ⚠️
...on/src/main/scala/org/apache/comet/CometConf.scala	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2634      +/-   ##
============================================
+ Coverage     56.12%   59.29%   +3.16%     
- Complexity      976     1449     +473     
============================================
  Files           119      147      +28     
  Lines         11743    13789    +2046     
  Branches       2251     2369     +118     
============================================
+ Hits           6591     8176    +1585     
- Misses         4012     4388     +376     
- Partials       1140     1225      +85

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

andygrove · 2025-10-27T14:52:22Z

spark/src/test/scala/org/apache/comet/CometExpressionSuite.scala

    }
  }

-  test("trunc with format array") {


we no longer support format being an array

I feel it is better to keep this functionality rather than lose it for a case that no user has complained about yet.

ok, I made it Incompatible rather than Unsupported and re-instated the tests

martin-g · 2025-10-28T08:23:24Z

spark/src/main/scala/org/apache/comet/serde/datetime.scala

+      "minute",
+      "hour",
+      "week",
+      "quarter")


week and quarter are listed twice

hsiang-c

LGTM

comphead · 2025-10-30T19:23:11Z

spark/src/main/scala/org/apache/comet/serde/datetime.scala

 object CometTruncDate extends CometExpressionSerde[TruncDate] {
+
+  val supportedFormats: Seq[String] =
+    Seq("year", "yyyy", "yy", "quarter", "mon", "month", "mm", "week")


The list of possible formats are:

fmt - the format representing the unit to be truncated to
"YEAR", "YYYY", "YY" - truncate to the first date of the year that the ts falls in, the time part will be zero out
"QUARTER" - truncate to the first date of the quarter that the ts falls in, the time part will be zero out
"MONTH", "MM", "MON" - truncate to the first date of the month that the ts falls in, the time part will be zero out
"WEEK" - truncate to the Monday of the week that the ts falls in, the time part will be zero out
"DAY", "DD" - zero out the time part
"HOUR" - zero out the minute and second with fraction part
"MINUTE"- zero out the second with fraction part
"SECOND" - zero out the second fraction part
"MILLISECOND" - zero out the microseconds
"MICROSECOND" - everything remains

Am I missing a reason why Comet doesn't support smaller than week formats?

https://spark.apache.org/docs/latest/api/sql/#date_trunc

This is very confusing in Spark, but date_trunc is actually TruncTimestamp and not TruncDate.

comphead

Thanks @andygrove lgtm
Btw when reviewing the code I found custom DateTrunc impl in Comet which probably not in use anymore spark-expr/src/datetime_funcs/date_trunc.rs

If it is I'll create a clean up PR

andygrove · 2025-10-30T20:57:53Z

Thanks @andygrove lgtm Btw when reviewing the code I found custom DateTrunc impl in Comet which probably not in use anymore spark-expr/src/datetime_funcs/date_trunc.rs

If it is I'll create a clean up PR

I assumed that those were the implementations that we were using, but I could be mistaken

andygrove · 2025-10-30T20:58:15Z

Thanks for the reviews @hsiang-c and @comphead

…mat string is unsupported, or is not a literal value (apache#2634)

andygrove added 11 commits October 22, 2025 16:23

save

6524002

add failing tests [skip ci]

b389449

tests pass

5e8b981

save [skip ci]

51455b0

check for fallbacks

a00e506

upmerge

3c6c9f0

fix

886c45e

check for specific fallback reason

21f4419

refactor

b3cd20f

format

aacb552

update signature

c8a3006

andygrove changed the title ~~fix: Fix various bugs in trunc / date_trunc functions [WIP]~~ fix: Fix various bugs in trunc / date_trunc functions Oct 23, 2025

remove tests that are no longer valid

8bd3287

andygrove changed the title ~~fix: Fix various bugs in trunc / date_trunc functions~~ fix: Fall back to Spark for trunc / date_trunc functions when format string is not a literal value Oct 23, 2025

andygrove mentioned this pull request Oct 25, 2025

date_trunc incorrect results in non-UTC timezone #2649

Open

prep for review

d828a67

andygrove marked this pull request as ready for review October 25, 2025 15:40

andygrove changed the title ~~fix: Fall back to Spark for trunc / date_trunc functions when format string is not a literal value~~ fix: Fall back to Spark for trunc / date_trunc functions when format string is unsupported, or is not a literal value Oct 27, 2025

andygrove commented Oct 27, 2025

View reviewed changes

Merge remote-tracking branch 'apache/main' into date-trunc

d6f5fd4

andygrove requested review from mbutrovich and parthchandra October 27, 2025 14:53

andygrove added 2 commits October 27, 2025 14:02

address feedback

77ccedd

fix

4fe51c0

martin-g reviewed Oct 28, 2025

View reviewed changes

andygrove added 2 commits October 28, 2025 16:47

Merge remote-tracking branch 'apache/main' into date-trunc

c354a05

remove duplicate and reorder list

8d15e51

hsiang-c approved these changes Oct 30, 2025

View reviewed changes

comphead reviewed Oct 30, 2025

View reviewed changes

comphead approved these changes Oct 30, 2025

View reviewed changes

andygrove merged commit 977a189 into apache:main Oct 30, 2025
103 checks passed

andygrove deleted the date-trunc branch October 30, 2025 20:58

andygrove mentioned this pull request Nov 6, 2025

trunc (TruncDate) should fall back to Spark for unsupported formats #2633

Closed

coderfender pushed a commit to coderfender/datafusion-comet that referenced this pull request Dec 13, 2025

fix: Fall back to Spark for trunc / date_trunc functions when for…

93fd82a

…mat string is unsupported, or is not a literal value (apache#2634)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fall back to Spark for `trunc` / `date_trunc` functions when format string is unsupported, or is not a literal value#2634

fix: Fall back to Spark for `trunc` / `date_trunc` functions when format string is unsupported, or is not a literal value#2634
andygrove merged 18 commits intoapache:mainfrom
andygrove:date-trunc

andygrove commented Oct 22, 2025 •

edited

Loading

Uh oh!

codecov-commenter commented Oct 23, 2025 •

edited

Loading

Uh oh!

andygrove Oct 27, 2025 •

edited

Loading

Uh oh!

parthchandra Oct 27, 2025

Uh oh!

andygrove Oct 27, 2025

Uh oh!

martin-g Oct 28, 2025

Uh oh!

hsiang-c left a comment

Uh oh!

comphead Oct 30, 2025

Uh oh!

comphead Oct 30, 2025

Uh oh!

andygrove Oct 30, 2025

Uh oh!

comphead left a comment

Uh oh!

andygrove commented Oct 30, 2025

Uh oh!

andygrove commented Oct 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

andygrove commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Uh oh!

codecov-commenter commented Oct 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

andygrove Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parthchandra Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

andygrove Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

martin-g Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

hsiang-c left a comment

Choose a reason for hiding this comment

Uh oh!

comphead Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

comphead Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

andygrove Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

andygrove commented Oct 30, 2025

Uh oh!

andygrove commented Oct 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

andygrove commented Oct 22, 2025 •

edited

Loading

codecov-commenter commented Oct 23, 2025 •

edited

Loading

andygrove Oct 27, 2025 •

edited

Loading