Feature: store learns to delete prefixes when overwriting/creating hierarchy nodes #2430
Conversation
overwriting nodes - add Store.delete_dir and Store.delete_prefix - update array and group creation methods to call delete_dir - change list_prefix to return absolue keys
…into feature/store_erase_prefix
…python into feature/store_erase_prefix
| @@ -371,6 +371,36 @@ def list_dir(self, prefix: str) -> AsyncGenerator[str, None]: | |||
| """ | |||
| ... | |||
|
|
|||
| async def delete_dir(self, prefix: str, recursive: bool = True) -> None: | |||
| """ | |||
| Remove all keys and prefixes in the store that begin with a given prefix. | |||
There was a problem hiding this comment.
I think it would make life much easier for Store implementers if we declare that prefix will always be the path of a group or a array. Otherwise, all implementations need to deal with unnecessary edge cases like delete_dir("path/to/array/c/0") or worse.
Icechunk is an example of a store that could very cleanly and efficiently implement the case for a group/array prefix, but it would struggle to handle other prefixes.
There was a problem hiding this comment.
What is the use case for recursive = False ? If zarr itself doesn't have one, I'd remove the argument.
There was a problem hiding this comment.
I can see a use case for delete_dir("path/to/array/c/") (remove all chunks from an array) so I think we should allow that -- even if it makes life a little harder in icechunk.
There was a problem hiding this comment.
That's totally fine. Let's document the three possible prefix cases, and say that stores are allowed to do undefined behavior for other prefixes
| if not self.supports_listing: | ||
| raise NotImplementedError | ||
| self._check_writable() | ||
| async for key in self.list_prefix(prefix): |
There was a problem hiding this comment.
Can we document what's the expected return value of list_prefix? Absolute vs. relative, final "/" or not, etc.
There was a problem hiding this comment.
this is done in the list_prefix docstring
…into feature/store_erase_prefix
|
Looks good overall. I've been struggling a bit with whether Stores should be a bit more aware about the context the operation is occurring in (e.g. deleting a group vs a specific chunk of an array; I think this relates to the conversation at #2430 (comment)). But I think this is a good improvement on its own. |
|
@TomAugspurger / @d-v-b / @paraseba - any final comments here. I'd like to get this in ahead of the larger store mode refactor in #2442 |
Would defining these as "relative to the root of the store" be clearer? |
|
pre-commit.ci autofix |
This PR implements
delete_dirteaches the Group and Array classes to use it when overwriting nodes in a hierarchy.Also included here:
-delete_prefix- not used but perhaps we should just use it instead ofdelete_dirlist_prefixto return absolute paths instead of strippingprefix. Given thatlist_prefixwas unused before, this wasn't very disruptive but calling it out so others can help decide if this is what we want.Still needs:
delete_dirdelete_dircloses #2191
first steps toward #2108 and #2359
TODO: