Skip to content

Commit f9919b6

Browse files
Miguel Moncadaclaude
authored andcommitted
fix: four adversarial-review findings
- NaN/NaT in a chunk's dimension coordinate poisoned the shadow guarantee into (dim >= NaN), which Arrow simplifies every predicate against as false — chunks with matching rows were silently pruned. Such spans now carry an always-true guarantee (unprunable). - _affine_axis accepted NaN-poisoned axes (NaN > tol is false), letting the affine scatter place values in wrong cells when a result's dim column contains NULLs. The acceptance test is now a <= .all() so NaN rejects and the positional searchsorted path handles it. - Projected scans omit dimension columns from the scan schema; the cftime coordinate-conversion loop assumed every dim had a schema field and raised KeyError on e.g. SELECT SUM(v) over a 360_day dataset. Dims absent from the schema are now skipped. - pyramid() rebinned float cell origins level-over-level, occasionally aliasing boundary points into a neighboring cell. Cells are now tracked as integer indices that halve exactly per level, with float origins kept as query labels. Also: docs notes from stress testing (Polars is_in float-literal pushdown caveat, per-thread DuckDB cursor re-registration pattern). Regression tests for all four fixes; battle-test matrix passed on duckdb 1.4.5 LTS and 1.5.4, 4-thread concurrency, dtype zoo, and a 600-query memory soak. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent abfaa95 commit f9919b6

8 files changed

Lines changed: 148 additions & 22 deletions

File tree

‎docs/engines.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,12 @@ Polars pushes its predicate and column selection into the dataset scan
131131
(verified: a filtered group-by read 1 of 20 chunks and 3 of 5 columns),
132132
and its results round-trip through `xql.to_dataset` unchanged.
133133

134+
One upstream caveat: Polars' translation of `is_in` **float** literals
135+
into pyarrow expressions loses precision and can drop matching rows
136+
(reproducible without xarray-sql). Prefer range predicates
137+
(`is_between`) for float coordinates; integer and timestamp value sets
138+
are unaffected.
139+
134140
### Relation to duckdb-zarr
135141

136142
[duckdb-zarr](https://github.com/xqlsystems/duckdb-zarr) reads Zarr

‎docs/performance.md‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,24 @@ only the variables a query references are read. Corollaries:
8989
- A query with no `WHERE` on dimension columns is a full scan on any
9090
engine; that's physics, not a missing optimization.
9191

92+
## Threads and DuckDB connections
93+
94+
Registered Python objects are connection-local in DuckDB: `con.cursor()`
95+
does not inherit them, and one connection's result slot is not
96+
thread-safe. For multithreaded querying, give each thread its own
97+
cursor and register the *same* dataset object on it:
98+
99+
```python
100+
dataset = xql.arrow_dataset(ds)
101+
def worker():
102+
cur = con.cursor()
103+
cur.register("t", dataset) # cheap; shares the pruning index
104+
...
105+
```
106+
107+
The dataset object itself is safe to share across threads (verified
108+
under concurrent query load).
109+
92110
## Stop re-scanning: materialize and pyramid
93111

94112
Repeated statistics should pay the scan once:

‎tests/test_duckdb_backend.py‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -344,3 +344,44 @@ def test_register_dispatches_to_datafusion():
344344
def test_register_rejects_unknown_connection(ds):
345345
with pytest.raises(TypeError, match="No xarray-sql engine adapter"):
346346
xql.register(object(), "weather", ds)
347+
348+
349+
def test_nan_coordinate_chunk_is_not_pruned():
350+
# NaN in a chunk's coordinate must disable pruning for that span,
351+
# never poison the range guarantee (which would silently drop rows).
352+
ds = xr.Dataset(
353+
{"v": (["lat"], np.arange(6.0))},
354+
coords={"lat": [np.nan, 5.0, 10.0, 20.0, 30.0, 40.0]},
355+
).chunk({"lat": 2})
356+
con = duckdb.connect()
357+
xql.register(con, "t", ds)
358+
assert con.sql("SELECT v FROM t WHERE lat = 5.0").fetchall() == [(1.0,)]
359+
360+
361+
def test_cftime_dataset_aggregates_under_projection():
362+
cftime = pytest.importorskip("cftime")
363+
364+
times = xr.date_range(
365+
"2000-01-01", periods=6, calendar="360_day", use_cftime=True
366+
)
367+
ds = xr.Dataset(
368+
{"v": (["time"], np.arange(6.0))}, coords={"time": times}
369+
).chunk({"time": 3})
370+
con = duckdb.connect()
371+
xql.register(con, "t", ds)
372+
# The scan projects only `v`; the cftime dim column is absent from
373+
# the scan schema but still shapes the iteration.
374+
assert con.sql("SELECT SUM(v) FROM t").fetchone()[0] == 15.0
375+
376+
377+
def test_null_dimension_value_round_trips_positionally():
378+
# A NULL in a result's dim column must reject the affine fast path
379+
# and fall back to positional scatter.
380+
table = pa.table(
381+
{
382+
"lat": pa.array([0.0, None, 1.0, 3.0], type=pa.float64()),
383+
"v": [10.0, 99.0, 11.0, 13.0],
384+
}
385+
)
386+
out = xql.to_dataset(table, dims=["lat"])
387+
np.testing.assert_allclose(out["v"].values, [10.0, 99.0, 11.0, 13.0])

‎tests/test_materialize.py‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,3 +140,23 @@ def test_pyramid_filter_prunes_source_scan(con):
140140
n = _rows(con, "SELECT SUM(n) FROM pyr")[0][0]
141141
direct = _rows(con, "SELECT COUNT(*) FROM grid WHERE y > -32")[0][0]
142142
assert n == direct
143+
144+
145+
def test_pyramid_cell_membership_is_exact_across_levels():
146+
# Rebinning float origins level-over-level can alias points across
147+
# cell boundaries; integer indices must halve exactly instead.
148+
import math
149+
150+
con = duckdb.connect()
151+
con.execute("CREATE TABLE pts AS SELECT -130.943 AS x, 0.0005 AS y")
152+
xql.pyramid(
153+
con,
154+
"pyr",
155+
"pts",
156+
aggs={"n": ("count", "*")},
157+
base_cell=0.001,
158+
levels=2,
159+
)
160+
rows = con.sql("SELECT level, x_idx FROM pyr ORDER BY level").fetchall()
161+
assert rows[0][1] == math.floor(-130.943 / 0.001)
162+
assert rows[1][1] == math.floor(-130.943 / 0.002)

‎xarray_sql/backends/pyarrow.py‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -122,13 +122,22 @@ def _build(self, spans: list[tuple[int, int]]) -> pads.FileSystemDataset:
122122
fragments = []
123123
for i, (lo_chunk, hi_chunk) in enumerate(spans):
124124
vals = self._coord[self._bounds[lo_chunk] : self._bounds[hi_chunk]]
125-
# min/max (not first/last) so descending axes like latitude
126-
# 90→-90 carry correct ranges.
127-
lo = pa.scalar(vals.min(), type=self._field_type)
128-
hi = pa.scalar(vals.max(), type=self._field_type)
129-
guarantee = (pc.field(self._name) >= lo) & (
130-
pc.field(self._name) <= hi
131-
)
125+
if (vals.dtype.kind == "f" and np.isnan(vals).any()) or (
126+
vals.dtype.kind == "M" and np.isnat(vals).any()
127+
):
128+
# NaN/NaT poisons min/max into a (dim >= NaN) guarantee
129+
# that Arrow simplifies every predicate against as false,
130+
# silently pruning rows. An always-true guarantee keeps
131+
# the span unprunable instead.
132+
guarantee = pc.scalar(True)
133+
else:
134+
# min/max (not first/last) so descending axes like
135+
# latitude 90→-90 carry correct ranges.
136+
lo = pa.scalar(vals.min(), type=self._field_type)
137+
hi = pa.scalar(vals.max(), type=self._field_type)
138+
guarantee = (pc.field(self._name) >= lo) & (
139+
pc.field(self._name) <= hi
140+
)
132141
fragments.append(
133142
fmt.make_fragment(str(i), fs, partition_expression=guarantee)
134143
)

‎xarray_sql/df.py‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -349,8 +349,12 @@ def iter_record_batches(
349349

350350
# Preload small 1-D coordinate arrays (negligible memory).
351351
# Convert cftime objects to numeric values matching the schema type.
352+
# Projected scans may omit dimension columns from the schema; those
353+
# dims still shape the iteration but never emit a column.
352354
coord_values = {}
353355
for name in dim_names:
356+
if name not in schema.names:
357+
continue
354358
vals = ds.coords[name].values
355359
if cft.is_cftime(vals):
356360
coord_values[name] = cft.convert_for_field(vals, schema.field(name))

‎xarray_sql/ds.py‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -169,7 +169,10 @@ def _affine_axis(requested: np.ndarray) -> tuple[float, float] | None:
169169
if step == 0 or not np.isfinite(step):
170170
return None
171171
predicted = numeric[0] + step * np.arange(len(numeric))
172-
if np.abs(numeric - predicted).max() > 0.25 * abs(step):
172+
# Written as a <= comparison so a NaN anywhere in the axis (e.g. a
173+
# NULL dim value in the result) fails the check and falls back to
174+
# the searchsorted path, which handles it positionally.
175+
if not (np.abs(numeric - predicted) <= 0.25 * abs(step)).all():
173176
return None
174177
return float(numeric[0]), float(step)
175178

‎xarray_sql/materialize.py‎

Lines changed: 39 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -110,11 +110,14 @@ def pyramid(
110110
the total cost beyond the single scan is negligible. The result is
111111
one long table::
112112
113-
(level INTEGER, x_bin DOUBLE, y_bin DOUBLE, <agg columns...>)
113+
(level, x_idx BIGINT, y_idx BIGINT, x_bin, y_bin, <aggs...>)
114114
115-
where ``x_bin``/``y_bin`` are the cell origins at that level's cell
116-
size (``base_cell * 2**level``). Query the level whose cells match
117-
the resolution you need::
115+
``x_idx``/``y_idx`` are exact integer cell indices at that level
116+
(they halve from level to level, so cell membership never drifts
117+
across levels); ``x_bin``/``y_bin`` are the float cell origins
118+
(``idx * base_cell * 2**level``) for human-readable querying —
119+
filter them with ranges rather than equality. Query the level whose
120+
cells match the resolution you need::
118121
119122
SELECT x_bin, y_bin, class4_n / n AS share
120123
FROM grid_pyramid
@@ -160,19 +163,32 @@ def pyramid(
160163

161164
run_sql = get_adapter(con).run_sql
162165

166+
# Cells are tracked as integer indices (x_idx, y_idx) and only
167+
# labeled with float origins (x_bin, y_bin) for querying: rebinning
168+
# float origins at each level occasionally lands boundary points in
169+
# a different cell than direct binning would (float aliasing);
170+
# integer indices halve exactly at every level.
163171
# Level 0: the single scan of the source.
164172
run_sql(
165173
con,
166174
f"""
167175
CREATE OR REPLACE TABLE {_ident(name)} AS
168176
SELECT
169177
0 AS level,
170-
FLOOR({_ident(x)} / {base_cell}) * {base_cell} AS x_bin,
171-
FLOOR({_ident(y)} / {base_cell}) * {base_cell} AS y_bin,
178+
x_idx,
179+
y_idx,
180+
x_idx * {base_cell} AS x_bin,
181+
y_idx * {base_cell} AS y_bin,
172182
{base_aggs}
173-
FROM {_ident(table)}
174-
{where}
175-
GROUP BY 2, 3
183+
FROM (
184+
SELECT
185+
CAST(FLOOR({_ident(x)} / {base_cell}) AS BIGINT) AS x_idx,
186+
CAST(FLOOR({_ident(y)} / {base_cell}) AS BIGINT) AS y_idx,
187+
*
188+
FROM {_ident(table)}
189+
{where}
190+
)
191+
GROUP BY x_idx, y_idx
176192
""",
177193
)
178194

@@ -183,18 +199,27 @@ def pyramid(
183199
f"{_ROLLUP[kind]}({_ident(n)}) AS {_ident(n)}"
184200
for n, (kind, _) in aggs.items()
185201
)
202+
pass_cols = ", ".join(_ident(n) for n in aggs)
186203
run_sql(
187204
con,
188205
f"""
189206
INSERT INTO {_ident(name)}
190207
SELECT
191208
{level} AS level,
192-
FLOOR(x_bin / {cell}) * {cell} AS x_bin,
193-
FLOOR(y_bin / {cell}) * {cell} AS y_bin,
209+
x_idx,
210+
y_idx,
211+
x_idx * {cell} AS x_bin,
212+
y_idx * {cell} AS y_bin,
194213
{rollup_aggs}
195-
FROM {_ident(name)}
196-
WHERE level = {level - 1}
197-
GROUP BY 2, 3
214+
FROM (
215+
SELECT
216+
CAST(FLOOR(x_idx / 2.0) AS BIGINT) AS x_idx,
217+
CAST(FLOOR(y_idx / 2.0) AS BIGINT) AS y_idx,
218+
{pass_cols}
219+
FROM {_ident(name)}
220+
WHERE level = {level - 1}
221+
)
222+
GROUP BY x_idx, y_idx
198223
""",
199224
)
200225
return con

0 commit comments

Comments
 (0)