[dagster-airlift][datasets 3/3] dataset filtering #29164
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Apr 10, 2025
Merged
dpeng817
changed the title
This was referenced Apr 11, 2025
[dagster-airlift][jobs 1/n] move assets def construction to common code path; other refactors
#29206
Merged
dpeng817
force-pushed
the
dpeng817/dataset_assets
branch
from
da21f73 to
63fbddc
Compare
dpeng817
force-pushed
the
dpeng817/dataset_filtering
branch
from
3c88083 to
3812316
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Comment on lines
+192
to
+195
| if ( | ||
| retrieval_filter.dataset_uri_ilike | ||
| and retrieval_filter.dataset_uri_ilike not in dataset.uri | ||
| ): |
There was a problem hiding this comment.
The current implementation checks for substring presence with retrieval_filter.dataset_uri_ilike not in dataset.uri, which doesn't accurately simulate SQL's ILIKE operator behavior. For better fidelity with the real implementation, consider using case-insensitive pattern matching (e.g., with regex) or at minimum a case-insensitive substring check. This would more closely match how ILIKE works in the actual SQL queries used by the production code.
Suggested change
| if ( | |
| retrieval_filter.dataset_uri_ilike | |
| and retrieval_filter.dataset_uri_ilike not in dataset.uri | |
| ): | |
| if ( | |
| retrieval_filter.dataset_uri_ilike | |
| and retrieval_filter.dataset_uri_ilike.lower() not in dataset.uri.lower() | |
| ): |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
OwenKephart
approved these changes
Apr 15, 2025
| ) -> Sequence["Dataset"]: | ||
| params: dict[str, Any] = {"limit": limit, "offset": offset} | ||
| if retrieval_filter.dataset_uri_ilike: | ||
| params["uri_pattern"] = retrieval_filter.dataset_uri_ilike |
There was a problem hiding this comment.
would prefer to match the python name to the REST API name, e.g. just rename dataset_uri_ilike to dataset_uri_pattern.
This was referenced Apr 15, 2025
Closed
This was referenced Apr 17, 2025
dpeng817
force-pushed
the
dpeng817/dataset_assets
branch
from
63fbddc to
339f24e
Compare
dpeng817
force-pushed
the
dpeng817/dataset_filtering
branch
from
3812316 to
28ea70e
Compare
prha
approved these changes
Apr 18, 2025
dpeng817
force-pushed
the
dpeng817/dataset_assets
branch
from
339f24e to
de133c2
Compare
dpeng817
force-pushed
the
dpeng817/dataset_filtering
branch
2 times, most recently
from
ae9608c to
730f322
Compare
dpeng817
force-pushed
the
dpeng817/dataset_assets
branch
from
de133c2 to
d67e1e0
Compare
dpeng817
force-pushed
the
dpeng817/dataset_filtering
branch
from
730f322 to
ccef52b
Compare
dpeng817
force-pushed
the
dpeng817/dataset_assets
branch
from
d67e1e0 to
2b29b96
Compare
dpeng817
force-pushed
the
dpeng817/dataset_filtering
branch
from
ccef52b to
fb6bb06
Compare
This was referenced Apr 25, 2025
Closed
dpeng817
force-pushed
the
dpeng817/dataset_assets
branch
from
2b29b96 to
54e61fb
Compare
dpeng817
force-pushed
the
dpeng817/dataset_filtering
branch
from
fb6bb06 to
c375273
Compare
This was referenced Apr 30, 2025
Merged
Merged
dpeng817
force-pushed
the
dpeng817/dataset_assets
branch
from
54e61fb to
0eeaed4
Compare
dpeng817
force-pushed
the
dpeng817/dataset_filtering
branch
from
c375273 to
4f62ded
Compare
dpeng817
force-pushed
the
dpeng817/dataset_assets
branch
from
0eeaed4 to
0cbd753
Compare
dpeng817
force-pushed
the
dpeng817/dataset_filtering
branch
from
4f62ded to
c1b22dd
Compare
…de path; other refactors (#29206) ## Summary & Motivation Right now we're separately coercing specs to assets definitions across many different code paths. Instead consolidate into a single code path. Additionally, get rid of the build_defs method on the state loader. We're not using it, we're instead pulling state out of thin air using get_or_fetch_state. ## How I Tested These Changes Existing tests
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit 4e0e816. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary & Motivation
Adds filtering properties for datasets.
How I Tested These Changes
Additional tests.
Changelog