pierrejeambrun commented on code in PR #69489:
URL: https://github.com/apache/airflow/pull/69489#discussion_r3535028777
##########
airflow-core/docs/migrations-ref.rst:
##########
@@ -39,7 +39,9 @@ Here's the list of all the Database Migrations that are
executed via when you ru
+-------------------------+------------------+-------------------+--------------------------------------------------------------+
| Revision ID | Revises ID | Airflow Version | Description
|
+=========================+==================+===================+==============================================================+
-| ``d2f4e1b3c5a7`` (head) | ``9ff64e1c35d3`` | ``3.3.0`` | Add
partition_date to asset_partition_dag_run. |
+| ``c4e7a1f9b2d0`` (head) | ``d2f4e1b3c5a7`` | ``3.3.0`` | Add index
on asset.uri. |
Review Comment:
Target version needs to be updated
##########
airflow-core/newsfragments/69489.improvement.rst:
##########
@@ -0,0 +1 @@
+Add an indexed exact-match ``uri`` query parameter to ``GET /api/v2/assets``
for fast single-asset lookup by URI (much faster than ``uri_pattern``, which
uses an unindexed ``ILIKE '%...%'``).
Review Comment:
This needs to be removed. (I know doc mentions that user facing change
should have a fragment, but that's only true for significant / behavior change
that needs a user warning basically).
##########
airflow-core/src/airflow/api_fastapi/common/parameters.py:
##########
@@ -1578,6 +1578,22 @@ def _transform_ti_states(states: list[str] | None) ->
list[TaskInstanceState | N
QueryUriPrefixPatternSearch = Annotated[
_PrefixSearchParam, Depends(prefix_search_param_factory(AssetModel.uri,
"uri_prefix_pattern"))
]
+QueryUriExactMatch = Annotated[
+ FilterParam[str | None],
+ Depends(
+ filter_param_factory(
+ AssetModel.uri,
+ str | None,
+ filter_name="uri",
+ description=(
+ "Exact-match filter on the full asset URI. Compiles to an
indexed equality "
Review Comment:
We could make this filter accept multiple values.
To allow `?uri=some_uri1&uri=some_uri2` to get both assets with those URI.
(And it makes more sense to me for a list endpoint to accept multiple values,
otherwise it's really close to a single GET endpoint and maybe we should
instead have `GET assets/{uri}`, but lets not go there)
It's really a simple change, as an exemple for implementation:
```python
QueryTIStateFilter = Annotated[
FilterParam[list[str]],
Depends(
filter_param_factory(
TaskInstance.state,
list[str],
FilterOptionEnum.ANY_EQUAL,
default_factory=list,
transform_callable=_transform_ti_states,
)
),
]
```
##########
airflow-core/src/airflow/api_fastapi/common/parameters.py:
##########
@@ -1578,6 +1578,22 @@ def _transform_ti_states(states: list[str] | None) ->
list[TaskInstanceState | N
QueryUriPrefixPatternSearch = Annotated[
_PrefixSearchParam, Depends(prefix_search_param_factory(AssetModel.uri,
"uri_prefix_pattern"))
]
+QueryUriExactMatch = Annotated[
+ FilterParam[str | None],
+ Depends(
+ filter_param_factory(
+ AssetModel.uri,
+ str | None,
+ filter_name="uri",
+ description=(
+ "Exact-match filter on the full asset URI. Compiles to an
indexed equality "
+ "comparison (``uri = ...``), so it is far faster than
``uri_pattern`` (which uses "
+ "``ILIKE '%...%'`` and cannot use an index) for resolving a
single asset by its "
+ "known URI."
+ ),
+ )
+ ),
+]
Review Comment:
I would remove the piece that compares to search params. They have their own
doc explaining the performance pitfalls, this can stay about 'exact match'.
```suggestion
QueryUriExactMatch = Annotated[
FilterParam[str | None],
Depends(
filter_param_factory(
AssetModel.uri,
str | None,
filter_name="uri",
description=(
"Exact-match filter on the full asset URI. Compiles to an
indexed equality "
"comparison (``uri = ...``).
),
)
),
]
```
##########
airflow-core/src/airflow/models/asset.py:
##########
@@ -333,6 +333,9 @@ class AssetModel(Base):
__tablename__ = "asset"
__table_args__ = (
Index("idx_asset_name_uri_unique", name, uri, unique=True),
+ # Single-column index so exact-match lookups by URI (GET
/assets?uri=...) can use an
+ # index; the composite index above leads with ``name`` and cannot
serve uri-only queries.
Review Comment:
I would remove this, to not mention the API layer in ORM models.
##########
airflow-core/src/airflow/migrations/versions/0124_3_3_0_add_index_on_asset_uri.py:
##########
@@ -0,0 +1,47 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+Add index on asset.uri.
+
+Revision ID: c4e7a1f9b2d0
+Revises: d2f4e1b3c5a7
+Create Date: 2026-07-06 00:00:00.000000
+"""
+
+from __future__ import annotations
+
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = "c4e7a1f9b2d0"
+down_revision = "d2f4e1b3c5a7"
+branch_labels = None
+depends_on = None
+airflow_version = "3.3.0"
+
+
+def upgrade():
+ """Apply Add index on asset.uri."""
+ with op.batch_alter_table("asset", schema=None) as batch_op:
+ batch_op.create_index("idx_asset_uri", ["uri"], unique=False)
+
+
+def downgrade():
+ """Unapply Add index on asset.uri."""
+ with op.batch_alter_table("asset", schema=None) as batch_op:
+ batch_op.drop_index("idx_asset_uri")
Review Comment:
It's fine to have a 'filter/order_by' in the API that isn't backed by an
index. Indeed it will be slow if the table grows out of control but does the
table grows out of control in most common cases? I would avoid adding an index
(costly) just for a single filter of a single endpoint.
I'm not sure here but I would say the table isn't huge most of the time.
(millions of assets) and no index is probably fine for most? (the index
probably wasn't there in 2.x as well)
If someone happen to have a huge asset table they can add their own index to
improve performance following
https://github.com/apache/airflow/blob/main/airflow-core/docs/howto/performance.rst
--
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]
