Skip to content
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

Clarify Store Interface #638

Closed
Carreau opened this issue Oct 22, 2020 · 2 comments
Closed

Clarify Store Interface #638

Carreau opened this issue Oct 22, 2020 · 2 comments

Comments

@Carreau
Copy link
Contributor

Carreau commented Oct 22, 2020

prompted by this discussion

The current Store (in the sens of Zarr Python implementation) interface is ill-defined, in particular dict() seem to be special cased in many area:

  • tests: many test use dict, while assuming the behavior we see will be true for all stores.
  • it is said that a Zarr Store is anything exposing the MutableMapping, though MutableMapping have the property that: mm[key] = x ; y = mm[key]; assert x == y which is currently untrue, as many stores are responsible from encoding/ensuring bytes.
  • class that would behave like dict/MutableMapping but not be subclass of dict would fail some tests, and return non-bytes in __getitems__.

Personally I believe that currently trying to say that stores are MutableMapping and dict is an acceptable store is the exception to the rule; proof being that there is a MemoryStore.

@Carreau
Copy link
Contributor Author

Carreau commented Oct 27, 2020

The test also use the following mock:

# custom store, does not support getsize()
class CustomMapping(object):

    def __init__(self):
        self.inner = dict()

    def keys(self):
        return self.inner.keys()

    def values(self):
        return self.inner.values()

    def get(self, item, default=None):
        try:
            return self.inner[item]
        except KeyError:
            return default

    def __getitem__(self, item):
        return self.inner[item]

    def __setitem__(self, item, value):
        self.inner[item] = ensure_bytes(value)

    def __delitem__(self, key):
        del self.inner[key]

    def __contains__(self, item):
        return item in self.inner

This is actually not a custom mapping as it does not implement __iter__ and thus the idiom

for k in CustomMapping():
   ...

Fails, as it recognize this as a sequence, and try to get keys, 0, 1, 2 ..., we should clarify whether stores can list their keys or not (my guess is not, as stores may not be listable), but the above should likely be fixed in the test; Either it's a real mapping so should implement iter, or shoudl be a real store.

@jhamman
Copy link
Member

jhamman commented Feb 3, 2024

Going to close this as the v3 spec + the v3 design have addressed this issue.

@jhamman jhamman closed this as completed Feb 3, 2024
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

No branches or pull requests

2 participants