andygrove commented on code in PR #5956:
URL: https://github.com/apache/datafusion-comet/pull/5956#discussion_r4155873697
##########
native/spark-expr/src/kernels/temporal.rs:
##########
@@ -680,6 +776,175 @@ where
Ok(result)
}
+/// The scalar-format implementation retained for values outside DataFusion
55.1's internal
+/// TimestampNanosecond range. Row-format paths continue to call the same
underlying helpers.
+fn timestamp_trunc_legacy(
+ array: &TimestampMicrosecondArray,
+ format: &str,
+) -> Result<TimestampMicrosecondArray, SparkError> {
+ let builder = TimestampMicrosecondBuilder::with_capacity(array.len());
+ let iter = ArrayIter::new(array);
+ match array.data_type() {
+ DataType::Timestamp(TimeUnit::Microsecond, None) => {
+ timestamp_trunc_ntz(array, format.to_string())
+ }
+ DataType::Timestamp(TimeUnit::Microsecond, Some(tz)) => {
+ let trunc_fn = tz_trunc_fn_for_format(format)?;
+ as_timestamp_tz_with_op::<&TimestampMicrosecondArray,
TimestampMicrosecondType, _>(
+ iter,
+ builder,
+ tz,
+ |dt| as_micros_from_unix_epoch_utc(trunc_fn(dt)),
+ )
+ }
+ dt => return_compute_error_with!(
+ "Unsupported input type '{:?}' for function 'timestamp_trunc'",
+ dt
+ ),
+ }
+}
+
+fn datafusion_timestamp_trunc_requires_nanos(granularity: &str, has_timezone:
bool) -> bool {
+ match granularity {
+ "microsecond" | "millisecond" | "second" | "minute" => false,
+ "hour" | "day" => has_timezone,
+ "week" | "month" | "quarter" | "year" => true,
+ _ => unreachable!("granularity was normalized before compatibility
dispatch"),
+ }
+}
+
+/// Returns whether a timezone has a zero UTC offset at every instant.
+///
+/// Keep this list conservative: an unlisted timezone takes the zone-aware
path, which is always
+/// correct even when its current offset happens to be zero.
+fn is_utc_timezone(timezone: &str) -> bool {
+ matches!(
+ timezone,
+ "UTC" | "Etc/UTC" | "Etc/GMT" | "GMT" | "Z" | "+00:00" | "-00:00" |
"00:00"
+ )
+}
+
+/// Truncate timezone-aware timestamps to the local minute boundary.
+///
+/// DataFusion floors the stored UTC microseconds directly. That is only
equivalent to Spark's
+/// local-time truncation when the timezone offset is a whole number of
minutes. Historical offsets
+/// can include seconds, so resolve the offset for each instant before finding
the local remainder.
+/// If truncation crosses an offset transition, re-resolve the truncated local
datetime just like
+/// `ZonedDateTime.truncatedTo` rather than retaining the input instant's
offset.
+fn timestamp_trunc_minute_tz(
+ array: &TimestampMicrosecondArray,
+ timezone: &str,
+) -> Result<TimestampMicrosecondArray, SparkError> {
+ as_timestamp_tz_with_op::<&TimestampMicrosecondArray,
TimestampMicrosecondType, _>(
+ ArrayIter::new(array),
+ TimestampMicrosecondBuilder::with_capacity(array.len()),
+ timezone,
+ |dt| {
+ let micros = dt.timestamp_micros();
+ let timezone = dt.timezone();
+ let original_offset_secs = dt.offset().fix().local_minus_utc();
+ let offset_micros = i64::from(original_offset_secs) * 1_000_000;
+ let candidate = micros - (micros +
offset_micros).rem_euclid(MICROS_PER_MINUTE);
+ let candidate_dt =
+
as_datetime_with_timezone::<TimestampMicrosecondType>(candidate, timezone)
+ .expect("truncated minute candidate must be a valid
datetime");
+ let candidate_offset_secs =
candidate_dt.offset().fix().local_minus_utc();
+
+ if candidate_offset_secs == original_offset_secs {
+ return candidate;
+ }
+
+ let truncated_local = dt
+ .naive_local()
+ .with_second(0)
+ .and_then(|local| local.with_nanosecond(0))
+ .expect("truncated local minute must be a valid datetime");
+ match timezone.from_local_datetime(&truncated_local) {
+ LocalResult::Single(resolved) => resolved.timestamp_micros(),
+ LocalResult::Ambiguous(earlier, later) => {
+ // ZonedDateTime retains the original offset when it is
valid in an overlap.
+ if earlier.offset().fix().local_minus_utc() ==
original_offset_secs {
+ earlier.timestamp_micros()
+ } else if later.offset().fix().local_minus_utc() ==
original_offset_secs {
+ later.timestamp_micros()
+ } else {
+ earlier.timestamp_micros()
+ }
+ }
+ LocalResult::None => {
+ // The candidate lies immediately before a forward
transition. Java advances
+ // a nonexistent local time by the gap, which is
equivalent to resolving it
+ // with the candidate's pre-transition offset.
+ naive_to_micros(truncated_local) -
i64::from(candidate_offset_secs) * 1_000_000
+ }
+ }
+ },
+ )
+}
+
+fn timestamp_trunc_upstream(
+ array: &TimestampMicrosecondArray,
+ format: &str,
+) -> Result<TimestampMicrosecondArray, SparkError> {
+ let granularity = normalize_timestamp_trunc_format(format)?;
+
+ if granularity == "minute" {
+ if let Some(timezone) = array.timezone().filter(|tz|
!is_utc_timezone(tz)) {
+ return timestamp_trunc_minute_tz(array, timezone);
+ }
+ }
+
+ let requires_nanos =
Review Comment:
With a non-UTC zone and `allowIncompatible` on, `WEEK`, `MONTH`, `QUARTER`
and `YEAR` can differ from Spark when the truncated local midnight is
ambiguous. DataFusion's `_date_trunc_coarse_with_tz` picks the occurrence whose
offset matches the input instant. Spark's `DateTimeUtils.truncTimestamp`
resolves these four levels with `daysToMicros`, which uses
`LocalDate.atStartOfDay(zoneId)` and always takes the earlier occurrence.
`HOUR` and `DAY` are fine because Spark uses `ZonedDateTime.truncatedTo`, which
keeps the input offset.
A real case is `America/Havana`, where DST ended at 01:00 on 2026-11-01 so
local midnight happened twice. With
`spark.sql.session.timeZone=America/Havana`, `date_trunc('MONTH',
timestamp('2026-11-15T12:00:00Z'))` is `2026-11-01T04:00:00Z` in Spark but
`2026-11-01T05:00:00Z` here. The same thing happens for November 2015 and 2020.
The kernel this replaces took the earlier occurrence
(`LocalResult::Ambiguous(resolved, _)` in `as_micros_from_unix_epoch_utc`), so
this is a regression for those inputs.
Could these four granularities with a non-UTC zone use a small zone-aware
helper like `timestamp_trunc_minute_tz`? It would truncate the local date, then
resolve midnight with the earlier occurrence on an overlap and shift by the gap
length on a gap, which is what `atStartOfDay` does. Can you also add
`America/Havana` and a mid-November 2026 row to
`trunc_timestamp_dst_ambiguous.sql` and a Rust test for it?
--
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]
