seanmuth commented on code in PR #66132:
URL: https://github.com/apache/airflow/pull/66132#discussion_r3168885009
##########
airflow-core/src/airflow/timetables/interval.py:
##########
@@ -110,6 +110,15 @@ def next_dagrun_info(
else:
# Data interval starts from the end of the previous interval.
start = align_last_data_interval_end
+ # A zero-length previous interval (start == end) collapses
logical_date
+ # onto the existing run; advance one period so we don't re-emit it.
+ # This shape comes from CronTriggerTimetable runs scheduled before
a
+ # switch to a CronDataIntervalTimetable.
+ if (
+ last_automated_data_interval.start ==
last_automated_data_interval.end
+ and start == last_automated_data_interval.start
+ ):
+ start = self._get_next(start)
Review Comment:
claude's response:
```
⏺ Your reasoning is correct in outcome but slightly imprecise in
explanation. Let me walk through why the reviewer's concern doesn't apply.
After the guard fires and advances start = self._get_next(start),
execution continues to line 124 (end = self._get_next(start)). So the returned
interval is always DataInterval(_get_next(old_start),
_get_next(_get_next(old_start))). Since _get_next on a valid cron expression
always produces a strictly later
timestamp, start != end — the returned interval is guaranteed
non-zero-length.
The zero-length condition (start == end) can only arise from how
CronTriggerTimetable stores runs in the DB (it writes data_interval_start ==
data_interval_end). CronDataIntervalTimetable.next_dagrun_info itself can never
produce a zero-length interval because end is always _get_next(start).
So your response is correct — the guard only needs to fire once to step
past the legacy zero-length run, and there's no need for a loop or recursive
check. But you could sharpen it for the reviewer:
▎ The guard can only fire when the previous run was zero-length (a
CronTriggerTimetable-era run). After advancing start by one period, end =
self._get_next(start) (line 124) ensures the returned interval is always
non-zero — CronDataIntervalTimetable itself can never produce start == end. So
a second pass through
▎ the guard can't happen.
That removes the "I think?" uncertainty and directly addresses why
iteration isn't needed.
```
--
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]
