Skip to content

Commit c294260

Browse files
committed
fix: infer surviving dims in to_dataset for aggregations (closes #189)
to_dataset() inferred dims only when a registered Dataset's entire dim set appeared in the result, so a GROUP BY that aggregates dims away (e.g. SELECT "time", AVG("air") ... GROUP BY "time" over a time/lat/lon grid) raised "dims cannot be inferred" instead of using the surviving group-by dim. _infer_dimension_columns now uses the registered dims that survive into the result columns, in the data-variable axis order, so such aggregations round-trip on the remaining dim(s). Several registered Datasets that imply different surviving dims still raise; a global aggregation with no surviving dim raises a clearer message. Repurposes the now-inferable test to a global-aggregation case and adds an auto-inference regression test.
1 parent d8ffa52 commit c294260

2 files changed

Lines changed: 65 additions & 25 deletions

File tree

tests/test_ds.py

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,42 @@ def test_aggregation_drops_dim(air_dataset_small):
139139
np.testing.assert_allclose(actual, expected)
140140

141141

142+
def test_aggregation_infers_dims(air_dataset_small):
143+
"""#189: to_dataset() infers the surviving GROUP BY dims without dims=."""
144+
ctx = XarrayContext()
145+
ctx.from_dataset("air", air_dataset_small)
146+
147+
# GROUP BY lat, lon: time is aggregated away, lat/lon survive.
148+
out = ctx.sql(
149+
"SELECT lat, lon, AVG(air) AS air_avg FROM air GROUP BY lat, lon"
150+
).to_dataset()
151+
assert set(out.dims) == {"lat", "lon"}
152+
assert "air_avg" in out.data_vars
153+
expected = (
154+
air_dataset_small.compute()
155+
.sortby(["lat", "lon"])
156+
.mean(dim="time")["air"]
157+
.values
158+
)
159+
np.testing.assert_allclose(
160+
out.sortby(["lat", "lon"])["air_avg"].values, expected
161+
)
162+
163+
# The reporter's exact case: GROUP BY the time coordinate.
164+
single = ctx.sql(
165+
'SELECT "time", AVG("air") AS air FROM "air" GROUP BY "time"'
166+
).to_dataset()
167+
assert set(single.dims) == {"time"}
168+
assert "air" in single.data_vars
169+
expected_t = (
170+
air_dataset_small.compute()
171+
.sortby("time")
172+
.mean(dim=["lat", "lon"])["air"]
173+
.values
174+
)
175+
np.testing.assert_allclose(single.sortby("time")["air"].values, expected_t)
176+
177+
142178
def test_barrier_query_scans_source_once(air_dataset_small):
143179
"""A barrier plan (aggregation) executes the source exactly once.
144180
@@ -383,14 +419,12 @@ def test_to_dataset_multi_registered_requires_explicit_template(
383419
assert set(out.dims) == {"time", "lat", "lon"}
384420

385421

386-
def test_to_dataset_infer_fails_when_no_template_fits(air_dataset_small):
387-
"""If no registered Dataset's dims fit the result -> clear error."""
422+
def test_to_dataset_infer_fails_when_no_dim_survives(air_dataset_small):
423+
"""A global aggregation leaves no registered dim in the result -> clear error."""
388424
ctx = XarrayContext()
389425
ctx.from_dataset("air", air_dataset_small)
390426
with pytest.raises(ValueError, match="dims cannot be inferred"):
391-
ctx.sql(
392-
"SELECT lat, lon, AVG(air) AS air_avg FROM air GROUP BY lat, lon"
393-
).to_dataset()
427+
ctx.sql("SELECT AVG(air) AS air_avg FROM air").to_dataset()
394428

395429

396430
def test_template_accepts_name_or_dataset(air_dataset_small):

xarray_sql/ds.py

Lines changed: 26 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -757,10 +757,12 @@ def to_dataset(
757757
758758
Args:
759759
dims: Result columns to use as Dataset dimensions. When
760-
``None``, defaults to the dims of the registered Dataset
761-
referenced by the SQL ``FROM`` clause (if exactly one
762-
matches), or any single registered Dataset whose dims are
763-
all present in the result columns.
760+
``None``, defaults to a registered Dataset's dimensions that
761+
survive into the result columns, so an aggregation that drops
762+
dims (e.g. ``GROUP BY time`` over a ``(time, lat, lon)`` grid)
763+
round-trips on the remaining dim. Raises when no dimension
764+
survives, or when several registered Datasets imply different
765+
dims (pass ``dims`` explicitly then).
764766
template: Source to recover metadata (attrs, encoding, non-dim
765767
coordinates, dim-coord dtype) from. Either an ``xr.Dataset``
766768
used directly, or the name of a registered table (e.g.
@@ -879,33 +881,37 @@ def _infer_dimension_columns(
879881
) -> list[str]:
880882
"""Pick a default ``dimension_columns`` from the registry, or raise.
881883
882-
Uses the data variable's dim order (via :func:`_ds_var_dims`) so
883-
the round-trip preserves the original axis order.
884+
A registered Dataset's dims that survive into the result columns
885+
become the dimensions, so aggregations that drop dims (e.g.
886+
``GROUP BY time`` over a ``(time, lat, lon)`` grid) round-trip on the
887+
surviving dim(s). Uses the data variable's dim order (via
888+
:func:`_ds_var_dims`) so the original axis order is preserved.
884889
"""
885890
result_cols = set(self._result_columns())
886-
if (
887-
preferred_template is not None
888-
and set(preferred_template.dims) <= result_cols
889-
):
890-
return _ds_var_dims(preferred_template)
891+
892+
def surviving(template: xr.Dataset) -> list[str]:
893+
# Template dims still present in the result, in var axis order.
894+
return [d for d in _ds_var_dims(template) if d in result_cols]
895+
896+
if preferred_template is not None:
897+
preferred = surviving(preferred_template)
898+
if preferred:
899+
return preferred
891900
if not self._templates:
892901
raise ValueError(
893902
"dims cannot be inferred (no registered "
894903
"Dataset on this result); pass dims=[...] "
895904
"explicitly."
896905
)
897-
candidates = [
898-
_ds_var_dims(t)
899-
for t in self._templates.values()
900-
if set(t.dims) <= result_cols
901-
]
906+
candidates = {tuple(surviving(t)) for t in self._templates.values()}
907+
candidates.discard(()) # templates with no surviving dim
902908
if len(candidates) == 1:
903-
return candidates[0]
909+
return list(next(iter(candidates)))
904910
if not candidates:
905911
raise ValueError(
906-
"dims cannot be inferred: no registered "
907-
"Dataset has all of its dims present in the result "
908-
"columns. Pass dims=[...] explicitly."
912+
"dims cannot be inferred: no registered Dataset "
913+
"dimension survives in the result columns. Pass "
914+
"dims=[...] explicitly."
909915
)
910916
raise ValueError(
911917
"dims cannot be inferred unambiguously: multiple "

0 commit comments

Comments
 (0)