amoghrajesh commented on code in PR #75847:
URL: https://github.com/apache/airflow/pull/75847#discussion_r4410069665
##########
airflow-core/src/airflow/timetables/_cron.py:
##########
@@ -31,6 +31,27 @@
from pendulum.tz.timezone import FixedTimezone, Timezone
+# A wall-clock gap of one hour or more may span a DST transition, so only gaps
shorter than this limit
+# are added to ``current`` as elapsed time.
Review Comment:
Two problems with this comment and the constant.
The comment says a gap of an hour or more may span a DST change. A 50 minute
gap in `Australia/Lord_Howe` spans one too.
One hour is also too small for the look-back window. `Antarctica/Troll` runs
+00 in winter and +02 in summer, so its October fall-back repeats two hours of
wall clock. I swept Troll with 13 crons and found no failure, but only because
the branch needs the next tick an hour out, which rules out dense ticks in the
second hour. Nothing records that. Use two hours and name it for the largest
DST shift in tzdata, not a fold limit.
##########
airflow-core/src/airflow/timetables/_cron.py:
##########
@@ -172,6 +214,7 @@ def _get_prev(self, current: DateTime) -> DateTime:
scheduled = cron.get_prev(datetime.datetime)
if TYPE_CHECKING:
assert isinstance(scheduled, datetime.datetime)
+ delta = naive - scheduled
Review Comment:
`delta = naive - scheduled` moved above the `_covers_every_hour` check, but
that branch never reads it. Move it back below.
##########
airflow-core/tests/unit/timetables/test_cron_mixin.py:
##########
@@ -39,3 +44,209 @@ def test_dom_and_dow_conflict():
assert "(or)" in desc
assert "Every minute, on day 1 of the month" in desc
assert "Every minute, only on Monday" in desc
+
+
[email protected](
+ ("timezone", "friday_last_tick", "monday_first_tick"),
+ [
+ pytest.param(
+ "Europe/Berlin",
+ pendulum.datetime(2026, 10, 23, 21, 0, tz="UTC"), # Fri 23:00 CEST
+ pendulum.datetime(2026, 10, 25, 23, 0, tz="UTC"), # Mon 00:00 CET
+ id="berlin-fall-back",
+ ),
+ pytest.param(
+ "America/Chicago",
+ pendulum.datetime(2026, 10, 31, 4, 0, tz="UTC"), # Fri 2026-10-30
23:00 CDT
+ pendulum.datetime(2026, 11, 2, 6, 0, tz="UTC"), # Mon 00:00 CST
+ id="chicago-fall-back",
+ ),
+ pytest.param(
+ "Australia/Sydney",
+ pendulum.datetime(2026, 4, 3, 12, 0, tz="UTC"), # Fri 23:00 AEDT
+ pendulum.datetime(2026, 4, 5, 14, 0, tz="UTC"), # Mon 00:00 AEST
+ id="sydney-fall-back",
+ ),
+ pytest.param(
+ "Europe/Berlin",
+ pendulum.datetime(2027, 3, 26, 22, 0, tz="UTC"), # Fri 23:00 CET
+ pendulum.datetime(2027, 3, 28, 22, 0, tz="UTC"), # Mon 00:00 CEST
+ id="berlin-spring-forward",
+ ),
+ ],
+)
+def test_weekday_hourly_across_weekend_dst(
+ timezone: str, friday_last_tick: pendulum.DateTime, monday_first_tick:
pendulum.DateTime
+):
+ mixin = CronMixin("0 * * * 1-5", timezone)
+
+ assert mixin._get_next(friday_last_tick) == monday_first_tick
+ assert mixin._get_prev(monday_first_tick) == friday_last_tick
+
+
[email protected](
+ ("timezone", "cron", "current", "expected"),
+ [
+ pytest.param(
+ "America/Havana",
+ "0 * * * 0",
+ pendulum.datetime(2026, 10, 26, 3, 0, tz="UTC"),
+ # Sun 00:00 -04:00, first of the folded hour
+ pendulum.datetime(2026, 11, 1, 4, 0, tz="UTC"),
+ id="havana-target-in-fold-hour",
+ ),
+ pytest.param(
+ "Europe/Berlin",
+ "0 * * * *",
+ pendulum.datetime(2026, 10, 25, 0, 0, tz="UTC"),
+ pendulum.datetime(2026, 10, 25, 1, 0, tz="UTC"),
+ id="berlin-fold-first-step",
+ ),
+ pytest.param(
+ "Europe/Berlin",
+ "0 * * * *",
+ pendulum.datetime(2026, 10, 25, 1, 0, tz="UTC"),
+ pendulum.datetime(2026, 10, 25, 2, 0, tz="UTC"),
+ id="berlin-fold-second-step",
+ ),
+ pytest.param(
+ "America/Havana",
+ "0 * * * 0",
+ pendulum.datetime(2027, 3, 13, 4, 0, tz="UTC"),
+ # Sun 00:00 is skipped by the spring-forward gap, so the next tick
is the following hour
+ pendulum.datetime(2027, 3, 14, 5, 0, tz="UTC"),
+ id="havana-next-target-in-gap",
+ ),
+ pytest.param(
+ "America/Godthab",
+ "0 * * * 6",
+ pendulum.datetime(2026, 10, 25, 0, 0, tz="UTC"),
+ # Sat 23:00 -02:00 is followed by its fold twin Sat 23:00 -03:00
+ pendulum.datetime(2026, 10, 25, 1, 0, tz="UTC"),
+ id="godthab-next-fold-twin",
+ ),
+ pytest.param(
+ "America/Santiago",
+ "0 * * * 6",
+ pendulum.datetime(2026, 4, 5, 2, 0, tz="UTC"),
+ # Sat 23:00 -03:00 is followed by its fold twin Sat 23:00 -04:00
+ pendulum.datetime(2026, 4, 5, 3, 0, tz="UTC"),
+ id="santiago-next-fold-twin",
+ ),
+ pytest.param(
+ "America/Godthab",
+ "0,30 * * * 6",
+ pendulum.datetime(2026, 10, 25, 0, 30, tz="UTC"),
+ # Sat 23:00 -02:00 (second pass of the fold hour) comes before the
twin Sat 23:30 -02:00
+ pendulum.datetime(2026, 10, 25, 1, 0, tz="UTC"),
+ id="godthab-next-half-hour-fold",
+ ),
+ ],
+)
+def test_get_next_across_dst(
Review Comment:
Should also test `Australia/Lord_Howe`
Worth adding Antarctica/Troll too, for the two-hour fold.
##########
airflow-core/src/airflow/timetables/_cron.py:
##########
@@ -153,6 +174,18 @@ def localize_partition_datetime(self, dt:
datetime.datetime) -> DateTime:
"""
return convert_to_utc(make_aware(dt.replace(tzinfo=None),
self._timezone))
+ def _localized_ticks_within_limit(self, naive: datetime.datetime, *,
before: bool) -> list[DateTime]:
+ """Localize every cron tick within ``_FOLD_DELTA_LIMIT`` on one side
of ``naive``, ``naive`` included."""
+ nudge = datetime.timedelta(microseconds=1)
+ cron = croniter(self._expression, start_time=naive + nudge if before
else naive - nudge)
+ step = cron.get_prev if before else cron.get_next
+ localized: list[DateTime] = []
+ tick = step(datetime.datetime)
+ while abs(tick - naive) <= _FOLD_DELTA_LIMIT:
+ localized += _localize_all_folds(tick, self._timezone)
+ tick = step(datetime.datetime)
+ return localized
+
Review Comment:
`0 * * * *` has delta of exactly one hour, so it takes this path on every
call, all year. I measured `_get_next` at `31.8 us` on main and `83.3 us` here,
`2.6x`. Hourly crons are common, and the scheduler calls this a lot.
An offset check before the scan brings it back to `30.8 us`, because the
scan only runs on the two days a year it is needed.
##########
airflow-core/src/airflow/timetables/_cron.py:
##########
@@ -162,7 +195,16 @@ def _get_next(self, current: DateTime) -> DateTime:
assert isinstance(scheduled, datetime.datetime)
if not _covers_every_hour(cron):
return convert_to_utc(make_aware(scheduled, self._timezone))
+
delta = scheduled - naive
+ if delta >= _FOLD_DELTA_LIMIT:
+ # Ticks up to one limit before ``naive`` may have a later fold
occurrence after ``current``.
Review Comment:
These comments are mirrored pairs. Line 201 and line 228 say the same thing,
and so do
203 and 230. Keep one of each.
##########
airflow-core/src/airflow/timetables/_cron.py:
##########
@@ -162,7 +195,16 @@ def _get_next(self, current: DateTime) -> DateTime:
assert isinstance(scheduled, datetime.datetime)
if not _covers_every_hour(cron):
return convert_to_utc(make_aware(scheduled, self._timezone))
+
delta = scheduled - naive
+ if delta >= _FOLD_DELTA_LIMIT:
Review Comment:
This branch assumes a gap under an hour cannot cross a DST change.
`Australia/Lord_Howe` shifts by 30 minutes, so it can.
Repro: `0,10 * * * 0` in `Australia/Lord_Howe`, current `2026-04-04T14:10Z`
(local `Sun 01:10 +11:00`). True next is `2026-04-04T15:30Z`. Both main and
this branch give `2026-04-04T15:00Z`. Same for `0,45 * * * 0`, and again at the
October transition.
Suggest dropping the branch and gating on the zone instead: if the UTC
offset is the same at both ends of the window, the old elapsed time shortcut is
exact, so take it; otherwise run the fold search.
I ran both versions against minute by minute truth, `21,388` checks over `8`
zones and `14` cron expressions: main has 6 mismatches, this PR has 8, offset
gated version 0.
--
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]
