-
-
Notifications
You must be signed in to change notification settings - Fork 316
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
Update set_partial_values to accept Buffer rather than BytesLike #2688
base: main
Are you sure you want to change the base?
Update set_partial_values to accept Buffer rather than BytesLike #2688
Conversation
""" | ||
assert not store.read_only | ||
# Create empty key | ||
await store.set(key, self.buffer_cls.from_bytes(b"")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set_partial_values
for a LocalStore
requires that a key exist. This test assumes this is the intended behavior and creates a key containing no data prior to testing set_partial_values.
@@ -38,7 +38,7 @@ class MemoryStore(Store): | |||
|
|||
supports_writes: bool = True | |||
supports_deletes: bool = True | |||
supports_partial_writes: bool = True | |||
supports_partial_writes: bool = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm realizing this may be wrong. I interpreted supports_partial_writes
to refer to whether set_partial_values
is implemented whereas it may actually refer to whether set
includes a byte_range
parameter. I am not sure which, if either, is correct because MemoryStore
had supports_partial_writes=True
with byte_range
implemented for set
but no implemented set_partial_values
whereas LocalStore had supports_partial_writes=True
with no byte_range
implemented for set
but included a set_partial_values
implementation.
When working on #2614 in https://github.com/maxrjones/zarr-python/tree/testing-storage, I found that the current implementation of
set_partial_values
for local store cannot work as written because it passesBytesLike
to_put
which expects a buffer. None of the other stores implementset_partial_values
, so it's not possible to look to them for the correct way of implementing this method.I believe that one of the following needs to happen in order for stores to correctly implement
set_partial_values
:prototype
argument toset_partial_values
that can be used to go from BytesLike -> Bufferset_partial_values
to acceptBuffer
types rather thanBytesLike
typesI chose the second option for this PR because all the other
set
methods seem to acceptBuffer
types. Of course, I welcome corrections if I am not understanding something here and the function signature should work as-is.