Remove obsolete AIP-76 partition-date storage TODOs#68703
Open
Lee-W wants to merge 5 commits into
Open
Conversation
boring-cyborg
Bot
added
area:API
These TODOs predate the dedicated next_dagrun_partition_date / next_dagrun_partition_key columns and the partition_date / partition_key fields on DagRunInfo. Partition date is now stored separately rather than re-derived from key_format, so the comments no longer describe the code.
The existing db-backed test only exercised the catchup path with no prior run. Add a direct unit test over the remaining branches — catchup advancing from a last run, the no-earliest first-run path, the non-catchup candidate selection, the earliest floor, and the latest cutoff returning None — and drop the "add test for this logic" TODO now that it is covered.
These comments posed open design questions ("we could…", "perhaps we
need…", "unclear whether we should / need to") rather than marking
concrete pending work, so they read as TODOs without being actionable.
Drop them to avoid a standing punch-list of undecided ideas in the
source; the kept TODOs remain those that document a limitation at its
enforcing site or link a tracked issue.
These two comments describe accepted current behaviour — the run-date to partition mapping being lossy for some offsets, and null-logical_date runs not being matched — rather than pending work. Rewording them as neutral comments keeps the caveat without implying a standing TODO.
The auto-reprocessing note was a discussion prompt with no tracked issue, and the run_offset timedelta/relativedelta note was redundant with the adjacent guard that already raises "not supported yet". Neither marked actionable, tracked work, so drop both. The remaining AIP-76 TODO links issue apache#59618.
Lee-W
force-pushed
the
remove-stale-aip76-todos
branch
from
61fd017 to
5fd08f4
Compare
Lee-W
requested review from
XD-DENG,
amoghrajesh,
ashb,
ephraimbuddy,
jason810496,
kaxil,
pierrejeambrun,
rawwar and
uranusjr
as code owners
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
AIP-76 (Asset Partitions) left a scattering of
# todo: AIP-76notes, but most are no longer actionable: some describe behavior that has since changed, some are open-ended design musings, and some restate an accepted limitation. Comments that read like a TODO but cannot be acted on rot — they mislead readers into thinking there is pending work.What
No behavior change — this is comment cleanup plus one added test.
next_dagrun_partition_date/next_dagrun_partition_keycolumns andDagRunInfo.partition_date/partition_key. The partition date is now stored independently and no longer back-derived from the key format, so the note no longer matches the code (core + SDK, 3 sites).trigger.py,dag.py, and the scheduler are not actionable items; they read as a parked idea list rather than work.logical_dateare not matched). They are rewritten as neutral comments that keep the caveat without implying pending work.run_offsettimedelta / relativedelta note duplicated the adjacent guard that already raises "not supported yet".CronPartitionTimetable.next_dagrun_info_v2branches with tests. The existing db-backed test only exercised the catch-up + no-previous-run branch. A direct unit test now covers the rest (catching up advancing from a previous run, first run with no earliest, non-catchup candidate selection, the earliest lower bound, and the latest upper bound returningNone), letting the "add test for this logic" TODO go.Was generative AI tooling used to co-author this PR?
Generated-by: [Claude] following the guidelines
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.