vranes opened a new pull request, #57401:
URL: https://github.com/apache/spark/pull/57401
### What changes were proposed in this pull request?
For `TIMESTAMP` (LTZ) inputs in a non-UTC session, `BinByExec` placed
multi-day civil-time bin boundaries by forward-walking (each bin's end reused
as the next bin's start) rather than computing every boundary independently
from the origin on the absolute grid `bin_start(k) =
timestampAddDayTime(origin, k * width)` that `time_bucket` uses. This PR
changes `BinByExec` to compute every boundary on that absolute grid.
- `DateTimeUtils.timeBucketDTInterval` is factored into two reusable pieces,
and defined in terms of them:
- `timeBucketIndexDTInterval(width, ts, origin, zone)`: the index of the
bucket containing `ts`.
- `timeBucketBoundaryDTInterval(width, k, origin, zone)`: the grid
boundary of bucket index `k`.
- `timeBucketDTInterval` becomes `boundary(index(ts))`, so it is
behavior-preserving for `time_bucket` (its only other caller); the existing
`DateTimeUtilsSuite` cases are the regression guard.
- `BinByExec` advances the bucket index and reads each boundary off the grid
via `timeBucketBoundaryDTInterval`, instead of walking with
`timestampAddDayTime`. The zero-length-range path computes `bin_end` as the
next grid boundary for the same reason.
### Why are the changes needed?
`timestampAddDayTime` adds the width's calendar-day part as calendar days
(which absorb DST: a calendar day is 23h or 25h of real time across a
transition) and the sub-day remainder as real microseconds, so it is not
associative across a DST or offset transition:
```
addDayTime(addDayTime(origin, k*W), W) != addDayTime(origin, (k+1)*W)
```
The forward walk (left side) therefore drifts off the grid (right side) by
the transition hour. For a width with a non-whole-day part (for example
`INTERVAL '36' HOUR`) whose bins span a DST transition, boundaries are wrong
from the second emitted bin onward:
- The same calendar bin is reported with a different `bin_start` / `bin_end`
depending on where a row's range began (a row that walked into bin N vs. a row
that started in bin N), so `GROUP BY bin_start` fragments one bin into two
groups an hour apart.
- At a whole-day zone skip (for example `Pacific/Apia` on 2011-12-30, which
does not exist in that zone), two consecutive grid boundaries collapse to the
same instant.
Whole-day widths (`INTERVAL '1' DAY`, `'2' DAY`) were unaffected because the
walk and the grid agree when the width has no sub-day remainder; sub-day widths
use the UTC fast path and are already grid-aligned; `TIMESTAMP_NTZ` uses UTC
arithmetic and is unaffected. The defect lived only in the emission of the
second and later bins of a row, a path `time_bucket` does not have (it returns
one bucket per row), so the shared `time_bucket` tests could not surface it,
and the prior `BinBySuite` DST tests all used whole-day widths.
### Does this PR introduce _any_ user-facing change?
Yes, a bug fix for a not-yet-released feature (BIN BY is gated off by
default and has no released version). For a `TIMESTAMP` (LTZ) input in a
non-UTC session with a bin width that has a non-whole-day part (e.g. 36h), the
`bin_start` / `bin_end` of the second and later bins of a range spanning a DST
transition now land on the absolute grid (matching `time_bucket`) instead of
drifting by the transition hour. Whole-day widths, sub-day widths, and
`TIMESTAMP_NTZ` inputs are unchanged.
### How was this patch tested?
- `DateTimeUtilsSuite`: the existing `timeBucketDTInterval` cases are
unchanged (regression guard proving the factoring is behavior-preserving), plus
a new round-trip test that `boundary(index(ts)) == timeBucketDTInterval(ts)`
across sub-day, 36h spring-forward, and fall-back cases, and a whole-day
`Pacific/Apia` skip where two consecutive boundaries collapse.
- `BinBySuite`: new tests that (1) 36h boundaries across a spring-forward
spanning several bins land on the grid, (2) two rows reaching the same bin from
different starts report the identical boundary, (3) a whole-day `Pacific/Apia`
skip emits the collapsed zero-width ratio-0 bin while the real bins still tile
the range, and (4) a zero-length range under a 36h width places `bin_end` on
the grid. The multi-bin and row-independence tests were confirmed to fail on
the previous forward-walk code and pass after the fix.
- `SQLQueryTestSuite -- -z bin-by`: the golden file is unchanged (its cases
are UTC sub-day, unaffected by this fix).
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Anthropic)
--
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]
