gengliangwang commented on code in PR #56908:
URL: https://github.com/apache/spark/pull/56908#discussion_r3503279814
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala:
##########
@@ -592,6 +605,13 @@ object DateTimeUtils extends SparkDateTimeUtils {
return Math.subtractExact(micros, Math.floorMod(micros,
MICROS_PER_SECOND))
case _ =>
}
+ // For date-level units the fast path resolves the truncated local
midnight with the
+ // source offset, which diverges from the slow path at a fall-back
overlap. The legacy
+ // flag forces the slow path so the earliest (offset-independent) result
is used.
+ if (level >= MIN_LEVEL_OF_DATE_TRUNC &&
Review Comment:
This reads the flag with `SQLConf.get.getConf(...)` per row, inside the
static `truncTimestamp` util, on every date-level call — including the default
`false` case, which is the common path SPARK-56769's fast path was built to
keep cheap. It also adds the first `SQLConf` dependency to a previously
config-free arithmetic utility.
The peer pattern in this module captures a behavior flag once at expression
construction and branches on the captured `Boolean` per row:
`SubtractTimestamps(legacyInterval: Boolean)` (datetimeExpressions.scala:4043,
defaulted from `SQLConf.get.legacyIntervalEnabled` at :4051) and the ANSI
`failOnError: Boolean = SQLConf.get.ansiEnabled` family. The sole caller here
is the `TruncTimestamp` expression (datetimeExpressions.scala:2886/2894),
already a `TimeZoneAwareExpression` and the natural capture site. Consider
capturing the flag on `TruncTimestamp` and threading it into `truncTimestamp` —
that removes the per-row lookup and keeps the config read on the driver,
matching the peer expressions. Non-blocking: the result is correct either way.
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala:
##########
@@ -902,6 +902,74 @@ class DateTimeUtilsSuite extends SparkFunSuite with
Matchers with SQLHelper {
assert(DateTimeUtils.truncTimestamp(apr, DateTimeUtils.TRUNC_TO_MONTH, la)
=== expectedMonth)
}
+ test("SPARK-57769: truncTimestamp date-level units at a fall-back overlap") {
+ // Europe/Berlin observed its first DST in 1916: clocks went back from
01:00 CEST
+ // (+02:00) to 00:00 CET (+01:00) on 1916-10-01, so the local time
1916-10-01 00:00
+ // (the truncated MONTH/WEEK boundary) is an overlap that exists at both
+02:00 and
+ // +01:00. The fast path resolves that midnight with the source
timestamp's offset,
+ // while the slow path always picks the earliest valid offset (+02:00
CEST).
+ val berlin = getZoneId("Europe/Berlin")
+ // Source is 1916-10-15, well after the transition, so its offset is
+01:00 CET.
+ val src = DateTimeUtils.stringToTimestamp(
+ UTF8String.fromString("1916-10-15T12:00:00+01:00"), berlin).get
+ // Default (new) behavior: the truncated midnight reuses the source offset
(+01:00).
+ val expectedDefault = DateTimeUtils.stringToTimestamp(
+ UTF8String.fromString("1916-10-01T00:00:00+01:00"), berlin).get
+ // Legacy behavior: the earliest valid offset (+02:00 CEST) is always used.
+ val expectedLegacy = DateTimeUtils.stringToTimestamp(
+ UTF8String.fromString("1916-10-01T00:00:00+02:00"), berlin).get
+ // MONTH truncation of the 1916-10-15 source lands on 1916-10-01, the
overlap day.
+ // (WEEK truncates to Monday 1916-10-09, which is not on the overlap, so
it is not
+ // affected and is covered by the QUARTER/YEAR Azores case below.)
+ assert(expectedDefault !== expectedLegacy)
+ withSQLConf(SQLConf.LEGACY_TIMESTAMP_TRUNCATE_OVERLAP_EARLIEST.key ->
"false") {
+ assert(DateTimeUtils.truncTimestamp(src, DateTimeUtils.TRUNC_TO_MONTH,
berlin)
+ === expectedDefault)
+ }
+ withSQLConf(SQLConf.LEGACY_TIMESTAMP_TRUNCATE_OVERLAP_EARLIEST.key ->
"true") {
+ assert(DateTimeUtils.truncTimestamp(src, DateTimeUtils.TRUNC_TO_MONTH,
berlin)
+ === expectedLegacy)
+ }
+
+ // Determinism: a second source whose offset is +02:00 CEST (an instant
sitting in
+ // the overlap itself) truncates to the +02:00 boundary under the fast
path. Under
+ // the default behavior the two October sources therefore disagree, while
the legacy
+ // behavior maps both to the same instant (so GROUP BY date_trunc is
stable).
+ val srcInOverlap = DateTimeUtils.stringToTimestamp(
+ UTF8String.fromString("1916-10-01T00:30:00+02:00"), berlin).get
+ withSQLConf(SQLConf.LEGACY_TIMESTAMP_TRUNCATE_OVERLAP_EARLIEST.key ->
"false") {
+ assert(DateTimeUtils.truncTimestamp(srcInOverlap,
DateTimeUtils.TRUNC_TO_MONTH, berlin)
+ === expectedLegacy)
+ assert(DateTimeUtils.truncTimestamp(src, DateTimeUtils.TRUNC_TO_MONTH,
berlin)
+ !== DateTimeUtils.truncTimestamp(srcInOverlap,
DateTimeUtils.TRUNC_TO_MONTH, berlin))
+ }
+ withSQLConf(SQLConf.LEGACY_TIMESTAMP_TRUNCATE_OVERLAP_EARLIEST.key ->
"true") {
+ assert(DateTimeUtils.truncTimestamp(src, DateTimeUtils.TRUNC_TO_MONTH,
berlin)
+ === DateTimeUtils.truncTimestamp(srcInOverlap,
DateTimeUtils.TRUNC_TO_MONTH, berlin))
+ }
+
+ // Atlantic/Azores switched from LMT (-01:54:32) to -02:00 at 1912-01-01
00:00, so the
+ // YEAR/MONTH boundary 1912-01-01 00:00 is an overlap between the two
offsets (a 328s
+ // difference). The fast path uses the source offset (-02:00), the slow
path the
+ // earliest (-01:54:32 LMT).
+ val azores = getZoneId("Atlantic/Azores")
+ val azSrc = DateTimeUtils.stringToTimestamp(
+ UTF8String.fromString("1912-01-15T12:00:00-02:00"), azores).get
+ // Build the expected instants directly: the LMT offset (-01:54:32) has
sub-minute
+ // precision that the timestamp string parser does not accept.
+ val azDefault = instantToMicros(Instant.parse("1912-01-01T02:00:00Z"))
+ val azLegacy = instantToMicros(Instant.parse("1912-01-01T01:54:32Z"))
+ assert(azDefault !== azLegacy)
+ for (level <- Seq(DateTimeUtils.TRUNC_TO_YEAR,
DateTimeUtils.TRUNC_TO_MONTH)) {
Review Comment:
The Berlin block above says WEEK "is not affected and is covered by the
QUARTER/YEAR Azores case below", but this loop tests only YEAR and MONTH, so
the named QUARTER case doesn't exist. `QUARTER(1912-01-15)` truncates to
1912-01-01 — the Azores overlap, same instant as YEAR/MONTH — so QUARTER is
worth adding here:
```suggestion
for (level <- Seq(DateTimeUtils.TRUNC_TO_YEAR,
DateTimeUtils.TRUNC_TO_QUARTER,
DateTimeUtils.TRUNC_TO_MONTH)) {
```
WEEK is a different matter: it truncates `1912-01-15` to `1912-01-15` and
`1916-10-15` to `1916-10-09`, neither of which is an overlap, so WEEK can't
actually be exercised at an overlap by either source (don't add it here — it
would assert against the wrong instant). Worth rewording the WEEK clause in the
comment above to say it simply isn't covered at an overlap here, rather than
pointing at the QUARTER/YEAR case.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
