Search code, repositories, users, issues, pull requests...

Copy link

Member

@andygrove andygrove commented Oct 22, 2025 •

edited

Loading

Which issue does this PR close?

Closes #2621

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?

andygrove added 11 commits

October 22, 2025 16:23


 save


 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 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

@codecov-commenter

Copy link

codecov-commenter commented Oct 23, 2025 •

edited

Loading

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.

date_trunc incorrect results in non-UTC timezone #2649


 remove tests that are no longer valid

8bd3287

@andygrove 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 andygrove mentioned this pull request

Oct 25, 2025

Open


 prep for review

d828a67

@andygrove andygrove marked this pull request as ready for review

October 25, 2025 15:40

@andygrove 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

andygrove commented

Oct 27, 2025

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

}

test("trunc with format array") {

Copy link

Member Author

@andygrove andygrove Oct 27, 2025 •

edited

Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we no longer support format being an array

Copy link

Contributor

@parthchandra parthchandra Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Member Author

@andygrove andygrove Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

d6f5fd4

@andygrove 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

martin-g reviewed

Oct 28, 2025

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

"minute",

"hour",

"week",

"quarter")

Copy link

Member

@martin-g martin-g Oct 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

week and quarter are listed twice

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

hsiang-c approved these changes

Copy link

Contributor

@hsiang-c hsiang-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

comphead

comphead reviewed

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

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")

Copy link

Contributor

@comphead comphead Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Contributor

@comphead comphead Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Member Author

@andygrove andygrove Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

comphead

comphead approved these changes

Copy link

Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Member Author

andygrove commented Oct 30, 2025

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

Copy link

Member Author

andygrove commented Oct 30, 2025

Thanks for the reviews @hsiang-c and @comphead

@andygrove andygrove merged commit 977a189 into apache:main