pierrejeambrun commented on code in PR #69940:
URL: https://github.com/apache/airflow/pull/69940#discussion_r3646673720
##########
airflow-core/src/airflow/api_fastapi/common/parameters.py:
##########
@@ -499,14 +499,10 @@ def search_param_factory(
else "The pipe `|` is matched literally, not as an OR separator. "
)
DESCRIPTION = (
- "SQL LIKE expression — use `%` / `_` wildcards (e.g. `%customer_%`). "
+ "Case-insensitive substring match; `%` / `_` wildcards allowed. "
f"{pipe_clause}"
- "Regular expressions are **not** supported. "
- "\n\n"
- "**Performance note:** this full-match pattern is evaluated as ``ILIKE
'%term%'`` and "
- "most of the time prevents the database from using B-tree indexes,
which can be very "
- "slow on large tables. Prefer the equivalent "
- f"``{pattern_name.replace('_pattern', '_prefix_pattern')}`` parameter
when possible."
+ f"Not index-friendly — prefer ``{pattern_name.replace('_pattern',
'_prefix_pattern')}`` "
+ "on large tables."
Review Comment:
For both descriptoin update here I prefered the original one, which is more
explicit.
That's not related directly to the PR maybe we should keep the original, it
will also reduce the diff cause all generated search have this description
embedded.
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/ui/test_assets.py:
##########
@@ -471,3 +480,315 @@ def
test_non_partitioned_asset_inactive_true_when_deactivated(self, test_client,
body = response.json()
assert len(body["events"]) == 1
assert body["events"][0]["asset_inactive"] is True
+
+
+class TestGetAssetsUi:
+ @pytest.fixture(autouse=True)
+ def cleanup_assets(self):
Review Comment:
Can we add some `assert_queries_count` guards just to make sure we don't
have N+1 queries problem (by forgetting an eager loading or explicit join etc..)
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/assets.py:
##########
@@ -49,6 +71,83 @@
assets_router = AirflowRouter(tags=["Asset"])
+@assets_router.get(
+ "/assets",
+ dependencies=[
+ Depends(requires_access_asset(method="GET")),
+ Depends(requires_access_asset_alias(method="GET")),
+ ],
+ operation_id="get_assets_ui",
+)
+def get_assets(
+ limit: QueryLimit,
+ offset: QueryOffset,
+ name_pattern: QueryAssetNamePatternSearch,
+ name_prefix_pattern: QueryAssetNamePrefixPatternSearch,
+ uri: QueryUriExactMatch,
+ uri_pattern: QueryUriPatternSearch,
+ uri_prefix_pattern: QueryUriPrefixPatternSearch,
+ group_pattern: QueryAssetGroupPatternSearch,
+ group_prefix_pattern: QueryAssetGroupPrefixPatternSearch,
+ dag_ids: QueryAssetDagIdPatternSearch,
+ only_active: Annotated[OnlyActiveFilter,
Depends(OnlyActiveFilter.depends)],
+ last_asset_event_timestamp_range: Annotated[
+ RangeFilter,
+ Depends(
+ datetime_range_filter_factory(
+ "last_asset_event_timestamp", AssetEvent,
attribute_name="timestamp"
+ )
+ ),
+ ],
+ order_by: Annotated[
+ SortParam,
+ Depends(
+ SortParam(
+ ["id", "name", "uri", "group", "created_at", "updated_at"],
+ AssetModel,
+ {"last_asset_event_timestamp": AssetEvent.timestamp},
+ ).dynamic_depends(default="-last_asset_event_timestamp")
+ ),
+ ],
+ session: SessionDep,
+) -> AssetCollectionResponse:
+ """Get assets. Like the public endpoint, but also supports sorting by
group and last asset event timestamp."""
+ assets_select, total_entries = paginated_select(
+ statement=generate_assets_with_last_event_query(),
+ filters=[
+ only_active,
+ name_pattern,
+ name_prefix_pattern,
+ uri,
+ uri_pattern,
+ uri_prefix_pattern,
+ group_pattern,
+ group_prefix_pattern,
+ dag_ids,
+ last_asset_event_timestamp_range,
+ ],
+ order_by=order_by,
+ offset=offset,
+ limit=limit,
+ session=session,
+ )
+
+ # CASE key keeps assets with no event (NULL timestamp) last in both
directions.
+ order_columns: list[ColumnElement] = []
Review Comment:
I would keep things consistent with other APIs, nulls behavior is decided by
the backend implementation (db).
Also a case prevent the leverage of indexes, so performance wise it's not
great, that's why we removed a very similar piece of code from. the
`parameters.py` module.
##########
airflow-core/src/airflow/ui/src/pages/AssetsList/AssetsList.tsx:
##########
@@ -126,21 +110,36 @@ export const AssetsList = () => {
const namePattern = searchParams.get(NAME_PATTERN) ?? "";
const advancedSearch = useAdvancedSearch("assets");
- const { setTableURLState, tableURLState } = useTableURLState();
+ const { setTableURLState, tableURLState } = useTableURLState({
+ sorting: [{ desc: true, id: "last_asset_event_timestamp" }],
+ });
const { pagination, sorting } = tableURLState;
const [sort] = sorting;
- const orderBy = sort ? [`${sort.desc ? "-" : ""}${sort.id}`] : undefined;
+ const orderBy = sort ? [`${sort.desc ? "-" : ""}${sort.id}`] :
["-last_asset_event_timestamp"];
- const { onClose, onOpen, open } = useDisclosure();
+ const { filterConfigs, handleFiltersChange, initialValues } =
useFiltersHandler(assetsFilterKeys);
- const { data, error, isLoading } = useAssetServiceGetAssets({
+ const lastAssetEventTimestampGte =
searchParams.get(SearchParamsKeys.LAST_ASSET_EVENT_TIMESTAMP_GTE);
+ const lastAssetEventTimestampLte =
searchParams.get(SearchParamsKeys.LAST_ASSET_EVENT_TIMESTAMP_LTE);
+ const groupArg = useAdvancedSearchArg({
+ patternApiKey: "groupPattern",
+ prefixApiKey: "groupPrefixPattern",
+ storageKey: SearchParamsKeys.GROUP_PATTERN,
+ value: searchParams.get(SearchParamsKeys.GROUP_PATTERN),
+ });
+
Review Comment:
We are missing the ability to switch from substring to prefix search for
search fields:
<img width="349" height="183" alt="Image"
src="https://github.com/user-attachments/assets/fe1ff53a-7af5-4d58-905d-269541c3b773";
/>
<img width="389" height="173" alt="Image"
src="https://github.com/user-attachments/assets/e6ec2b88-02d8-4fb2-8ca4-7af6efccbbeb";
/>
##########
airflow-core/src/airflow/api_fastapi/common/db/assets.py:
##########
@@ -0,0 +1,54 @@
+# 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.
+
+from __future__ import annotations
+
+from typing import TYPE_CHECKING
+
+from sqlalchemy import func, select
+from sqlalchemy.orm import subqueryload
+
+from airflow.models.asset import AssetEvent, AssetModel, AssetWatcherModel
+
+if TYPE_CHECKING:
+ from sqlalchemy.sql import Select
+
+
+def generate_assets_with_last_event_query() -> Select:
+ """Fetch Assets outer-joined to their latest AssetEvent id/timestamp."""
+ max_asset_event_id_query = (
+ select(AssetEvent.asset_id,
func.max(AssetEvent.id).label("max_asset_event_id"))
Review Comment:
This changes the `max(timestamp)` to `max(id)` that's behavior change for
the public endpoint. Is that expected? Should we keep the former?
--
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]
