Fix create_delta_data_intervals being ignored for timedelta schedules - #69869
Fix create_delta_data_intervals being ignored for timedelta schedules#69869sharetheknowledge wants to merge 8 commits into
Conversation
_create_timetable() checked create_cron_data_intervals for both the cron-string branch and the timedelta/relativedelta branch, so create_delta_data_intervals had no effect and DAGs with a timedelta schedule silently followed the cron config instead. Closes apache#69868
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
potiuk
added
the
ready for maintainer review
| return ContinuousTimetable() | ||
| if isinstance(interval, timedelta | relativedelta): | ||
| if airflow_conf.getboolean("scheduler", "create_cron_data_intervals"): | ||
| if airflow_conf.getboolean("scheduler", "create_delta_data_intervals"): |
There was a problem hiding this comment.
The one-line fix is correct, but it silently changes behavior for the cohort our own upgrade guide created: upgrading_to_airflow3.rst tells 2.x migrants to set create_cron_data_intervals = True to keep data-interval semantics, and because of this bug that flag currently also pins timedelta/relativedelta DAGs to DeltaDataIntervalTimetable, which matches Airflow 2 behavior. After this change those DAGs fall through to DeltaTriggerTimetable on the next parse, so logical_date becomes the trigger time and ds, ts and data_interval_* all shift by one period (no run is skipped in that direction, but nothing warns the user). The same applies to Airflow 2 serialized DAGs converted through conversion_v1_to_v2, which rebuilds the timetable via this function.
Can you add a 69869.significant.rst newsfragment naming who is affected and the remedy? Setting [scheduler] create_delta_data_intervals = True is a no-op on current main (the key is read nowhere), so users can safely set it before upgrading to keep current behavior. The docs need the same treatment in this PR: a bullet in upgrading_to_airflow3.rst next to the cron one, this key in the "Switching between trigger and data interval timetables" section of timetable.rst, and the skip-one-period paragraph the cron entry has in config.yml but the delta entry lacks (flipping this key from False to True after Airflow 3 runs exist skips one period, same collision guard).
| from airflow.sdk.definitions.timetables.interval import DeltaDataIntervalTimetable | ||
| from airflow.sdk.definitions.timetables.trigger import DeltaTriggerTimetable | ||
|
|
||
| from tests_common.test_utils.config import conf_vars |
There was a problem hiding this comment.
Can you hoist this to the top of the file? The sibling files here (test_connection.py, test_variables.py, test_operator_resources.py) all import conf_vars at module level, and there is no circular-import reason for it to live in the function body.
| with pytest.raises(ValueError, match="ContinuousTimetable requires max_active_runs <= 1"): | ||
| dag = DAG("continuous", start_date=DEFAULT_DATE, schedule="@continuous", max_active_runs=25) | ||
|
|
||
| def test_timedelta_schedule_respects_create_delta_data_intervals_config(self): |
There was a problem hiding this comment.
Consider @pytest.mark.parametrize over (delta, cron, expected) here: if the first assert fails, pytest never runs the other two scenarios, and this file already uses parametrize for similar multi-case checks. A relativedelta row would also back up the docstring, which claims relativedelta coverage while only timedelta is exercised.
Per kaxil review: sibling files (test_connection.py, test_variables.py, test_operator_resources.py) all import conf_vars at module level. No circular-import reason for it to live in the function body.
Per kaxil review: convert the three inline conf_vars blocks to a single @pytest.mark.parametrize test. Stack a schedule parametrize to also cover relativedelta, giving 6 test cases (3 config combos x timedelta/relativedelta). Hoist DeltaDataIntervalTimetable, DeltaTriggerTimetable, and relativedelta to module level (required for parametrize decorator to reference them).
3 participants
Fixes a bug where
create_delta_data_intervalshad no effect on DAGs with atimedelta/relativedeltaschedule._create_timetable()intask-sdk/src/airflow/sdk/definitions/dag.pycheckedcreate_cron_data_intervalsfor both the cron-string branch and thetimedelta/relativedeltabranch. As a result:create_delta_data_intervalshad no effect at all — it's referenced nowhereelse in the codebase besides CLI config-list metadata and docs.
create_cron_data_intervalssilently controlled timetable selection fortimedelta/relativedeltaschedules too, which isn't what it's documentedto do (
config.ymldescribes it as governing only cron-string schedules).The
timedelta | relativedeltabranch now checkscreate_delta_data_intervalsinstead, matching
config.yml's documented behavior and making the two configkeys independent, as intended.
Added
test_timedelta_schedule_respects_create_delta_data_intervals_configintask-sdk/tests/task_sdk/definitions/test_dag.py, covering:False) →DeltaTriggerTimetablecreate_delta_data_intervals=True→DeltaDataIntervalTimetablecreate_cron_data_intervals=Truealone → stillDeltaTriggerTimetable(regression guard against the exact bug fixed here)
Confirmed the new test fails against the old code and passes against the fix.
Full
test_dag.pysuite (94 tests) still passes.Note:
unit_tests.cfgsets bothcreate_cron_data_intervalsandcreate_delta_data_intervalstotrue, which is why the existing test suitenever caught this — both keys being
trueproduced the same (accidentallycorrect) result before and after this fix for any test not overriding them
individually.
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code following the guidelines
{pr_number}.significant.rst, in airflow-core/newsfragments.