kaxil commented on code in PR #72699:
URL: https://github.com/apache/airflow/pull/72699#discussion_r3963859248
##########
airflow-core/tests/unit/api_fastapi/execution_api/versions/head/test_task_instances.py:
##########
@@ -3884,6 +3886,28 @@ 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_query_is_bounded(self, client, session,
create_task_instance):
+ """The single-row previous-TI lookup must ask the DB for one row."""
+ for i in range(5):
+ 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-05T00:00:00Z"},
+ )
+
+ assert response.status_code == 200
Review Comment:
`test_get_previous_ti_returns_most_recent` just above (line 3825) has the
identical fixture loop and the identical request, and it finishes by asserting
`data["run_id"] == "run4"`. Carrying that same assertion here would let this
test prove the row `LIMIT 1` keeps is still the right one, rather than only
that the bound reached the SQL.
##########
airflow-core/src/airflow/api_fastapi/execution_api/routes/task_instances.py:
##########
@@ -1225,7 +1225,7 @@ def get_previous_task_instance(
if state:
query = query.where(TI.state == state)
- ti = session.scalars(query).first()
+ ti = session.scalars(query.limit(1)).first()
Review Comment:
`TI.dag_run` is already `lazy="joined"` on the mapper
(`models/taskinstance.py:727`), so the `joinedload(TI.dag_run)` on line 1216
changes nothing and this query joins `dag_run` twice: once explicitly for the
`ORDER BY`, once as `dag_run_1` for the eager load. The explicit ON clause is
the same FK pair the relationship itself uses (`dag_id`, `run_id`), so
`contains_eager(TI.dag_run)` reuses that join rather than adding a second one,
and it keeps `ti.dag_run.logical_date` below populated. Compiling the three
variants gives `JOIN dag_run` twice as written here, twice with the option
removed, and once with `contains_eager`, all three still carrying `LIMIT 1`.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/dags.py:
##########
@@ -436,10 +436,12 @@ def unfavorite_dag(dag_id: str, session: SessionDep,
user: GetUserDep):
user_id = str(user.get_id())
favorite_exists = session.execute(
- select(DagFavorite).where(
+ select(DagFavorite)
+ .where(
DagFavorite.dag_id == dag_id,
DagFavorite.user_id == user_id,
)
+ .limit(1)
Review Comment:
`user_id` and `dag_id` are the only two columns on `DagFavorite` and both
are `primary_key=True`, so a filter on both can match at most one row and this
`LIMIT 1` is a no-op for the planner. That also makes
`test_unfavorite_dag_existence_check_is_bounded` a permanent assertion about a
bound the schema already guarantees. If the point is to spend less on this
endpoint, `session.execute(delete(DagFavorite).where(...)).rowcount` gives you
the 409 signal without a separate probe.
--
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]
