You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "pierrejeambrun (via GitHub)" <gi...@apache.org> on 2026/09/09 14:00:20 UTC
[PR] Drop the redundant joinedload on the previous-TI lookup [airflow]
pierrejeambrun opened a new pull request, #72811:
URL: https://github.com/apache/airflow/pull/72811
Follow-up to #72699. That PR bounded the ``get_previous_task_instance`` lookup
with ``.limit(1)``; @kaxil noted in review that the same query also asks for an
eager-load that duplicates work the mapper already does.
``TaskInstance.dag_run`` is ``lazy="joined"`` (``airflow-core/src/airflow/models/taskinstance.py:727``),
so the explicit ``joinedload(TI.dag_run)`` attaches a parallel ``JOIN dag_run AS dag_run_1``
next to the explicit ``JOIN dag_run`` the query already needs for the
``ORDER BY dag_run.logical_date``. Both joins select the same rows over the same
``(dag_id, run_id)`` FK pair, so every request paid for one extra join.
Switch to ``contains_eager(TI.dag_run)`` so the mapper's eager-load reuses the
explicit join. Compiled SQL drops from ``JOIN dag_run ... JOIN dag_run AS dag_run_1 ...``
to a single ``JOIN dag_run``; the regression test in this PR fails on the old
shape and passes on the new one.
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes — Claude Code (Opus 4.7)
Generated-by: Claude Code (Opus 4.7) following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reuse the explicit dag_run join on the previous-TI lookup [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #72811:
URL: https://github.com/apache/airflow/pull/72811#issuecomment-5633420752
### Backport successfully created: v3-3-test
Note: As of [Merging PRs targeted for Airflow 3.X](https://github.com/apache/airflow/blob/main/dev/README_AIRFLOW3_DEV.md#merging-prs-targeted-for-airflow-3x)
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.
In matter of doubt please ask in [#release-management](https://apache-airflow.slack.com/archives/C03G9H97MM2) Slack channel.
<table>
<tr>
<th>Status</th>
<th>Branch</th>
<th>Result</th>
</tr>
<tr>
<td>✅</td>
<td>v3-3-test</td>
<td><a href="https://github.com/apache/airflow/pull/72944"><img src="https://img.shields.io/badge/PR-72944-blue" alt="PR Link"></a></td>
</tr>
</table>
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reuse the explicit dag_run join on the previous-TI lookup [airflow]
Posted by "vatsrahul1001 (via GitHub)" <gi...@apache.org>.
vatsrahul1001 merged PR #72811:
URL: https://github.com/apache/airflow/pull/72811
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Drop the redundant joinedload on the previous-TI lookup [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #72811:
URL: https://github.com/apache/airflow/pull/72811#discussion_r3972719248
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -1213,7 +1213,7 @@ def get_previous_task_instance(
query = (
select(TI)
.join(DR, (TI.dag_id == DR.dag_id) & (TI.run_id == DR.run_id))
- .options(joinedload(TI.dag_run))
+ .options(contains_eager(TI.dag_run))
Review Comment:
Since the response only reads `ti.dag_run.logical_date`, `contains_eager(TI.dag_run).load_only(DR.logical_date)` trims this further. Compiling all three shapes locally: the dag_run side goes from 28 columns (including the undeferred `conf` JSON) down to 2, and the whole SELECT from 66 columns to 40. Same shape the scheduler uses in [scheduler_job_runner.py#L826-L830](https://github.com/apache/airflow/blob/251cd91bb24a335f9ec320ec1b3f2929f9e4577c/airflow-core/src/airflow/jobs/scheduler_job_runner.py#L826-L830).
##########
airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_task_instances.py:
##########
@@ -3884,6 +3886,32 @@ def test_get_previous_ti_with_all_filters(self, client, session, create_task_ins
assert data["run_id"] == "target_run_1"
assert data["state"] == State.SUCCESS
+ def test_get_previous_ti_joins_dag_run_once(self, client, session, create_task_instance):
+ """The eager-load of ``TI.dag_run`` must reuse the explicit ORDER BY join, not add a
+ second parallel ``JOIN dag_run AS dag_run_1``."""
+ for i in range(3):
+ create_task_instance(
+ task_id="test_task",
+ state=State.SUCCESS,
+ logical_date=timezone.datetime(2025, 1, i + 1),
+ run_id=f"run{i + 1}",
+ )
+ session.commit()
+
+ with capture_orm_selects("task_instance") as statements:
+ response = client.get(
+ "/execution/task-instances/previous/dag/test_task",
+ params={"logical_date": "2025-01-03T00:00:00Z"},
+ )
+
+ assert response.status_code == 200
Review Comment:
The compiled shape is data-independent, so this passes even when the lookup returns `null`, and the three TIs above aren't contributing to the assertion. `logical_date` is the one response field sourced from the eager-loaded `dag_run`, so asserting it here would cover the half of the change the SQL text can't see.
##########
airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_task_instances.py:
##########
@@ -3884,6 +3886,32 @@ def test_get_previous_ti_with_all_filters(self, client, session, create_task_ins
assert data["run_id"] == "target_run_1"
assert data["state"] == State.SUCCESS
+ def test_get_previous_ti_joins_dag_run_once(self, client, session, create_task_instance):
Review Comment:
This branch is based on 428e873c, before #72699, so the new test lands exactly where `test_get_previous_ti_query_is_bounded` sits on main and `git merge-tree upstream/main` conflicts on this file (the route file auto-merges cleanly, `.limit(1)` and the loader option are different lines). Worth deciding on the rebase whether the join-count assert folds into that test, since the setup and the `capture_orm_selects` call are identical.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reuse the explicit dag_run join on the previous-TI lookup [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #72811:
URL: https://github.com/apache/airflow/pull/72811#issuecomment-5623970801
> On the new docstring line, I was wrong last round: I said asserting logical_date would cover the load_only half, and it does not, so "eager-loaded" claims more than the test checks. capture_orm_selects keeps only statements matching \bFROM task_instance\b ([asserts.py#L202](https://github.com/apache/airflow/blob/a9f6c898f01dc1e5e1dc18172f5bc65097957a71/devel-common/src/tests_common/test_utils/asserts.py#L202)). If logical_date were deferred rather than eager-loaded, it would arrive through a separate SELECT dag_run.logical_date ... FROM dag_run WHERE dag_run.id = ?, which that filter drops, so the loop never inspects it and data["logical_date"] still comes back correct. The assert proves the value is right, not that it came from the join. A query count around the request would pin the eager-load claim; short of that, the docstring is the thing to soften.
Updated
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reuse the explicit dag_run join on the previous-TI lookup [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun closed pull request #72811: Reuse the explicit dag_run join on the previous-TI lookup
URL: https://github.com/apache/airflow/pull/72811
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org