Skip to content

Fix depends_on_past for manual/asset runs when logical_date is NULL #66285

Open
Subham-KRLX wants to merge 4 commits into
apache:mainfrom
Subham-KRLX:fix/depends-on-past-nullable-logical-date
Open

Fix depends_on_past for manual/asset runs when logical_date is NULL #66285
Subham-KRLX wants to merge 4 commits into
apache:mainfrom
Subham-KRLX:fix/depends-on-past-nullable-logical-date

Conversation

@Subham-KRLX

@Subham-KRLX Subham-KRLX commented May 2, 2026

Copy link
Copy Markdown
Contributor

This PR restores depends_on_past for manual and asset-triggered runs when DagRun.logical_date is NULL.

Problem:
Lookups used only logical_date, so manual/asset runs were ignored and depends_on_past could be bypassed.

Fix:
Get_previous_dagrun() falls back to run_after; get_last_dagrun() orders by COALESCE(logical_date, run_after). Tests and checks updated.

closes: #65578

Was generative AI tooling used to co-author this PR?
Yes — Claude (for PR description)


Important

🛠️ Maintainer triage note for @Subham-KRLX · by @potiuk · 2026-07-08 16:05 UTC

This PR was in the maintainer review queue, but its Postgres core tests are now failing — so I've removed the ready for maintainer review label and moved it back to your court.

The ball is in your court — you've been assigned. Fix the failing tests, then mark it Ready for review again. See the Pull Request quality criteria.

Automated triage — may be imperfect; a maintainer takes the next look.

…gical_date

Fall back to run_after when logical_date is None to ensure depends_on_past works for all run types.

Fixes apache#65578
@Subham-KRLX
Subham-KRLX requested review from XD-DENG and ashb as code owners May 2, 2026 18:21
@Subham-KRLX
Subham-KRLX marked this pull request as draft May 2, 2026 19:20
@Subham-KRLX
Subham-KRLX force-pushed the fix/depends-on-past-nullable-logical-date branch from d1f4f95 to b31f9b3 Compare May 2, 2026 19:30
@Subham-KRLX
Subham-KRLX marked this pull request as ready for review May 3, 2026 03:59
@potiuk potiuk added the ready for maintainer review Set after triaging when all criteria pass. label May 24, 2026
Comment thread airflow-core/src/airflow/models/dagrun.py
Comment thread airflow-core/src/airflow/models/dagrun.py
Comment thread airflow-core/src/airflow/models/dag.py
Comment thread airflow-core/src/airflow/models/dagrun.py

@jscheffl jscheffl left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fear besides describing this as a bug fix that an expert (not mine :-D) Opinion is required if it is okay to change this. Because it is a behaviroal change in semantics of how a "previous" run is treated. It for sure requires a newsfragment to highlight this to users and I'd request a short call to devlist if there is somebody raising concerns. Personally for me it is OK to change but all things that are related to dates and times has a long long hostory.

@jscheffl
jscheffl requested review from dstandish and kaxil June 6, 2026 10:46
@Subham-KRLX

Copy link
Copy Markdown
Contributor Author

I fear besides describing this as a bug fix that an expert (not mine :-D) Opinion is required if it is okay to change this. Because it is a behaviroal change in semantics of how a "previous" run is treated. It for sure requires a newsfragment to highlight this to users and I'd request a short call to devlist if there is somebody raising concerns. Personally for me it is OK to change but all things that are related to dates and times has a long long hostory.

I agree this behavioral change needs a newsfragment and I will add it along with the missing pytests. Instead of the dev mailing list I prefer to raise this in the Airflow Slack channel to get feedback from core maintainers does that sound good to you?

@jscheffl

jscheffl commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

I fear besides describing this as a bug fix that an expert (not mine :-D) Opinion is required if it is okay to change this. Because it is a behaviroal change in semantics of how a "previous" run is treated. It for sure requires a newsfragment to highlight this to users and I'd request a short call to devlist if there is somebody raising concerns. Personally for me it is OK to change but all things that are related to dates and times has a long long hostory.

I agree this behavioral change needs a newsfragment and I will add it along with the missing pytests. Instead of the dev mailing list I prefer to raise this in the Airflow Slack channel to get feedback from core maintainers does that sound good to you?

Devlist id the "official" Apache medium, Slack is accepted for collaboration. Slack is not archived and content vanishes after 90 days, so no point to reference later (...mememe but Jens said on Slack in Dec 2021 that it is okay).

@Subham-KRLX

Copy link
Copy Markdown
Contributor Author

I have just started a thread on the dev mailing list to get feedback from the broader community as requested i will also add the newsfragment and the missing pytests in the meantime.

@potiuk potiuk removed the ready for maintainer review Set after triaging when all criteria pass. label Jul 8, 2026
@potiuk

potiuk commented Jul 11, 2026

Copy link
Copy Markdown
Member

@Subham-KRLX — Some review feedback from @jscheffl is waiting on you: 4 unresolved review thread(s). Could you either push a fix or reply in each thread explaining why the feedback doesn't apply? When you believe the feedback is addressed, please mark the threads as resolved and ping the reviewer (@jscheffl) for a final look. Thanks!


Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you.

@potiuk potiuk added the ready for maintainer review Set after triaging when all criteria pass. label Jul 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for maintainer review Set after triaging when all criteria pass.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

depends_on_past and run lookups broken by nullable logical_date (AIP-39)

3 participants