Skip to content

change UI to narrow asset events query #28903

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 18, 2025
Merged

change UI to narrow asset events query #28903

merged 3 commits into from
Apr 18, 2025

Conversation

prha
Copy link
Member

@prha prha commented Mar 31, 2025

Summary & Motivation

This PR simplifies the data fetching for metadata plots in a couple ways.

Before:
For partitioned assets, we were using the partitionInLast parameter to filter materializations. This fetches all partition keys (expensive), and then grabs the last N materializations that show up in the last N partitions* (incorrect). We were also fetching the materialization in the most recent partition to represent the most recent materialization overall.

After:
We separate out the most recent materialization query to be just that, fetch the most recent materialization regardless of partition (this may or may not be the right decision, based on product, but it's easy to change). For partitioned assets, we now do a two-phased fetch. We first fetch the last N partition keys, using the new paginated partition key graphql fields. We then use those partition keys to pass into the latestMaterializationByPartition field, which gets the most recent materialization for the provided set of partition keys.

Breaking apart the partition history data fetching to be closer to the graphs component also means that the main components become interactive much earlier.

This should result in better correctness and massively reduce overfetching. This should also result in much better performance in Cloud.

How I Tested These Changes

Screen.Recording.2025-03-31.at.2.48.50.PM.mov

Changelog

Copy link
Member Author

prha commented Mar 31, 2025

Copy link

github-actions bot commented Mar 31, 2025

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-1s19fbl9m-elementl.vercel.app
https://prha-ui-partition-keys.core-storybook.dagster-docs.io

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

@prha prha force-pushed the prha/ui_partition_keys branch from 301de08 to b40b177 Compare April 1, 2025 22:30
@prha prha force-pushed the prha/graphql_partition_keys branch from 9b77cc2 to 1d6f53d Compare April 1, 2025 22:30
@prha prha force-pushed the prha/ui_partition_keys branch from b40b177 to 745a442 Compare April 1, 2025 22:34
@prha prha force-pushed the prha/graphql_partition_keys branch from 9842748 to 63408e2 Compare April 1, 2025 23:15
@prha prha force-pushed the prha/ui_partition_keys branch 2 times, most recently from 895733a to c4a0e83 Compare April 2, 2025 00:41
@prha prha force-pushed the prha/graphql_partition_keys branch from 63408e2 to 66476ee Compare April 2, 2025 00:41
@prha prha marked this pull request as ready for review April 2, 2025 06:50
@prha prha force-pushed the prha/ui_partition_keys branch from c4a0e83 to 4759b17 Compare April 2, 2025 06:51
Copy link
Collaborator

@bengotow bengotow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome - left a few inline comments! I didn't realize that fetching the partition keys and then passing them in explicitly would be so much better, that's great :-o

const value = useMemo(() => {
const assetNode =
data?.assetNodeOrError.__typename === 'AssetNode' ? data?.assetNodeOrError : null;
const materializations = (assetNode?.latestMaterializationByPartition || []).filter((_) => !!_);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't think it's worth changing but if you want, you can also express this as filter(Boolean) (running each through the Boolean typecast essentially)

@prha
Copy link
Member Author

prha commented Apr 2, 2025

This looks awesome - left a few inline comments! I didn't realize that fetching the partition keys and then passing them in explicitly would be so much better, that's great :-o

Yeah, it's not the fact that prefetching is so much better. It's the fact that we were using resolvers on the GrapheneAsset before, and we're now using resolvers on the GrapheneAssetNode (the difference between assetOrError vs assetNodeOrError). The latter has access to the definition, which makes it much easier to grab the last N partitions and query the materializations cheaply by them. The former does not and thus has to scan through history.

We were also using the same query to generate graphs for the case where there is an asset definition and the case where there is not. This means that we were doing the inefficient fetch for both. This PR (and #28941) changes the query for the definition-based query to use the more efficient fetch and removes the UI altogether for the non-definition case.

@prha prha force-pushed the prha/ui_partition_keys branch from 4227fbc to 9d25a61 Compare April 3, 2025 23:51
@prha prha force-pushed the prha/graphql_partition_keys branch 2 times, most recently from 075048f to ca6890c Compare April 4, 2025 17:47
@prha prha force-pushed the prha/ui_partition_keys branch 2 times, most recently from 9f759c9 to 95db953 Compare April 4, 2025 20:40
@prha prha force-pushed the prha/graphql_partition_keys branch from ca6890c to dd30b2e Compare April 4, 2025 20:40
@prha prha force-pushed the prha/graphql_partition_keys branch 2 times, most recently from 1e4ec29 to 25bf7cb Compare April 15, 2025 17:05
Base automatically changed from prha/graphql_partition_keys to master April 15, 2025 18:10
@prha prha force-pushed the prha/ui_partition_keys branch from 95db953 to 3c5831b Compare April 18, 2025 18:25
@prha prha merged commit ceff96e into master Apr 18, 2025
7 checks passed
@prha prha deleted the prha/ui_partition_keys branch April 18, 2025 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants