You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "Licht-T (via GitHub)" <gi...@apache.org> on 2026/06/28 15:22:09 UTC
[PR] [SPARK-57737][SQL] Cache zone offset per task in date_trunc [spark]
Licht-T opened a new pull request, #56848:
URL: https://github.com/apache/spark/pull/56848
### What changes were proposed in this pull request?
`date_trunc` (`TruncTimestamp`) resolves the session zone offset for each row via `ZoneRules.getOffset(Instant)` -- a binary search over the zone's transition array -- and for non-fixed-offset zones it does so twice per row (the input instant and the candidate truncated instant used by the DST-equality guard from SPARK-56663 / SPARK-56769).
This PR adds a per-task `ZoneOffsetCache` that memoizes the resolved offset over the half-open epoch-second interval `[lo, hi)` on which it is provably constant, derived from the surrounding zone transitions (`nextTransition` / `previousTransition`, anchored on an interior point to avoid an off-by-one when an instant sits exactly on a transition). A lookup inside the cached interval reduces to two comparisons instead of a binary search.
### Why are the changes needed?
The session time zone is constant for a query and a zone's offset is piecewise-constant between DST/historical transitions, so consecutive rows almost always fall in the same constant-offset window (analytic data is typically temporally clustered -- time series, date-partitioned tables, post-sort). Repeating the transition-array binary search on every row is redundant work on the hot path.
`DateTimeBenchmark` Truncation, whole-stage codegen on, session zone `America/Los_Angeles`, OpenJDK 17 on a 12th Gen Intel i7-1260P, ns/row (lower is better):
| level | without cache | with cache | speedup |
|-------|--------------:|-----------:|--------:|
| date_trunc YEAR | 98.2 | 56.8 | 1.73x |
| date_trunc QUARTER | 109.3 | 71.7 | 1.52x |
| date_trunc MONTH | 90.8 | 53.7 | 1.69x |
| date_trunc WEEK | 77.8 | 40.6 | 1.92x |
| date_trunc DAY | 64.8 | 33.0 | 1.96x |
| date_trunc SECOND (control) | 28.7 | 27.7 | ~1.0x |
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing `DateTimeUtilsSuite` and `DateExpressionsSuite` pass.
### Was this patch authored or co-authored using generative AI tooling?
Yes, co-authored with Claude Code.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SQL] Speed up date_trunc zone offset resolution with a per-task cache [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #56848:
URL: https://github.com/apache/spark/pull/56848#issuecomment-5530716103
Nits
1. The on-transition unit test still describes the old previousTransition(hi - 1) anchor. In-range lookups now use floorIndex; hi - 1 remains only on the post-horizon miss path.
2. Interpreted eval still does new ZoneOffsetCache(zoneId) per row (documented as one-shot). That is safe if the expression instance is shared across threads; codegen is the hot path. No change needed unless you want interpreted to reuse a cache without putting mutable state on the expression. Please verify.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SQL] Cache zone offset per task in date_trunc [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #56848:
URL: https://github.com/apache/spark/pull/56848#discussion_r3490150016
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala:
##########
@@ -1287,3 +1298,60 @@ object DateTimeUtils extends SparkDateTimeUtils {
c
}
}
+
+/**
+ * Per-task memoization of a zone's UTC offset, used by the [[DateTimeUtils.truncTimestamp]] hot
+ * path. The session zone is constant for a query and the offset is piecewise-constant between DST
+ * transitions, so consecutive rows almost always resolve to the same offset. The cache holds the
+ * half-open epoch-second interval `[lo, hi)` on which the offset is provably constant -- derived
+ * from the surrounding zone transitions -- so a lookup that falls in the interval reduces to two
+ * comparisons instead of a transition-array binary search.
+ *
+ * Not thread-safe by design: a fresh instance is created per task (codegen mutable state) and used
+ * single-threaded, mirroring how stateful per-row helpers are scoped in generated code.
+ */
+class ZoneOffsetCache(val zoneId: ZoneId) {
Review Comment:
Nice optimization, and the `hi-1` anchoring to dodge the on-transition off-by-one is well reasoned. One gap: `ZoneOffsetCache` has no direct unit test. The existing `DateTimeUtilsSuite` truncation tests all go through the one-shot `truncTimestamp(micros, level, zoneId)` overload, which builds a *fresh* cache per call — so they confirm the offset *values* near DST transitions, but never exercise (a) the persistent cross-row reuse the codegen per-task cache depends on, nor (b) the on-transition interval boundary directly (the subtle part).
Since the class is public, a small focused test would lock in the reasoning: consecutive `offsetSeconds` lookups within one window (hit), a lookup straddling a transition (miss + window reset), and a lookup with `epochSec` exactly on a transition instant (the `hi-1` anchor). Non-blocking.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SQL] Cache zone offset per task in date_trunc [spark]
Posted by "Licht-T (via GitHub)" <gi...@apache.org>.
Licht-T commented on PR #56848:
URL: https://github.com/apache/spark/pull/56848#issuecomment-5060151319
Re-running the bench.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SQL] Cache zone offset per task in date_trunc [spark]
Posted by "Licht-T (via GitHub)" <gi...@apache.org>.
Licht-T commented on code in PR #56848:
URL: https://github.com/apache/spark/pull/56848#discussion_r3639172444
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala:
##########
@@ -1287,3 +1298,60 @@ object DateTimeUtils extends SparkDateTimeUtils {
c
}
}
+
+/**
+ * Per-task memoization of a zone's UTC offset, used by the [[DateTimeUtils.truncTimestamp]] hot
+ * path. The session zone is constant for a query and the offset is piecewise-constant between DST
+ * transitions, so consecutive rows almost always resolve to the same offset. The cache holds the
+ * half-open epoch-second interval `[lo, hi)` on which the offset is provably constant -- derived
+ * from the surrounding zone transitions -- so a lookup that falls in the interval reduces to two
+ * comparisons instead of a transition-array binary search.
+ *
+ * Not thread-safe by design: a fresh instance is created per task (codegen mutable state) and used
+ * single-threaded, mirroring how stateful per-row helpers are scoped in generated code.
+ */
+class ZoneOffsetCache(val zoneId: ZoneId) {
+ private val rules = zoneId.getRules
+ val isFixedOffset: Boolean = rules.isFixedOffset
+
+ private var cached = false
+ private var lo = 0L
+ private var hi = 0L
+ private var offsetSec = 0L
+
+ /**
+ * Offset in seconds at `epochSec`, equal to
+ * `rules.getOffset(Instant.ofEpochSecond(epochSec)).getTotalSeconds`, memoized over the
+ * constant-offset interval containing the previous lookup.
+ */
+ def offsetSeconds(epochSec: Long): Long = {
+ if (cached && epochSec >= lo && epochSec < hi) {
+ offsetSec
+ } else {
+ val instant = Instant.ofEpochSecond(epochSec)
+ val o = rules.getOffset(instant).getTotalSeconds.toLong
+ if (isFixedOffset) {
+ lo = Long.MinValue
+ hi = Long.MaxValue
+ } else {
+ val nextT = rules.nextTransition(instant)
Review Comment:
Good catch. The benchmarked input (`timestamp_seconds(id)`) is monotonic and never misses, so it hid exactly this. I added two miss-heavy `date_trunc` cases to `DateTimeBenchmark`: timestamps shuffled over a bounded ~2-year span, and spread uniformly over ~95 years.
Those confirmed your prediction: the single-window cache as originally written **did regress** the miss-heavy cases. Especially, on uniform-over-95-years, it was slower than the uncached path at every level.
So I reworked the miss path with three changes:
1. **A shared per-JVM transition table**: Each zone's transitions are materialized once into primitive arrays (the sorted transition epoch-seconds and the offset in effect on each interval) and shared read-only across tasks. A miss is now a single binary search over those arrays and cheaper than a bare `getOffset`.
2. **A read-only candidate lookup for the DST-cross guard**: The guard resolves the truncated candidate without installing it as the cached window, so a candidate that falls in a neighbouring window (common for coarse levels, e.g. `YEAR` truncating back to Jan 1) no longer evicts the working window and thrashes the next row.
3. **Reusing the truncated day on the DST-cross fallback**: When the guard does fall back (the truncation boundary crosses a transition), the date levels (WEEK/MONTH/QUARTER/YEAR) map the already-computed truncated local day straight back through the zone with `daysToMicros`, instead of re-deriving it via a second `ZonedDateTime` in the reference slow path.
Benchmarks (`America/Los_Angeles`, N=10M, whole-stage codegen on, ns/row):
| level | input | Before PR (no cache) | ZoneOffsetCache | ZoneOffsetCache + 1 + 2 + 3 |
|-------|-------|---------------------:|----------------:|----------------------------:|
| YEAR | clustered | 93.3 | 57.2 | 54.5 |
| YEAR | shuffled 2y | 257.0 | 369.7 | 123.6 |
| YEAR | spread 95y | 268.1 | 416.5 | 219.9 |
| MONTH | clustered | 93.4 | 56.0 | 55.9 |
| MONTH | shuffled 2y | 179.1 | 157.1 | 86.3 |
| MONTH | spread 95y | 153.5 | 202.0 | 99.7 |
| DAY | clustered | 68.2 | 38.2 | 37.8 |
| DAY | shuffled 2y | 127.4 | 86.2 | 47.9 |
| DAY | spread 95y | 107.7 | 152.3 | 63.9 |
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SQL] Speed up date_trunc zone offset resolution with a per-task cache [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #56848:
URL: https://github.com/apache/spark/pull/56848#issuecomment-5530725307
Also, @Licht-T please merge latest master
```
Unable to re-run one or more workflows. Check if the workflows are already running, are more than 30 days old, or are disabled.
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SQL] Cache zone offset per task in date_trunc [spark]
Posted by "Licht-T (via GitHub)" <gi...@apache.org>.
Licht-T commented on code in PR #56848:
URL: https://github.com/apache/spark/pull/56848#discussion_r3499408440
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala:
##########
@@ -1287,3 +1298,60 @@ object DateTimeUtils extends SparkDateTimeUtils {
c
}
}
+
+/**
+ * Per-task memoization of a zone's UTC offset, used by the [[DateTimeUtils.truncTimestamp]] hot
+ * path. The session zone is constant for a query and the offset is piecewise-constant between DST
+ * transitions, so consecutive rows almost always resolve to the same offset. The cache holds the
+ * half-open epoch-second interval `[lo, hi)` on which the offset is provably constant -- derived
+ * from the surrounding zone transitions -- so a lookup that falls in the interval reduces to two
+ * comparisons instead of a transition-array binary search.
+ *
+ * Not thread-safe by design: a fresh instance is created per task (codegen mutable state) and used
+ * single-threaded, mirroring how stateful per-row helpers are scoped in generated code.
+ */
+class ZoneOffsetCache(val zoneId: ZoneId) {
Review Comment:
Thanks @MaxGekk. As per your guidance, just added tests for `ZoneOffsetCache`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SPARK-57769][SQL] Speed up date_trunc zone offset resolution with a per-task cache [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #56848:
URL: https://github.com/apache/spark/pull/56848#issuecomment-5571043110
Also, this is a fairly big rewrite - adding @stevomitric to also take a look and help with review
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SQL] Speed up date_trunc zone offset resolution with a per-task cache [spark]
Posted by "Licht-T (via GitHub)" <gi...@apache.org>.
Licht-T commented on PR #56848:
URL: https://github.com/apache/spark/pull/56848#issuecomment-5193881728
Hi @MaxGekk and @uros-b, is there anything I can help on this? This is a blocker to close SPARK-57769: addressing a regression in SPARK-56769. I believe the the 4.3 branch-cut was made last week it will ship that bug which causes inconsistency at date partitioning (describled at https://github.com/apache/spark/pull/56908#pullrequestreview-4607554590).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SQL] Speed up date_trunc zone offset resolution with a per-task cache [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #56848:
URL: https://github.com/apache/spark/pull/56848#issuecomment-5530706789
Suggestions:
1. Left edge of the transition table is unbounded; the right edge is not
ZoneOffsetCache materializes transitions from 1600-01-01Z to 2200-01-01Z. Instants >= horizonSec go back to ZoneRules. Instants before the first table transition use offBefore0 all the way to -inf.
Current TZDB has no transitions before 1600, so this is fine today. The right-hand cut is already defensive; the left is an implicit TZDB invariant. Either:
walk from year 1 / Instant.MIN (cheap: first transition is typically 19th-century LMT), or
store tableStartSec and treat epochSec < tableStartSec like the horizon (consult ZoneRules).
The “before the first transition” cache test uses 1700-06-01, which does not exercise a pre-1600 instant.
2. truncTimestamp Scaladoc is stale
It still says the DST-cross path always falls back to truncTimestampSlow on offset mismatch. Date-level now:
falls back to daysToMicros(truncatedDays, zone) (not truncTimestampSlow);
also falls back when offsets match but the candidate sits in the fall-back overlap (SPARK-57769).
3. SPARK-57769 is only tested through DateTimeUtils, not TruncTimestamp
checkEvaluation in DateExpressionsSuite runs interpreted + codegen. A small expression-level case (Havana 2015 month or Berlin 1916 quarter is enough) would lock the addMutableState wiring. WEEK has no dedicated midnight-overlap fixture; Licht-T’s scan found Monday overlaps in many zones.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SQL] Speed up date_trunc zone offset resolution with a per-task cache [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #56848:
URL: https://github.com/apache/spark/pull/56848#issuecomment-5530701580
The template still says “Does this PR introduce any user-facing change? No.” That is no longer true.
6f2fd53 changes date_trunc for WEEK / MONTH / QUARTER / YEAR when the truncated midnight is repeated at a DST/LMT fall-back. Two timestamps in the same period that used to truncate to different instants now both get the earlier one. That is the intended SPARK-56769 regression fix, and it is user-facing (partition / GROUP BY keys).
Please:
- Flip that section to Yes and describe the overlap case (the Berlin 1916 / Havana 2015 / Tokyo 1888 examples already in the tests are enough).
- Mention SPARK-57769 in the title, e.g. [SPARK-57737][SPARK-57769][SQL] ....
- Treat [#56908](https://github.com/apache/spark/pull/56908) as superseded (config, default unchanged) so the two PRs do not land as contradictory defaults.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SPARK-57769][SQL] Speed up date_trunc zone offset resolution with a per-task cache [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #56848:
URL: https://github.com/apache/spark/pull/56848#issuecomment-5571018138
Still can't seem to run workflows here, @Licht-T could you please check?
```
Unable to re-run one or more workflows. Check if the workflows are already running, are more than 30 days old, or are disabled.
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SPARK-57769][SQL] Speed up date_trunc zone offset resolution with a per-task cache [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on code in PR #56848:
URL: https://github.com/apache/spark/pull/56848#discussion_r3949973292
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala:
##########
@@ -1393,3 +1448,235 @@ object DateTimeUtils extends SparkDateTimeUtils {
c
}
}
+
+/**
+ * A zone's transition schedule as primitive arrays: the sorted epoch-second transition instants
+ * (`transSec`) and the UTC offset (in seconds) in effect on each window `[transSec(i),
+ * transSec(i + 1))` (`offAfter(i)`); `offBefore0` is the offset before the first transition. Built
+ * once per zone from the transitions in `[startSec, horizonSec)` -- historical plus
+ * rule-generated; outside that range on either side the rules are consulted directly rather than
+ * assuming the table's edge windows extend indefinitely. Immutable and shared read-only across
+ * tasks via [[ZoneOffsetCache.tableFor]].
+ */
+private[util] final class ZoneTransitionTable(
+ val transSec: Array[Long],
+ val offAfter: Array[Int],
+ val offBefore0: Int,
+ val startSec: Long,
+ val horizonSec: Long) {
+
+ /** Largest `i` with `transSec(i) <= epochSec`, or -1 when before the first transition. */
+ def floorIndex(epochSec: Long): Int = {
+ var lo = 0
+ var hi = transSec.length - 1
+ var res = -1
+ while (lo <= hi) {
+ val mid = (lo + hi) >>> 1
+ if (transSec(mid) <= epochSec) {
+ res = mid
+ lo = mid + 1
+ } else {
+ hi = mid - 1
+ }
+ }
+ res
+ }
+}
+
+object ZoneOffsetCache {
+ // The transition table is the same for every task using a given zone, so build it once per JVM.
+ private val tables = new java.util.concurrent.ConcurrentHashMap[ZoneId, ZoneTransitionTable]()
+ private val tableStart = Instant.parse("1600-01-01T00:00:00Z")
+ private val tableHorizon = Instant.parse("2200-01-01T00:00:00Z")
+
+ private[util] def tableFor(zoneId: ZoneId): ZoneTransitionTable =
+ tables.computeIfAbsent(zoneId, z => buildTable(z))
+
+ private def buildTable(zoneId: ZoneId): ZoneTransitionTable = {
+ val rules = zoneId.getRules
+ val secs = scala.collection.mutable.ArrayBuffer.empty[Long]
+ val offs = scala.collection.mutable.ArrayBuffer.empty[Int]
+ var before0 = 0
+ var seen = false
+ var cur = tableStart
+ var t = rules.nextTransition(cur)
+ while (t != null && t.getInstant.isBefore(tableHorizon)) {
+ if (!seen) {
+ before0 = t.getOffsetBefore.getTotalSeconds
+ seen = true
+ }
+ secs += t.toEpochSecond
+ offs += t.getOffsetAfter.getTotalSeconds
+ // `plusNanos(1)` guarantees progress; transitions are >= 1s apart so none is skipped.
+ cur = t.getInstant.plusNanos(1)
+ t = rules.nextTransition(cur)
+ }
+ if (!seen) {
+ before0 = rules.getOffset(tableStart).getTotalSeconds
+ }
+ new ZoneTransitionTable(secs.toArray, offs.toArray, before0, tableStart.getEpochSecond,
+ tableHorizon.getEpochSecond)
+ }
+}
+
+/**
+ * Per-task memoization of a zone's UTC offset, used by the [[DateTimeUtils.truncTimestamp]] hot
+ * path. The session zone is constant for a query and the offset is piecewise-constant between DST
+ * transitions, so a lookup reduces to a range check against a cached constant-offset window
+ * `[lo, hi)`.
+ *
+ * The most-recently-used window is held in plain fields, a branch-only fast path that temporally
+ * clustered rows -- the common case -- keep hitting. A miss resolves the enclosing window
+ * with a single binary search over the shared [[ZoneTransitionTable]] (no allocation), which is
+ * itself cheaper than a bare `getOffset`, so even miss-heavy inputs (e.g. instants scattered over
+ * many decades in random order) stay close to the uncached path.
+ *
+ * Not thread-safe by design: a fresh instance is created per task (codegen mutable state) and used
+ * single-threaded, mirroring how stateful per-row helpers are scoped in generated code.
+ */
+class ZoneOffsetCache(val zoneId: ZoneId) {
Review Comment:
This class, and the cache-taking `truncTimestamp`/`truncTimestampNanos` overloads are still `public`, whereas a previous review iteration asked to scope this not-thread-safe, per-task-only helper `private[sql]` (still module-visible, so the generated Java's FQN reference keeps working).
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SQL] Speed up date_trunc zone offset resolution with a per-task cache [spark]
Posted by "Licht-T (via GitHub)" <gi...@apache.org>.
Licht-T commented on PR #56848:
URL: https://github.com/apache/spark/pull/56848#issuecomment-5141413364
Rerunning the bench.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SQL] Cache zone offset per task in date_trunc [spark]
Posted by "Licht-T (via GitHub)" <gi...@apache.org>.
Licht-T commented on code in PR #56848:
URL: https://github.com/apache/spark/pull/56848#discussion_r3507249092
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala:
##########
@@ -1287,3 +1298,60 @@ object DateTimeUtils extends SparkDateTimeUtils {
c
}
}
+
+/**
+ * Per-task memoization of a zone's UTC offset, used by the [[DateTimeUtils.truncTimestamp]] hot
+ * path. The session zone is constant for a query and the offset is piecewise-constant between DST
+ * transitions, so consecutive rows almost always resolve to the same offset. The cache holds the
+ * half-open epoch-second interval `[lo, hi)` on which the offset is provably constant -- derived
+ * from the surrounding zone transitions -- so a lookup that falls in the interval reduces to two
+ * comparisons instead of a transition-array binary search.
+ *
+ * Not thread-safe by design: a fresh instance is created per task (codegen mutable state) and used
+ * single-threaded, mirroring how stateful per-row helpers are scoped in generated code.
+ */
+class ZoneOffsetCache(val zoneId: ZoneId) {
+ private val rules = zoneId.getRules
+ val isFixedOffset: Boolean = rules.isFixedOffset
+
+ private var cached = false
+ private var lo = 0L
+ private var hi = 0L
+ private var offsetSec = 0L
+
+ /**
+ * Offset in seconds at `epochSec`, equal to
+ * `rules.getOffset(Instant.ofEpochSecond(epochSec)).getTotalSeconds`, memoized over the
+ * constant-offset interval containing the previous lookup.
+ */
+ def offsetSeconds(epochSec: Long): Long = {
+ if (cached && epochSec >= lo && epochSec < hi) {
+ offsetSec
+ } else {
+ val instant = Instant.ofEpochSecond(epochSec)
+ val o = rules.getOffset(instant).getTotalSeconds.toLong
+ if (isFixedOffset) {
+ lo = Long.MinValue
+ hi = Long.MaxValue
+ } else {
+ val nextT = rules.nextTransition(instant)
+ if (nextT == null) {
+ // No transition after `instant`: offset is constant on [epochSec, +inf).
+ lo = epochSec
+ hi = Long.MaxValue
+ } else {
+ hi = nextT.toEpochSecond
+ // `hi - 1` lies strictly inside the constant-offset window ending at `hi` (zone
+ // transitions are always more than a second apart), so its previous transition is
+ // exactly that window's start. Anchoring on an interior point avoids an off-by-one
+ // when `epochSec` sits exactly on a transition instant.
+ val prevT = rules.previousTransition(Instant.ofEpochSecond(hi - 1))
Review Comment:
`java.time`'s `ZoneRules` stores transition instants as a **strictly-increasing array of epoch-seconds** (`savingsInstantTransitions: long[]`), mirroring how TZif / RFC 8536 encodes transition times (a sorted list of integer seconds). So, the >= 1s gap is always guaranteed. I added this as a code comment.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SQL] Speed up date_trunc zone offset resolution with a per-task cache [spark]
Posted by "Licht-T (via GitHub)" <gi...@apache.org>.
Licht-T commented on PR #56848:
URL: https://github.com/apache/spark/pull/56848#issuecomment-5477589020
@MaxGekk @uros-b gently ping
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SQL] Cache zone offset per task in date_trunc [spark]
Posted by "Licht-T (via GitHub)" <gi...@apache.org>.
Licht-T commented on code in PR #56848:
URL: https://github.com/apache/spark/pull/56848#discussion_r3639172444
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala:
##########
@@ -1287,3 +1298,60 @@ object DateTimeUtils extends SparkDateTimeUtils {
c
}
}
+
+/**
+ * Per-task memoization of a zone's UTC offset, used by the [[DateTimeUtils.truncTimestamp]] hot
+ * path. The session zone is constant for a query and the offset is piecewise-constant between DST
+ * transitions, so consecutive rows almost always resolve to the same offset. The cache holds the
+ * half-open epoch-second interval `[lo, hi)` on which the offset is provably constant -- derived
+ * from the surrounding zone transitions -- so a lookup that falls in the interval reduces to two
+ * comparisons instead of a transition-array binary search.
+ *
+ * Not thread-safe by design: a fresh instance is created per task (codegen mutable state) and used
+ * single-threaded, mirroring how stateful per-row helpers are scoped in generated code.
+ */
+class ZoneOffsetCache(val zoneId: ZoneId) {
+ private val rules = zoneId.getRules
+ val isFixedOffset: Boolean = rules.isFixedOffset
+
+ private var cached = false
+ private var lo = 0L
+ private var hi = 0L
+ private var offsetSec = 0L
+
+ /**
+ * Offset in seconds at `epochSec`, equal to
+ * `rules.getOffset(Instant.ofEpochSecond(epochSec)).getTotalSeconds`, memoized over the
+ * constant-offset interval containing the previous lookup.
+ */
+ def offsetSeconds(epochSec: Long): Long = {
+ if (cached && epochSec >= lo && epochSec < hi) {
+ offsetSec
+ } else {
+ val instant = Instant.ofEpochSecond(epochSec)
+ val o = rules.getOffset(instant).getTotalSeconds.toLong
+ if (isFixedOffset) {
+ lo = Long.MinValue
+ hi = Long.MaxValue
+ } else {
+ val nextT = rules.nextTransition(instant)
Review Comment:
Good catch. The benchmarked input (`timestamp_seconds(id)`) is monotonic and never misses, so it hid exactly this. I added two miss-heavy `date_trunc` cases to `DateTimeBenchmark`: timestamps shuffled over a bounded ~2-year span, and spread uniformly over ~95 years.
Those confirmed your prediction: the single-window cache as originally written **did regress** the miss-heavy cases. Especially, on uniform-over-95-years, it was slower than the uncached path at every level.
So I reworked the miss path with three changes:
1. **A shared per-JVM transition table**: Each zone's transitions are materialized once into primitive arrays (the sorted transition epoch-seconds and the offset in effect on each interval) and shared read-only across tasks. A miss is now a single binary search over those arrays and cheaper than a bare `getOffset`.
2. **A read-only candidate lookup for the DST-cross guard**: The guard resolves the truncated candidate without installing it as the cached window, so a candidate that falls in a neighbouring window (common for coarse levels, e.g. `YEAR` truncating back to Jan 1) no longer evicts the working window and thrashes the next row.
3. **Reusing the truncated day on the DST-cross fallback**: When the guard does fall back (the truncation boundary crosses a transition), the date levels (WEEK/MONTH/QUARTER/YEAR) map the already-computed truncated local day straight back through the zone with `daysToMicros`, instead of re-deriving it via a second `ZonedDateTime` in the reference slow path.
Benchmarks (`America/Los_Angeles`, N=10M, whole-stage codegen on, ns/row):
| level | input | Before PR (no cache) | ZoneOffsetCache | ZoneOffsetCache + 1 + 2 + 3 |
|-------|-------|---------------------:|----------------:|----------------------------:|
| YEAR | clustered | 99.1 | 56.4 | 53.5 |
| YEAR | shuffled 2y | 248.1 | 352.4 | 115.3 |
| YEAR | spread 95y | 268.9 | 410.2 | 217.4 |
| QUARTER | clustered | 112.5 | 70.0 | 69.2 |
| QUARTER | shuffled 2y | 213.6 | 210.7 | 114.5 |
| QUARTER | spread 95y | 233.5 | 293.4 | 172.9 |
| MONTH | clustered | 95.7 | 54.2 | 52.9 |
| MONTH | shuffled 2y | 169.8 | 152.6 | 82.0 |
| MONTH | spread 95y | 150.0 | 196.7 | 93.9 |
| WEEK | clustered | 77.0 | 40.9 | 39.1 |
| WEEK | shuffled 2y | 131.3 | 88.5 | 50.6 |
| WEEK | spread 95y | 113.5 | 153.6 | 62.7 |
| DAY | clustered | 65.9 | 36.8 | 32.8 |
| DAY | shuffled 2y | 115.4 | 85.7 | 45.0 |
| DAY | spread 95y | 102.4 | 151.0 | 56.9 |
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SQL] Cache zone offset per task in date_trunc [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #56848:
URL: https://github.com/apache/spark/pull/56848#discussion_r3505187374
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala:
##########
@@ -1287,3 +1298,60 @@ object DateTimeUtils extends SparkDateTimeUtils {
c
}
}
+
+/**
+ * Per-task memoization of a zone's UTC offset, used by the [[DateTimeUtils.truncTimestamp]] hot
+ * path. The session zone is constant for a query and the offset is piecewise-constant between DST
+ * transitions, so consecutive rows almost always resolve to the same offset. The cache holds the
+ * half-open epoch-second interval `[lo, hi)` on which the offset is provably constant -- derived
+ * from the surrounding zone transitions -- so a lookup that falls in the interval reduces to two
+ * comparisons instead of a transition-array binary search.
+ *
+ * Not thread-safe by design: a fresh instance is created per task (codegen mutable state) and used
+ * single-threaded, mirroring how stateful per-row helpers are scoped in generated code.
+ */
+class ZoneOffsetCache(val zoneId: ZoneId) {
+ private val rules = zoneId.getRules
+ val isFixedOffset: Boolean = rules.isFixedOffset
+
+ private var cached = false
+ private var lo = 0L
+ private var hi = 0L
+ private var offsetSec = 0L
+
+ /**
+ * Offset in seconds at `epochSec`, equal to
+ * `rules.getOffset(Instant.ofEpochSecond(epochSec)).getTotalSeconds`, memoized over the
+ * constant-offset interval containing the previous lookup.
+ */
+ def offsetSeconds(epochSec: Long): Long = {
+ if (cached && epochSec >= lo && epochSec < hi) {
+ offsetSec
+ } else {
+ val instant = Instant.ofEpochSecond(epochSec)
+ val o = rules.getOffset(instant).getTotalSeconds.toLong
+ if (isFixedOffset) {
+ lo = Long.MinValue
+ hi = Long.MaxValue
+ } else {
+ val nextT = rules.nextTransition(instant)
Review Comment:
Question (non-blocking): on a cache *miss* this path now does three JDK rule searches — `getOffset` + `nextTransition` + `previousTransition` — versus the single `getOffset` before this PR. For temporally clustered data (the benchmarked case) that's a clear win, but a workload that straddles a transition on nearly every row, or randomly-distributed timestamps across a zone with many historical transitions, misses every row and would be a net regression vs the old path.
Did the benchmarks include a miss-heavy / random-timestamp case, or is that worst case considered acceptable?
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala:
##########
@@ -1287,3 +1298,60 @@ object DateTimeUtils extends SparkDateTimeUtils {
c
}
}
+
+/**
+ * Per-task memoization of a zone's UTC offset, used by the [[DateTimeUtils.truncTimestamp]] hot
+ * path. The session zone is constant for a query and the offset is piecewise-constant between DST
+ * transitions, so consecutive rows almost always resolve to the same offset. The cache holds the
+ * half-open epoch-second interval `[lo, hi)` on which the offset is provably constant -- derived
+ * from the surrounding zone transitions -- so a lookup that falls in the interval reduces to two
+ * comparisons instead of a transition-array binary search.
+ *
+ * Not thread-safe by design: a fresh instance is created per task (codegen mutable state) and used
+ * single-threaded, mirroring how stateful per-row helpers are scoped in generated code.
+ */
+class ZoneOffsetCache(val zoneId: ZoneId) {
+ private val rules = zoneId.getRules
+ val isFixedOffset: Boolean = rules.isFixedOffset
+
+ private var cached = false
+ private var lo = 0L
+ private var hi = 0L
+ private var offsetSec = 0L
+
+ /**
+ * Offset in seconds at `epochSec`, equal to
+ * `rules.getOffset(Instant.ofEpochSecond(epochSec)).getTotalSeconds`, memoized over the
+ * constant-offset interval containing the previous lookup.
+ */
+ def offsetSeconds(epochSec: Long): Long = {
+ if (cached && epochSec >= lo && epochSec < hi) {
+ offsetSec
+ } else {
+ val instant = Instant.ofEpochSecond(epochSec)
+ val o = rules.getOffset(instant).getTotalSeconds.toLong
+ if (isFixedOffset) {
+ lo = Long.MinValue
+ hi = Long.MaxValue
+ } else {
+ val nextT = rules.nextTransition(instant)
+ if (nextT == null) {
+ // No transition after `instant`: offset is constant on [epochSec, +inf).
+ lo = epochSec
+ hi = Long.MaxValue
+ } else {
+ hi = nextT.toEpochSecond
+ // `hi - 1` lies strictly inside the constant-offset window ending at `hi` (zone
+ // transitions are always more than a second apart), so its previous transition is
+ // exactly that window's start. Anchoring on an interior point avoids an off-by-one
+ // when `epochSec` sits exactly on a transition instant.
+ val prevT = rules.previousTransition(Instant.ofEpochSecond(hi - 1))
Review Comment:
Question (non-blocking): this interior-anchor makes `[lo, hi)` correct only if `hi - 1` is guaranteed inside the constant-offset window — i.e. no two zone transitions are ever <= 1s apart. Is that property guaranteed across the whole tzdb, including historical / LMT transitions?
What makes me want it confirmed rather than assumed: the fast path's `candidateOffsetSec != originalOffsetSec` fallback only guards a truncation that *spans* a transition. It does **not** backstop a wrong `originalOffsetSec` — if the window ever contained a transition, both the original and candidate lookups would read the same wrong cached offset, the guard would compare equal, and the wrong result would ship silently. So this invariant is load-bearing on its own. If the >= 1s gap can't be guaranteed, could we degrade defensively (e.g. narrow the window / fall back) when it's violated?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SQL] Speed up date_trunc zone offset resolution with a per-task cache [spark]
Posted by "Licht-T (via GitHub)" <gi...@apache.org>.
Licht-T commented on PR #56848:
URL: https://github.com/apache/spark/pull/56848#issuecomment-5141410776
@uros-b Resolved conflicts and pushed a patch for SPARK-57769 (https://github.com/apache/spark/pull/56908): https://github.com/apache/spark/pull/56848/changes/6f2fd53117e7b3a7884bdb13d6f4960aea8265e0.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SPARK-57769][SQL] Speed up date_trunc zone offset resolution with a per-task cache [spark]
Posted by "Licht-T (via GitHub)" <gi...@apache.org>.
Licht-T commented on PR #56848:
URL: https://github.com/apache/spark/pull/56848#issuecomment-5571431019
@uros-b Thanks for reviewing!
> The template still says "Does this PR introduce any user-facing change? No." That is no longer true.
> Mention SPARK-57769 in the title
> Treat #56908 as superseded
Done.
> 1. Left edge of the transition table is unbounded; the right edge is not
Fixed with your second option.
> 2. truncTimestamp Scaladoc is stale
Rewritten.
> 3. SPARK-57769 is only tested through DateTimeUtils, not TruncTimestamp ... WEEK has no dedicated midnight-overlap fixture.
Added a `checkEvaluation` test (interpreted + codegen) over the Havana MONTH and Berlin QUARTER overlaps, plus a WEEK fixture: `Asia/Jerusalem` 2001-09-24, a Monday-midnight fall-back. The same fixture is added to the `DateTimeUtilsSuite` regression test.
> 1. The on-transition unit test still describes the old previousTransition(hi - 1) anchor.
Fixed.
> 2. Interpreted eval still does new ZoneOffsetCache(zoneId) per row ... Please verify.
Verified safe: the instance is confined to the `eval` call stack and never escapes onto the expression, so concurrent evaluation shares nothing mutable; the only cross-thread state is the `ConcurrentHashMap` of immutable transition tables.
> Still can't seem to run workflows here
Rebased with master and pushed a new commit. Hope this works.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-57737][SPARK-57769][SQL] Speed up date_trunc zone offset resolution with a per-task cache [spark]
Posted by "Licht-T (via GitHub)" <gi...@apache.org>.
Licht-T commented on code in PR #56848:
URL: https://github.com/apache/spark/pull/56848#discussion_r3950221445
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala:
##########
@@ -1393,3 +1448,235 @@ object DateTimeUtils extends SparkDateTimeUtils {
c
}
}
+
+/**
+ * A zone's transition schedule as primitive arrays: the sorted epoch-second transition instants
+ * (`transSec`) and the UTC offset (in seconds) in effect on each window `[transSec(i),
+ * transSec(i + 1))` (`offAfter(i)`); `offBefore0` is the offset before the first transition. Built
+ * once per zone from the transitions in `[startSec, horizonSec)` -- historical plus
+ * rule-generated; outside that range on either side the rules are consulted directly rather than
+ * assuming the table's edge windows extend indefinitely. Immutable and shared read-only across
+ * tasks via [[ZoneOffsetCache.tableFor]].
+ */
+private[util] final class ZoneTransitionTable(
+ val transSec: Array[Long],
+ val offAfter: Array[Int],
+ val offBefore0: Int,
+ val startSec: Long,
+ val horizonSec: Long) {
+
+ /** Largest `i` with `transSec(i) <= epochSec`, or -1 when before the first transition. */
+ def floorIndex(epochSec: Long): Int = {
+ var lo = 0
+ var hi = transSec.length - 1
+ var res = -1
+ while (lo <= hi) {
+ val mid = (lo + hi) >>> 1
+ if (transSec(mid) <= epochSec) {
+ res = mid
+ lo = mid + 1
+ } else {
+ hi = mid - 1
+ }
+ }
+ res
+ }
+}
+
+object ZoneOffsetCache {
+ // The transition table is the same for every task using a given zone, so build it once per JVM.
+ private val tables = new java.util.concurrent.ConcurrentHashMap[ZoneId, ZoneTransitionTable]()
+ private val tableStart = Instant.parse("1600-01-01T00:00:00Z")
+ private val tableHorizon = Instant.parse("2200-01-01T00:00:00Z")
+
+ private[util] def tableFor(zoneId: ZoneId): ZoneTransitionTable =
+ tables.computeIfAbsent(zoneId, z => buildTable(z))
+
+ private def buildTable(zoneId: ZoneId): ZoneTransitionTable = {
+ val rules = zoneId.getRules
+ val secs = scala.collection.mutable.ArrayBuffer.empty[Long]
+ val offs = scala.collection.mutable.ArrayBuffer.empty[Int]
+ var before0 = 0
+ var seen = false
+ var cur = tableStart
+ var t = rules.nextTransition(cur)
+ while (t != null && t.getInstant.isBefore(tableHorizon)) {
+ if (!seen) {
+ before0 = t.getOffsetBefore.getTotalSeconds
+ seen = true
+ }
+ secs += t.toEpochSecond
+ offs += t.getOffsetAfter.getTotalSeconds
+ // `plusNanos(1)` guarantees progress; transitions are >= 1s apart so none is skipped.
+ cur = t.getInstant.plusNanos(1)
+ t = rules.nextTransition(cur)
+ }
+ if (!seen) {
+ before0 = rules.getOffset(tableStart).getTotalSeconds
+ }
+ new ZoneTransitionTable(secs.toArray, offs.toArray, before0, tableStart.getEpochSecond,
+ tableHorizon.getEpochSecond)
+ }
+}
+
+/**
+ * Per-task memoization of a zone's UTC offset, used by the [[DateTimeUtils.truncTimestamp]] hot
+ * path. The session zone is constant for a query and the offset is piecewise-constant between DST
+ * transitions, so a lookup reduces to a range check against a cached constant-offset window
+ * `[lo, hi)`.
+ *
+ * The most-recently-used window is held in plain fields, a branch-only fast path that temporally
+ * clustered rows -- the common case -- keep hitting. A miss resolves the enclosing window
+ * with a single binary search over the shared [[ZoneTransitionTable]] (no allocation), which is
+ * itself cheaper than a bare `getOffset`, so even miss-heavy inputs (e.g. instants scattered over
+ * many decades in random order) stay close to the uncached path.
+ *
+ * Not thread-safe by design: a fresh instance is created per task (codegen mutable state) and used
+ * single-threaded, mirroring how stateful per-row helpers are scoped in generated code.
+ */
+class ZoneOffsetCache(val zoneId: ZoneId) {
Review Comment:
Done, with one wrinkle: Scala emits no static forwarder for `private[sql]` members, which broke the generated code's `DateTimeUtils.truncTimestamp(...)` call (caught by the codegen tests). The `TruncInstant` codegen now calls through `DateTimeUtils$.MODULE$` instead.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org