[aladinor]

Description

Closes #11303.

Restructure _decode_cf_datetime_dtype (xarray/coding/times.py) to infer the decoded dtype from metadata first and fall back to reading the first and last values only when metadata cannot prove safety.

The metadata-first path handles explicit use_cftime, non-standard calendars, non-"ns" time_unit, non-chunked data, valid_max / time_coverage_end attrs that bound values, and narrow integer storage dtypes (int8/16/32) where overflow is provably impossible. Safe paths run decode_cf_datetime on a synthetic [0, 1] array so resolution logic matches real-data output.

Also adds a runtime _verify_decoded_dtype guard on CFDatetimeCoder.decode: if decode_cf_datetime later produces a different datetime/object dtype kind than was declared (the rare overflow case), raise TypeError instead of letting dask's map_blocks silently cast into garbage. This turns a latent silent-corruption edge case into a clean error even on the fallback path.

Benchmark — nexrad-arco/KLOT icechunk DataTree, 107 groups, 106 time variables

cProfile on unpatched main:

total open_datatree: 77.56s
_decode_cf_datetime_dtype: 35.17s (45% of total)
  ├─ first_n_items: 25.16s (32%)
  └─ last_item: 9.51s (12%)
decode_cf_datetime (actual CF decode): 0.09s (0.1%)

With this PR:

State	open_datatree	time in _decode_cf_datetime_dtype
unpatched	77.56s	35.17s (45%)
patched	~19.7s	~0s (106/106 variables hit the metadata fast path)

Reproducer in #11303.

Behavior

• Public decode_times / CFDatetimeCoder signatures unchanged.
• Default behavior unchanged for well-formed data.
• Malformed units still raises ValueError at open time (from the metadata path's unit validation).
• The dask-silent-corruption case is now a clean TypeError at access time rather than silent garbage — strictly safer.

Checklist

• Closes _decode_cf_datetime_dtype triggers 2 store reads per time variable — expensive on remote stores #11303
• Tests added (TestDecodeCFDatetimeDtype in test_coding_times.py, using InaccessibleArray to prove no reads on the fast paths)
• User visible changes documented in whats-new.rst

AI Disclosure

• This PR contains AI-generated content.
  • I have tested any AI-generated content in my PR.
  • I take responsibility for any AI-generated content in my PR.
    Tools: Claude Code

[kmuehlbauer]

@aladinor Sorry for the delay here. I've responded in the linked issue:

#11303 (comment)

Any chance you could have a look? That way we could separate concerns and keep fully backwards compatible. Thanks!

[aladinor]

@kmuehlbauer, I've implemented what you suggested. Please let me know your thoughts.

[aladinor]

Quick update — the branch is now merged with the latest main, and I've verified the implementation locally:

• Full xarray suite: 19,545 passed, 0 failures
• Targeted TestInferDtypeOptIn (both infer_dtype="data" and "metadata" paths): 9/9 passing
• pre-commit clean, mypy clean on the files this PR touches (xarray/coding/times.py, xarray/backends/zarr.py)

The 9 CI test-job failures and the Mypy CI Additional failure are all pre-existing on main, not from this PR:

• All 9 test-job failures hit the same properties/test_pandas_roundtrip.py::test_roundtrip_pandas_dataframe_datetime Hypothesis flake (random tz-aware datetime example fails decoding)
• The Mypy failure is the known "Group" has no attribute "create_dataset" at xarray/tests/test_backends.py:7755

Whenever you have time for another look, @kmuehlbauer, much appreciated!

[ianhi]

Is the assumption here that metadata inference should always return the same exact answer as data? If so we should have a test that compares them accorss possible inputs. AIed up a quick one which fails:

    @requires_cftime
    @pytest.mark.parametrize("calendar", [None, "standard", "noleap", "360_day"])
    @pytest.mark.parametrize("use_cftime", [None, True, False])
    @pytest.mark.parametrize("time_unit", ["s", "ms", "us", "ns"])
    def test_metadata_matches_data_path(self, calendar, use_cftime, time_unit):
        # The contract of infer_dtype="metadata" is that it predicts what
        # infer_dtype="data" produces without reading the data: the same
        # dtype on success, or the same failure. The data path is the oracle.
        attrs = {"units": "days since 2000-01-01"}
        if calendar is not None:
            attrs["calendar"] = calendar
        variable = Variable(("t",), np.array([0, 1, 2], dtype="int64"), attrs=attrs)

        def decode(infer_dtype):
            coder = CFDatetimeCoder(
                use_cftime=use_cftime, time_unit=time_unit, infer_dtype=infer_dtype
            )
            return coder.decode(variable).dtype

        try:
            expected = decode("data")
        except ValueError:
            with pytest.raises(ValueError):
                decode("metadata")
        else:
            assert decode("metadata") == expected

which fails wiht e.g.:

FAILED xarray/tests/test_coding_times.py::TestInferDtypeOptIn::test_metadata_matches_data_path[s-False-noleap] - Failed: DID NOT RAISE <class 'ValueError'>
FAILED xarray/tests/test_coding_times.py::TestInferDtypeOptIn::test_metadata_matches_data_path[s-False-360_day] - Failed: DID NOT RAISE <class 'ValueError'>
FAILED xarray/tests/test_coding_times.py::TestInferDtypeOptIn::test_metadata_matches_data_path[ms-False-noleap] - Failed: DID NOT RAISE <class 'ValueError'>
FAILED xarray/tests/test_coding_times.py::TestInferDtypeOptIn::test_metadata_matches_data_path[ms-False-360_day] - Failed: DID NOT RAISE <class 'ValueError'>
FAILED xarray/tests/test_coding_times.py::TestInferDtypeOptIn::test_metadata_matches_data_path[us-False-noleap] - Failed: DID NOT RAISE <class 'ValueError'>
FAILED xarray/tests/test_coding_times.py::TestInferDtypeOptIn::test_metadata_matches_data_path[us-False-360_day] - Failed: DID NOT RAISE <class 'ValueError'>
FAILED xarray/tests/test_coding_times.py::TestInferDtypeOptIn::test_metadata_matches_data_path[ns-False-noleap] - Failed: DID NOT RAISE <class 'ValueError'>
FAILED xarray/tests/test_coding_times.py::TestInferDtypeOptIn::test_metadata_matches_data_path[ns-False-360_day] - Failed: DID NOT RAISE <class 'ValueError'>

or are there known limitations that should be documented here?

[kmuehlbauer]

Thanks @aladinor for updating the PR and for following-up on @ianhi's comment. I went through the changes and the code flow looks sound. I think this would be a nice performance addition in any case.

I'd really like to get @spencerkclark's eyes on this one, too

JFTR: Actually, this is a nice showcase how splitting out coders helps adding features.

[dcherian]

I agree, this seems reasonable and perhaps should be the default. Since the failure just comes later, and we almost always decode datetim64 eagerly on open, it seems pretty safe. The one exception is the "time bounds" variable which stays lazy till compute-time, but perhaps that's OK.

[aladinor]

Thanks, @kmuehlbauer, for your prompt reply.

@ianhi, indeed, there were several scenarios where the metadata path didn't match the data path, including the ones mentioned in your comment. I've refactored it to make sure the metadata path now reuses the data path. It decodes and reads only the first and last elements in the array when metadata cannot infer the dtype. It now also raises an error instead of silently casting mismatched values. I also added and extended your proposed test.

[kmuehlbauer]

@aladinor Please resolve the conflicts, this can go in!

[kmuehlbauer]

Thanks @aladinor!