add and remove methods on SerializableEntitySubset #29607
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
| f"Cannot add {value} to subset. The types are incompatible. EntitySubset is not partitioned" | ||
| ) | ||
|
|
||
| def add(self, value: EntitySubsetValue) -> "SerializableEntitySubset": |
There was a problem hiding this comment.
this API feels a bit odd for non-partitioned assets when i use it. Maybe for non-partitioned assets you pass None instead of the boolean, but that strays from the conventions of the class.
Like this is just a bit awkward imo
new_subset = existing_subset.remove(True)
# does the same as
new_subset = existing_subset.remove(False)
jamiedemaria
force-pushed
the
jamie/add-remove-methods-on-entity-subset
branch
from
4b6c3d7 to
8ef90a9
Compare
jamiedemaria
changed the base branch from
master
to
jamie/update-state-for-reconciler
There was a problem hiding this comment.
I think we should add compute_union and compute_intersection methods here (which match the language of the EntitySubset class).
They'd take in another EntitySubset, so it'd be the caller's responsibility to construct that properly.
That would also clear up the weirdness you noted with the boolean case -- it's more obvious why intersecting two subsets with one of them being empty would always result in the same output vs. the current situation
Summary & Motivation
allows adding and removing partition subsets/bools from
SerializableEntitySubsetused in https://github.com/dagster-io/internal/pull/15287
How I Tested These Changes
Changelog