[search ui] Add asset filter fields to search index #20372

clairelin135 · 2024-03-08T21:59:52Z

This PR enables adds the following asset filters to the search index results:

asset owner
compute kind
code location
asset group

This involves:

Querying for these fields per-asset on SECONDARY_SEARCH_QUERY
Grouping by field to determine the # of assets per filter
Adding each filter to the list of possible search results

Open questions:

Perf impact of fetching these additional fields for each asset in graphQL?

clairelin135 · 2024-03-08T22:00:07Z

[search ui] Display asset search on asset landing page #20373
[search ui] Add asset filter fields to search index #20372 👈
[ui] Enable filtering asset table by owner #20420
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @clairelin135 and the rest of your teammates on Graphite

github-actions · 2024-03-08T22:02:43Z

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-9fgdh1fo4-elementl.vercel.app
https://03-07-claire-asset-search-ui.core-storybook.dagster-docs.io

Built with commit ed88cdc.
This pull request is being automatically deployed with vercel-action

clairelin135 · 2024-03-09T00:51:51Z

js_modules/dagster-ui/packages/ui-core/src/search/types.ts

+  AssetGroup = 'AssetFilterSearchResultType.AssetGroup',
+}
+
+export function isAssetFilterSearchResultType(


Not sure how to check if a value exists in a given enum... I was seeing the following always return true:

x: SearchResultType | AssetFilterSearchResultType = ... x in AssetFilterSearchResultType // Returns true??

For now, just tested for strict equivalence to make this logic work

Checking the value against each enum member individually is fine, though I suspect that you won't need this function if the asset search UI is built separately from SearchDialog.

Got it, thanks.

I think we still need this logic within the asset search component to render asset results and filter results a little differently.

I moved this function to the other PR where it's actually used

hellendag

Some thoughts inline about separating the search components/hooks, related to comments on #20373.

hellendag · 2024-03-11T14:20:03Z

js_modules/dagster-ui/packages/ui-core/src/search/SearchDialog.tsx

  const searchAndHandleSecondary = React.useCallback(
-    async (queryString: string) => {
+    async (queryString: string, returnAssetFilters: boolean) => {
      const {queryString: queryStringForResults, results} = await searchSecondary(queryString);
-      dispatch({type: 'complete-secondary', queryString: queryStringForResults, results});
+      if (!returnAssetFilters) {
+        dispatch({
+          type: 'complete-secondary',
+          queryString: queryStringForResults,
+          results: results.filter((result) => result.item.type === SearchResultType.Asset), // Only return asset results
+        });
+      } else {
+        dispatch({type: 'complete-secondary', queryString: queryStringForResults, results});
+      }
    },
    [searchSecondary],
  );


I think it's fine to add another parameter to configure what gets pushed to the worker, but here's a spot where I'd say that the behavior is specific to the search behavior itself, and not the component. I think you could pull this useCallback out into a hook that can be reused by SearchDialog and the new Asset UI. That way, SearchDialog never has to know about this behavior -- it can just skip the asset filters (by passing false to the hook, something like that) and otherwise remain unchanged.

Thanks for the explanation here.

I played around with pulling this callback out into a hook, though I realized after your other comment about avoiding building unwanted results that we could configure whether asset filters are returned at the useGlobalSearch hook level.

After that change, this filtering logic is no longer needed.

hellendag · 2024-03-11T14:39:25Z

js_modules/dagster-ui/packages/ui-core/src/search/types.ts

+  AssetGroup = 'AssetFilterSearchResultType.AssetGroup',
+}
+
+export function isAssetFilterSearchResultType(


Checking the value against each enum member individually is fine, though I suspect that you won't need this function if the asset search UI is built separately from SearchDialog.

hellendag · 2024-03-11T14:40:52Z

js_modules/dagster-ui/packages/ui-core/src/search/SearchDialog.tsx

+        dispatch({
+          type: 'complete-secondary',
+          queryString: queryStringForResults,
+          results: results.filter((result) => result.item.type === SearchResultType.Asset), // Only return asset results


Seems like it would be a bit more efficient to avoid building the unwanted results, instead of discarding them here.

Good call, I implemented that within useGlobalSearch. A flag now is provided to useGlobalSearch to indicate whether asset filter results should be returned or not.

clairelin135 · 2024-03-11T19:45:39Z

@hellendag appreciate the explanations here.

I addressed the feedback and now SearchDialog doesn't need to know about asset filtering behavior, I think it's ready for another look!

salazarm · 2024-03-11T19:56:52Z

js_modules/dagster-ui/packages/ui-core/src/assets/AssetsOverview.tsx

+type AssetDefinitionMetadata = {
+  definition: {
+    owners: Array<
+      {__typename: 'UserAssetOwner'; email: string} | {__typename: 'TeamAssetOwner'; team: string}
+    >;
+    computeKind: string | null;
+    groupName: string | null;
+    repository: {
+      name: string;
+      location: {name: string};
+    };
+  } | null;
+};
+


I would go with something like this in order to keep a single source of truth for the base types.

type AssetDefinitionMetadata = { definition: Pick<AssetNode['definition'], 'owners' | ' computeKind' | 'groupName' | 'repository'>; }

or if you don't like picking keys like that you could do

type AssetDefinitionMetadata = { definition: { owners: AssetNode['definition']['owners'] ... },

good call, I added this

salazarm · 2024-03-12T03:14:35Z

One down side to this approach is that since there can be 2 instances of useGlobalSearch and they don't share cache with each other which means they will separately query the same data (via apollo so they should hit the shared cache if done one after the other, though it's possible both are open together in which case the cache isn't available so absent any apollo client query deduping it would be a duplicate request. afaik apollo doesnt dedupe).

salazarm · 2024-03-12T03:15:53Z

I guess for now it's probably fine and we can optimize that later if it becomes a problem.

clairelin135 · 2024-03-12T18:43:48Z

js_modules/dagster-ui/packages/ui-core/src/search/useGlobalSearch.tsx

+// This is the version of the secondary query, used as part of the cache key.
+// When the data in the cache must be invalidated, this version should be bumped to prevent
+// fetching stale data.
+export const SEARCH_SECONDARY_DATA_VERSION = 'v1;';


We recently added caching to store the latest asset query result to load the search UI when the query hasn't completed. This PR adds additional fields to that query, but this causes an issue on the first load as the logic assumes those fields exist but they don't on the cached query data. This causes the asset search UI to be unloadable.

This PR adds a version to the key to ensure that we won't fetch stale data.

As the title. Used in #20372 <img width="1149" alt="image" src="https://github.com/dagster-io/dagster/assets/29110579/07bf4abb-a5aa-4a8d-88a4-140f76e63d81"> <img width="1455" alt="image" src="https://github.com/dagster-io/dagster/assets/29110579/506fd9e5-74e6-493d-96fe-a9040def6b22">

salazarm · 2024-03-12T20:23:18Z

js_modules/dagster-ui/packages/ui-core/src/search/useGlobalSearch.tsx

@@ -207,7 +297,7 @@ export const useGlobalSearch = () => {
    loading: secondaryDataLoading,
  } = useIndexedDBCachedQuery<SearchSecondaryQuery, SearchSecondaryQueryVariables>({
    query: SEARCH_SECONDARY_QUERY,
-    key: 'SearchSecondary',
+    key: `SearchSecondary:${SEARCH_SECONDARY_DATA_VERSION}`,


This works but it leaves the old cache in place so it never gets cleaned up. I think instead useIndexedDBCachedQuery should take a third parameter version that can be used to tell useIndexedDBCachedQuery to ignore caches on a different version.

Makes sense. This PR is updated to now also store the data version in the cache.

I added a data version for both the primary and secondary queries since we should always be providing a version

salazarm

Sweet

Feedback addressed

This PR enables adds the following asset filters to the search index results: - asset owner - compute kind - code location - asset group This involves: 1. Querying for these fields per-asset on `SECONDARY_SEARCH_QUERY` 2. Grouping by field to determine the # of assets per filter 3. Adding each filter to the list of possible search results Open questions: - Perf impact of fetching these additional fields for each asset in graphQL?

As the title. Used in #20372 <img width="1149" alt="image" src="https://github.com/dagster-io/dagster/assets/29110579/07bf4abb-a5aa-4a8d-88a4-140f76e63d81"> <img width="1455" alt="image" src="https://github.com/dagster-io/dagster/assets/29110579/506fd9e5-74e6-493d-96fe-a9040def6b22">

This PR enables adds the following asset filters to the search index results: - asset owner - compute kind - code location - asset group This involves: 1. Querying for these fields per-asset on `SECONDARY_SEARCH_QUERY` 2. Grouping by field to determine the # of assets per filter 3. Adding each filter to the list of possible search results Open questions: - Perf impact of fetching these additional fields for each asset in graphQL?

clairelin135 mentioned this pull request Mar 8, 2024

[search ui] Display asset search on asset landing page #20373

Merged

clairelin135 force-pushed the 03-07-claire/asset-search-ui branch 2 times, most recently from 1c9b8ca to 445969c Compare March 9, 2024 00:37

clairelin135 commented Mar 9, 2024

View reviewed changes

clairelin135 force-pushed the 03-07-claire/asset-search-ui branch from 445969c to 104a5dd Compare March 9, 2024 01:04

clairelin135 marked this pull request as ready for review March 9, 2024 01:24

clairelin135 requested review from bengotow, salazarm and hellendag March 9, 2024 01:25

hellendag previously requested changes Mar 11, 2024

View reviewed changes

clairelin135 force-pushed the 03-07-claire/asset-search-ui branch 3 times, most recently from 1010eea to ecffd16 Compare March 11, 2024 19:41

clairelin135 requested a review from hellendag March 11, 2024 19:45

salazarm reviewed Mar 11, 2024

View reviewed changes

clairelin135 force-pushed the 03-07-claire/asset-search-ui branch from ecffd16 to ce41857 Compare March 11, 2024 21:35

clairelin135 mentioned this pull request Mar 11, 2024

[ui] Enable filtering asset table by owner #20420

Merged

clairelin135 force-pushed the 03-07-claire/asset-search-ui branch from ce41857 to b58b72c Compare March 11, 2024 23:33

clairelin135 changed the base branch from master to 03-11-claire/allow-path-filter-by-owners March 11, 2024 23:33

clairelin135 force-pushed the 03-11-claire/allow-path-filter-by-owners branch from 2ed0ed1 to 9b8a2ec Compare March 12, 2024 18:42

clairelin135 force-pushed the 03-07-claire/asset-search-ui branch from b58b72c to c92d186 Compare March 12, 2024 18:42

clairelin135 commented Mar 12, 2024

View reviewed changes

Base automatically changed from 03-11-claire/allow-path-filter-by-owners to master March 12, 2024 18:50

clairelin135 force-pushed the 03-07-claire/asset-search-ui branch from c92d186 to 1cdabc9 Compare March 12, 2024 18:53

clairelin135 added 8 commits March 12, 2024 13:06

claire/asset-search-ui

8f3f6d6

add owners and result count

681b5e1

modify descriptions etc

5de4983

handle asset search fork in hook

3ff14b3

add correct links for groups / compute kind views

b6f9964

fix links

aceb331

pr feedback

e90fd23

add data version to key

289c094

clairelin135 force-pushed the 03-07-claire/asset-search-ui branch from 1cdabc9 to 289c094 Compare March 12, 2024 20:10

salazarm reviewed Mar 12, 2024

View reviewed changes

store data version as part of search cache

ed88cdc

salazarm approved these changes Mar 12, 2024

View reviewed changes

clairelin135 merged commit 2b3ed46 into master Mar 12, 2024
2 checks passed

clairelin135 deleted the 03-07-claire/asset-search-ui branch March 12, 2024 21:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[search ui] Add asset filter fields to search index #20372

[search ui] Add asset filter fields to search index #20372

clairelin135 commented Mar 8, 2024 •

edited

Loading

clairelin135 commented Mar 8, 2024 •

edited

Loading

github-actions bot commented Mar 8, 2024 •

edited

Loading

clairelin135 Mar 9, 2024

hellendag Mar 11, 2024

clairelin135 Mar 11, 2024

clairelin135 Mar 11, 2024

hellendag left a comment

hellendag Mar 11, 2024

clairelin135 Mar 11, 2024

hellendag Mar 11, 2024

hellendag Mar 11, 2024

clairelin135 Mar 11, 2024

clairelin135 commented Mar 11, 2024

salazarm Mar 11, 2024 •

edited

Loading

clairelin135 Mar 11, 2024

salazarm commented Mar 12, 2024

salazarm commented Mar 12, 2024

clairelin135 Mar 12, 2024

salazarm Mar 12, 2024 •

edited

Loading

clairelin135 Mar 12, 2024

salazarm left a comment

[search ui] Add asset filter fields to search index #20372

[search ui] Add asset filter fields to search index #20372

Conversation

clairelin135 commented Mar 8, 2024 • edited Loading

clairelin135 commented Mar 8, 2024 • edited Loading

github-actions bot commented Mar 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellendag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clairelin135 commented Mar 11, 2024

salazarm Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salazarm commented Mar 12, 2024

salazarm commented Mar 12, 2024

Choose a reason for hiding this comment

salazarm Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salazarm left a comment

Choose a reason for hiding this comment

clairelin135 commented Mar 8, 2024 •

edited

Loading

clairelin135 commented Mar 8, 2024 •

edited

Loading

github-actions bot commented Mar 8, 2024 •

edited

Loading

salazarm Mar 11, 2024 •

edited

Loading

salazarm Mar 12, 2024 •

edited

Loading