Preserve list attributes when reading with h5netcdf

doc/whats-new.rst

@@ -49,6 +49,22 @@ Deprecations
 Bug Fixes
 ~~~~~~~~~
 
+- The ``h5netcdf`` engine no longer collapses list-valued attributes when it
+  reads them. A ``["one"]`` written by ``engine="h5netcdf"`` came back as
+  ``"one"``, and an empty list as ``array([], dtype=float64)``; both now come
+  back as the lists they were written as (:issue:`10275`). h5netcdf collapses
+  every length-1 attribute to a scalar to mirror netcdf4-python, which hides two
+  things the file does record: netCDF stores one string as ``NC_CHAR`` in a
+  scalar HDF5 dataspace and a sequence of strings as ``NC_STRING`` in a
+  non-scalar one, and a zero length attribute cannot be a scalar at all. Nothing
+  about how xarray writes files changed, so existing files are unaffected.
+
+  Numeric attributes are deliberately left alone: netcdf-c stores a scalar
+  number as a length-1 vector, so ``1`` and ``[1]`` are the same bytes on disk
+  and cannot be told apart. The ``netcdf4`` engine is also unchanged, since
+  netcdf4-python collapses length-1 attributes before xarray sees them and
+  offers no way to ask for the length.
+  By `Mark Harfouche <https://github.com/hmaarrfk>`_.
 - Fix async zarr tests using ``wraps`` with ``autospec=True`` on async methods,
   which caused ``AsyncMock`` objects to leak through instead of real array data
   (:pull:`11232`).

xarray/backends/h5netcdf_.py

@@ -74,11 +74,68 @@ def _getitem(self, key):
             return array[key]
 
 
+#: Sentinel for an attribute whose dataspace we cannot inspect.
+_UNKNOWN_DATASPACE = object()
+
+
+def _attribute_dataspace_shape(h5attrs, key):
+    """Shape of the HDF5 dataspace backing attribute ``key``.
+
+    ``()`` is a scalar dataspace, holding exactly one value. ``(n,)`` is a
+    simple dataspace, holding a sequence of ``n``. ``None`` is a null dataspace,
+    which netcdf-c writes for a zero length attribute. Returns
+    :py:data:`_UNKNOWN_DATASPACE` when the dataspace cannot be inspected.
+
+    h5netcdf hands back an already-decoded value that hides this distinction,
+    because it collapses any length-1 attribute to a scalar to mirror
+    netcdf4-python (:issue:`10275`), so go to the underlying h5py attribute.
+    """
+    if h5attrs is None:
+        return _UNKNOWN_DATASPACE
+    try:
+        return h5attrs.get_id(key).shape
+    except (AttributeError, KeyError, TypeError):
+        # ``get_id`` is part of h5py's low level API, which the pyfive backend
+        # does not provide.
+        return _UNKNOWN_DATASPACE
+
+
+def _restore_sequence_attribute(value, shape):
+    """Undo h5netcdf's collapse of a length-1 or zero-length attribute.
+
+    Only the cases the file unambiguously records are restored:
+
+    * a sequence of strings, which netCDF stores as NC_STRING in a non-scalar
+      dataspace, as opposed to the single NC_CHAR string it stores in a scalar
+      one;
+    * a zero length attribute, which cannot be a scalar whatever its dataspace.
+
+    Numeric attributes are left alone. netcdf-c stores a scalar number as a
+    length-1 vector, so ``1`` and ``[1]`` are the same bytes on disk and there
+    is nothing to tell them apart -- guessing would turn every ``scale_factor``
+    in every existing file into a list.
+    """
+    if shape is _UNKNOWN_DATASPACE:
+        return value
+    if isinstance(value, str | bytes):
+        # a scalar dataspace holds one string; anything else holds a sequence.
+        # ``shape is None`` is the zero length _FillValue h5netcdf returns as
+        # b"", which is not a sequence of strings.
+        if shape is not None and len(shape) > 0:
+            return [value]
+    elif isinstance(value, np.ndarray) and value.size == 0:
+        return []
+    return value
+
+
 def _read_attributes(h5netcdf_var):
     # GH451
     # to ensure conventions decoding works properly on Python 3, decode all
     # bytes attributes to strings
     attrs = {}
+    # ``Attributes`` wraps the h5py attributes it decodes; we need the raw ones
+    # to tell a single value from a sequence of one -- see GH10275.
+    h5attrs = getattr(h5netcdf_var.attrs, "_h5attrs", None)
     for k, v in h5netcdf_var.attrs.items():
         if k not in ["_FillValue", "missing_value"] and isinstance(v, bytes):
             try:
@@ -90,6 +147,7 @@ def _read_attributes(h5netcdf_var):
                     f"returning bytes undecoded.",
                     UnicodeWarning,
                 )
+        v = _restore_sequence_attribute(v, _attribute_dataspace_shape(h5attrs, k))
         attrs[k] = v
     return attrs
 

xarray/tests/test_backends.py

@@ -4850,6 +4850,50 @@ def test_cross_engine_read_write_netcdf4(self) -> None:
                     with open_dataset(tmp_file, engine=read_engine) as actual:
                         assert_identical(data, actual)
 
+    def test_roundtrip_string_sequence_attrs(self) -> None:
+        # GH10275: netCDF stores a single string as NC_CHAR and a sequence of
+        # strings as NC_STRING, so a length-1 sequence should not come back as
+        # a bare string.
+        original = Dataset(
+            {"x": ("t", [1.0], {"attr": ["one_item_only"]})},
+            attrs={
+                "one": ["one_item_only"],
+                "two": ["one_item", "two_items"],
+                "scalar": "plain",
+            },
+        )
+        with self.roundtrip(original) as actual:
+            assert actual.attrs["one"] == ["one_item_only"]
+            assert actual.attrs["two"] == ["one_item", "two_items"]
+            # a scalar string is not a sequence and must stay a scalar
+            assert actual.attrs["scalar"] == "plain"
+            assert actual["x"].attrs["attr"] == ["one_item_only"]
+
+    def test_roundtrip_empty_list_attr(self) -> None:
+        # GH10275: a zero length attribute cannot be a scalar whatever its
+        # dataspace, so it is restored as an empty list rather than handed back
+        # as the empty float64 array h5netcdf types it as. This holds for files
+        # written by the netCDF4 engine too, which stores it as a null dataspace.
+        original = Dataset({"x": ("t", [1.0], {"attr": []})}, attrs={"empty": []})
+        with self.roundtrip(original) as actual:
+            assert actual.attrs["empty"] == []
+            assert actual["x"].attrs["attr"] == []
+
+        with create_tmp_file() as tmp_file:
+            original.to_netcdf(tmp_file, engine="netcdf4")
+            with open_dataset(tmp_file, engine="h5netcdf") as actual:
+                assert actual.attrs["empty"] == []
+
+    def test_numeric_attrs_are_not_turned_into_lists(self) -> None:
+        # netcdf-c stores a scalar number as a length-1 vector, so 1 and [1] are
+        # the same bytes on disk. Guessing "sequence" would turn the scale_factor
+        # of every existing file into a list, so numbers are left alone (GH10275).
+        original = Dataset(attrs={"scale_factor": 0.01, "flags": [1, 2]})
+        with self.roundtrip(original, open_kwargs={"decode_cf": False}) as actual:
+            assert actual.attrs["scale_factor"] == 0.01
+            assert not isinstance(actual.attrs["scale_factor"], list)
+            assert_array_equal(actual.attrs["flags"], [1, 2])
+
     def test_read_byte_attrs_as_unicode(self) -> None:
         with create_tmp_file() as tmp_file:
             with nc4.Dataset(tmp_file, "w") as nc: