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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
