Fix double counting V3 groups bug (#1268)

* fix double counting groups bug

* add release notes

Co-authored-by: Josh Moore <[email protected]>

Author: rabernat

docs/release.rst

@@ -14,6 +14,12 @@ Unreleased
     # .. warning::
     #    Pre-release! Use :command:`pip install --pre zarr` to evaluate this release.
 
+* Fix bug that caused double counting of groups in ``groups()`` and ``group_keys()``
+  methods with V3 stores.
+  By :user:`Ryan Abernathey <rabernat>` :issue:`1228`.
+
+.. _release_2.13.2:
+
 Maintenance
 ~~~~~~~~~~~
 

zarr/hierarchy.py

@@ -511,9 +511,15 @@ def group_keys(self):
         else:
             dir_name = meta_root + self._path
             group_sfx = '.group' + self._metadata_key_suffix
-            for key in sorted(listdir(self._store, dir_name)):
+            # The fact that we call sorted means this can't be a streaming generator.
+            # The keys are already in memory.
+            all_keys = sorted(listdir(self._store, dir_name))
+            for key in all_keys:
                 if key.endswith(group_sfx):
                     key = key[:-len(group_sfx)]
+                    if key in all_keys:
+                        # otherwise we will double count this group
+                        continue
                 path = self._key_prefix + key
                 if path.endswith(".array" + self._metadata_key_suffix):
                     # skip array keys
@@ -552,24 +558,16 @@ def groups(self):
                         zarr_version=self._version)
 
         else:
-            dir_name = meta_root + self._path
-            group_sfx = '.group' + self._metadata_key_suffix
-            for key in sorted(listdir(self._store, dir_name)):
-                if key.endswith(group_sfx):
-                    key = key[:-len(group_sfx)]
+            for key in self.group_keys():
                 path = self._key_prefix + key
-                if path.endswith(".array" + self._metadata_key_suffix):
-                    # skip array keys
-                    continue
-                if contains_group(self._store, path, explicit_only=False):
-                    yield key, Group(
-                        self._store,
-                        path=path,
-                        read_only=self._read_only,
-                        chunk_store=self._chunk_store,
-                        cache_attrs=self.attrs.cache,
-                        synchronizer=self._synchronizer,
-                        zarr_version=self._version)
+                yield key, Group(
+                    self._store,
+                    path=path,
+                    read_only=self._read_only,
+                    chunk_store=self._chunk_store,
+                    cache_attrs=self.attrs.cache,
+                    synchronizer=self._synchronizer,
+                    zarr_version=self._version)
 
     def array_keys(self, recurse=False):
         """Return an iterator over member names for arrays only.

zarr/tests/test_hierarchy.py

@@ -770,6 +770,19 @@ def visitor1(val, *args):
 
         g1.store.close()
 
+    # regression test for https://github.com/zarr-developers/zarr-python/issues/1228
+    def test_double_counting_group_v3(self):
+        root_group = self.create_group()
+        group_names = ["foo", "foo-", "foo_"]
+        for name in group_names:
+            sub_group = root_group.create_group(name)
+            sub_group.create("bar", shape=10, dtype="i4")
+        assert list(root_group.group_keys()) == sorted(group_names)
+        assert list(root_group.groups()) == [
+            (name, root_group[name])
+            for name in sorted(group_names)
+        ]
+
     def test_empty_getitem_contains_iterators(self):
         # setup
         g = self.create_group()