Fix DirectoryStore (#773)

* Drop skip_if_nested_chunks from test_storage.py

* Add failing nested test

* Make DirectoryStore dimension_separator aware

* Migrate key logic to core rather than storage

Previous tests (now commented out) used logic in the store
classes to convert "0/0" keys into "0.0" keys, forcing the
store to be aware of array details. This tries to swap the
logic so that stores are responsible for passing dimension
separator values down to the arrays only. Since arrays can
also get the dimension_separator value from a .zarray file
they are now in charge.

* Fix linting in new test

* Extend the test suite for dim_sep

* Try fsspec 2021.7 (see #802)

* Revert "Try fsspec 2021.7 (see #802)"

This reverts commit 68adca5.

* Fix N5Store

* Re-activate contested N5 test

Author: joshmoore

fixture/flat/.zarray

@@ -0,0 +1,22 @@
+{
+    "chunks": [
+        2,
+        2
+    ],
+    "compressor": {
+        "blocksize": 0,
+        "clevel": 5,
+        "cname": "lz4",
+        "id": "blosc",
+        "shuffle": 1
+    },
+    "dtype": "<i8",
+    "fill_value": 0,
+    "filters": null,
+    "order": "C",
+    "shape": [
+        2,
+        2
+    ],
+    "zarr_format": 2
+}

fixture/flat/0.0

@@ -0,0 +1,23 @@
+{
+    "chunks": [
+        2,
+        2
+    ],
+    "compressor": {
+        "blocksize": 0,
+        "clevel": 5,
+        "cname": "lz4",
+        "id": "blosc",
+        "shuffle": 1
+    },
+    "dimension_separator": "/",
+    "dtype": "<i8",
+    "fill_value": 0,
+    "filters": null,
+    "order": "C",
+    "shape": [
+        2,
+        2
+    ],
+    "zarr_format": 2
+}

fixture/nested/.zarray

@@ -1952,7 +1952,7 @@ def _process_for_setitem(self, ckey, chunk_selection, value, fields=None):
         return self._encode_chunk(chunk)
 
     def _chunk_key(self, chunk_coords):
-        return self._key_prefix + '.'.join(map(str, chunk_coords))
+        return self._key_prefix + self._dimension_separator.join(map(str, chunk_coords))
 
     def _decode_chunk(self, cdata, start=None, nitems=None, expected_shape=None):
         # decompress

fixture/nested/0/0

@@ -295,7 +295,7 @@ def invert_chunk_coords(key):
         last_segment = segments[-1]
         if _prog_ckey.match(last_segment):
             coords = list(last_segment.split('.'))
-            last_segment = '.'.join(coords[::-1])
+            last_segment = '/'.join(coords[::-1])
             segments = segments[:-1] + [last_segment]
             key = '/'.join(segments)
     return key

zarr/core.py

@@ -948,12 +948,37 @@ def dir_path(self, path=None):
         return dir_path
 
     def listdir(self, path=None):
+        return self._dimension_separator == "/" and \
+            self._nested_listdir(path) or self._flat_listdir(path)
+
+    def _flat_listdir(self, path=None):
         dir_path = self.dir_path(path)
         if os.path.isdir(dir_path):
             return sorted(os.listdir(dir_path))
         else:
             return []
 
+    def _nested_listdir(self, path=None):
+        children = self._flat_listdir(path=path)
+        if array_meta_key in children:
+            # special handling of directories containing an array to map nested chunk
+            # keys back to standard chunk keys
+            new_children = []
+            root_path = self.dir_path(path)
+            for entry in children:
+                entry_path = os.path.join(root_path, entry)
+                if _prog_number.match(entry) and os.path.isdir(entry_path):
+                    for dir_path, _, file_names in os.walk(entry_path):
+                        for file_name in file_names:
+                            file_path = os.path.join(dir_path, file_name)
+                            rel_path = file_path.split(root_path + os.path.sep)[1]
+                            new_children.append(rel_path.replace(os.path.sep, '.'))
+                else:
+                    new_children.append(entry)
+            return sorted(new_children)
+        else:
+            return children
+
     def rename(self, src_path, dst_path):
         store_src_path = normalize_storage_path(src_path)
         store_dst_path = normalize_storage_path(dst_path)
@@ -1315,49 +1340,12 @@ def __init__(self, path, normalize_keys=False, dimension_separator="/"):
                 "NestedDirectoryStore only supports '/' as dimension_separator")
         self._dimension_separator = dimension_separator
 
-    def __getitem__(self, key):
-        key = _nested_map_ckey(key)
-        return super().__getitem__(key)
-
-    def __setitem__(self, key, value):
-        key = _nested_map_ckey(key)
-        super().__setitem__(key, value)
-
-    def __delitem__(self, key):
-        key = _nested_map_ckey(key)
-        super().__delitem__(key)
-
-    def __contains__(self, key):
-        key = _nested_map_ckey(key)
-        return super().__contains__(key)
-
     def __eq__(self, other):
         return (
             isinstance(other, NestedDirectoryStore) and
             self.path == other.path
         )
 
-    def listdir(self, path=None):
-        children = super().listdir(path=path)
-        if array_meta_key in children:
-            # special handling of directories containing an array to map nested chunk
-            # keys back to standard chunk keys
-            new_children = []
-            root_path = self.dir_path(path)
-            for entry in children:
-                entry_path = os.path.join(root_path, entry)
-                if _prog_number.match(entry) and os.path.isdir(entry_path):
-                    for dir_path, _, file_names in os.walk(entry_path):
-                        for file_name in file_names:
-                            file_path = os.path.join(dir_path, file_name)
-                            rel_path = file_path.split(root_path + os.path.sep)[1]
-                            new_children.append(rel_path.replace(os.path.sep, '.'))
-                else:
-                    new_children.append(entry)
-            return sorted(new_children)
-        else:
-            return children
-
 
 # noinspection PyPep8Naming
 class ZipStore(MutableMapping):

zarr/n5.py

@@ -0,0 +1,75 @@
+import pytest
+from numpy.testing import assert_array_equal
+
+import zarr
+from zarr.core import Array
+from zarr.storage import (DirectoryStore, NestedDirectoryStore, FSStore)
+from zarr.tests.util import have_fsspec
+
+
+@pytest.fixture(params=("static_nested",
+                        "static_flat",
+                        "directory_nested",
+                        "directory_flat",
+                        "directory_default",
+                        "nesteddirectory_nested",
+                        "nesteddirectory_default",
+                        "fs_nested",
+                        "fs_flat",
+                        "fs_default"))
+def dataset(tmpdir, request):
+    """
+    Generate a variety of different Zarrs using
+    different store implementations as well as
+    different dimension_separator arguments.
+    """
+
+    loc = tmpdir.join("dim_sep_test.zarr")
+    which = request.param
+    kwargs = {}
+
+    if which.startswith("static"):
+        if which.endswith("nested"):
+            return "fixture/nested"
+        else:
+            return "fixture/flat"
+
+    if which.startswith("directory"):
+        store_class = DirectoryStore
+    elif which.startswith("nested"):
+        store_class = NestedDirectoryStore
+    else:
+        if have_fsspec is False:
+            pytest.skip("no fsspec")
+        store_class = FSStore
+        kwargs["mode"] = "w"
+        kwargs["auto_mkdir"] = True
+
+    if which.endswith("nested"):
+        kwargs["dimension_separator"] = "/"
+    elif which.endswith("flat"):
+        kwargs["dimension_separator"] = "."
+
+    store = store_class(str(loc), **kwargs)
+    zarr.creation.array(store=store, data=[[1, 2], [3, 4]])
+    return str(loc)
+
+
+def verify(array):
+    assert_array_equal(array[:], [[1, 2], [3, 4]])
+
+
+def test_open(dataset):
+    verify(zarr.open(dataset))
+
+
+def test_fsstore(dataset):
+    verify(Array(store=FSStore(dataset)))
+
+
+def test_directory(dataset):
+    verify(zarr.Array(store=DirectoryStore(dataset)))
+
+
+def test_nested(dataset):
+    verify(Array(store=NestedDirectoryStore(dataset)))

zarr/storage.py

@@ -804,12 +804,16 @@ def test_pickle(self):
 
 class TestDirectoryStore(StoreTests):
 
-    def create_store(self, normalize_keys=False, **kwargs):
-        skip_if_nested_chunks(**kwargs)
-
+    def create_store(self,
+                     normalize_keys=False,
+                     dimension_separator=".",
+                     **kwargs):
         path = tempfile.mkdtemp()
         atexit.register(atexit_rmtree, path)
-        store = DirectoryStore(path, normalize_keys=normalize_keys, **kwargs)
+        store = DirectoryStore(path,
+                               normalize_keys=normalize_keys,
+                               dimension_separator=dimension_separator,
+                               **kwargs)
         return store
 
     def test_filesystem_path(self):
@@ -1150,10 +1154,10 @@ def test_chunk_nesting(self):
         # any path where last segment looks like a chunk key gets special handling
         store['0.0'] = b'xxx'
         assert b'xxx' == store['0.0']
-        assert b'xxx' == store['0/0']
+        # assert b'xxx' == store['0/0']
         store['foo/10.20.30'] = b'yyy'
         assert b'yyy' == store['foo/10.20.30']
-        assert b'yyy' == store['foo/10/20/30']
+        # assert b'yyy' == store['foo/10/20/30']
         store['42'] = b'zzz'
         assert b'zzz' == store['42']
 
@@ -1198,7 +1202,7 @@ def test_chunk_nesting(self):
         store['0.0'] = b'xxx'
         assert '0.0' in store
         assert b'xxx' == store['0.0']
-        assert b'xxx' == store['0/0']
+        # assert b'xxx' == store['0/0']
         store['foo/10.20.30'] = b'yyy'
         assert 'foo/10.20.30' in store
         assert b'yyy' == store['foo/10.20.30']