Fix ds.merge to prevent altering original object depending on join value (#10596)

Co-authored-by: Justus Magin <[email protected]>
Co-authored-by: Deepak Cherian <[email protected]>

Author: 3 people

doc/whats-new.rst

@@ -61,6 +61,8 @@ Bug fixes
   By `Deepak Cherian <https://github.com/dcherian>`_.
 - Fix detection of the ``h5netcdf`` backend. Xarray now selects ``h5netcdf`` if the default ``netCDF4`` engine is not available (:issue:`10401`, :pull:`10557`).
   By `Scott Staniewicz <https://github.com/scottstanie>`_.
+- Fix :py:func:`merge` to prevent altering original object depending on join value (:pull:`10596`)
+  By `Julia Signell <https://github.com/jsignell>`_.
 - Ensure ``unlimited_dims`` passed to :py:meth:`xarray.DataArray.to_netcdf`, :py:meth:`xarray.Dataset.to_netcdf` or :py:meth:`xarray.DataTree.to_netcdf` only contains dimensions present in the object; raise ``ValueError`` otherwise (:issue:`10549`, :pull:`10608`).
   By `Kai Mühlbauer <https://github.com/kmuehlbauer>`_.
 

xarray/core/variable.py

@@ -971,7 +971,6 @@ def _replace(
             data = copy.copy(self.data)
         if attrs is _default:
             attrs = copy.copy(self._attrs)
-
         if encoding is _default:
             encoding = copy.copy(self._encoding)
         return type(self)(dims, data, attrs, encoding, fastpath=True)

xarray/structure/merge.py

@@ -276,6 +276,7 @@ def merge_collected(
             if index is not None:
                 merged_indexes[name] = index
         else:
+            attrs: dict[Any, Any] = {}
             indexed_elements = [
                 (variable, index)
                 for variable, index in elements_list
@@ -306,13 +307,7 @@ def merge_collected(
                     [var.attrs for var, _ in indexed_elements],
                     combine_attrs=combine_attrs,
                 )
-                if variable.attrs or attrs:
-                    # Make a shallow copy to so that assigning merged_vars[name].attrs
-                    # does not affect the original input variable.
-                    merged_vars[name] = variable.copy(deep=False)
-                    merged_vars[name].attrs = attrs
-                else:
-                    merged_vars[name] = variable
+                merged_vars[name] = variable
                 merged_indexes[name] = index
             else:
                 variables = [variable for variable, _ in elements_list]
@@ -342,10 +337,15 @@ def merge_collected(
                         raise
 
                 if name in merged_vars:
-                    merged_vars[name].attrs = merge_attrs(
+                    attrs = merge_attrs(
                         [var.attrs for var in variables], combine_attrs=combine_attrs
                     )
 
+            if name in merged_vars and (merged_vars[name].attrs or attrs):
+                # Ensure that assigning attrs does not affect the original input variable.
+                merged_vars[name] = merged_vars[name].copy(deep=False)
+                merged_vars[name].attrs = attrs
+
     return merged_vars, merged_indexes
 
 

xarray/tests/test_merge.py

@@ -365,13 +365,16 @@ def test_merge(self):
         with pytest.raises(ValueError, match=r"should be coordinates or not"):
             data.merge(data.reset_coords())
 
-    def test_merge_drop_attrs(self):
+    @pytest.mark.parametrize(
+        "join", ["outer", "inner", "left", "right", "exact", "override"]
+    )
+    def test_merge_drop_attrs(self, join):
         data = create_test_data()
         ds1 = data[["var1"]]
         ds2 = data[["var3"]]
         ds1.coords["dim2"].attrs["keep me"] = "example"
         ds2.coords["numbers"].attrs["foo"] = "bar"
-        actual = ds1.merge(ds2, combine_attrs="drop")
+        actual = ds1.merge(ds2, combine_attrs="drop", join=join)
         assert actual.coords["dim2"].attrs == {}
         assert actual.coords["numbers"].attrs == {}
         assert ds1.coords["dim2"].attrs["keep me"] == "example"