Skip to content

Skip string coordinates in partition bound computation#153

Merged
alxmrs merged 2 commits into
xqlsystems:mainfrom
ghostiee-11:fix/string-coord-partition-bounds
Mar 22, 2026
Merged

Skip string coordinates in partition bound computation#153
alxmrs merged 2 commits into
xqlsystems:mainfrom
ghostiee-11:fix/string-coord-partition-bounds

Conversation

@ghostiee-11

Copy link
Copy Markdown
Contributor

Description

String-typed coordinates (e.g. station IDs, model names) crash during dataset registration because _block_metadata() calls np.min()/np.max() on the coordinate array, and numpy's minimum ufunc does not support string dtypes.

import numpy as np
import xarray as xr
from xarray_sql import XarrayContext

# Weather station observations with string station IDs
ds = xr.Dataset(
    {"temperature": (["station", "time"], np.random.rand(4, 10))},
    coords={
        "station": ["KJFK", "KLAX", "KORD", "KATL"],
        "time": np.arange(10),
    },
)
ctx = XarrayContext()
ctx.from_dataset("obs", ds.chunk({"station": 2, "time": 5}))
# ufunc 'minimum' did not contain a loop with signature matching types
# (dtype('<U4'), dtype('<U4')) -> None

Fix

Skip coordinates with string/object dtypes (kind in ('U', 'S', 'O')) before computing min/max bounds. The Rust ScalarBound enum only supports Int64/Float64/TimestampNanos, so string bounds would fail downstream regardless. Skipping makes the pruning logic treat the dimension conservatively (never prunes on it).

Also flattened the indentation by converting the if len > 0 block to an early continue.

Tests

Added test_string_coordinates. All 11 tests pass.

Partial fix for #121.

@alxmrs alxmrs left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'm sorry, I introduced a merge conflict here by merging #155. But, that should be a quick fix, then I think this would be good to merge into main.

I really you filing the bug and making the fix! Welcome to the project.

Comment thread xarray_sql/df.py
Comment on lines +326 to +327
if len(coord_values) == 0:
continue

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is a good idea.

Comment thread tests/test_sql.py
@@ -192,3 +193,21 @@ def test_cross_join(air_and_stations):
"SELECT COUNT(*) AS total FROM air_data CROSS JOIN stations"
).to_pandas()
assert result["total"].iloc[0] > 0


def test_string_coordinates():

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good to have this test, and it's clear to read.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!!

String-typed coordinates (dtype U, S, O) crash in _block_metadata()
because numpy min/max ufuncs do not support string comparison.
Since ScalarBound only supports Int64/Float64/TimestampNanos, skip
string coordinates so the Rust pruning logic treats the dimension
conservatively (never prunes on it).

Partial fix for xqlsystems#121.
The project uses pyink-use-majority-quotes = true, which enforces
double quotes. Single quotes in the string dtype check caused the
lint CI to fail.
@ghostiee-11
ghostiee-11 force-pushed the fix/string-coord-partition-bounds branch from f5c5414 to e57a2b4 Compare March 22, 2026 04:50
@ghostiee-11

Copy link
Copy Markdown
Contributor Author

Fixed the conflicts..

@alxmrs
alxmrs merged commit fbb5c60 into xqlsystems:main Mar 22, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants