Skip to content

fix: register object-dtype and out-of-ns-range coordinates (#223)#224

Open
ghostiee-11 wants to merge 2 commits into
xqlsystems:mainfrom
ghostiee-11:fix/223-object-dtype-out-of-ns-datetimes
Open

fix: register object-dtype and out-of-ns-range coordinates (#223)#224
ghostiee-11 wants to merge 2 commits into
xqlsystems:mainfrom
ghostiee-11:fix/223-object-dtype-out-of-ns-datetimes

Conversation

@ghostiee-11

Copy link
Copy Markdown
Contributor

Overview

from_dataset could not register several common NetCDF/xarray datasets: non-cftime object-dtype variables and coordinates (strings) raised ArrowNotImplementedError, and datetime coordinates outside the datetime64[ns] range raised OverflowError. This fixes both in _parse_schema and _block_metadata, and revives cftime partition pruning that an object-dtype skip had silently disabled. Fixes #223.

Before

ds = xr.Dataset({"label": (["x"], np.array(["a", "b"], dtype=object))}, coords={"x": [1, 2]})
XarrayContext().from_dataset("t", ds, chunks={"x": 2})
# pyarrow.lib.ArrowNotImplementedError: Unsupported numpy type 17

times = xr.date_range("0001-01-01", periods=3, freq="100YS", use_cftime=True).to_datetimeindex(time_unit="us")
ds = xr.Dataset({"v": (["time"], np.arange(3.0))}, coords={"time": times})
XarrayContext().from_dataset("t", ds, chunks={"time": 2})
# OverflowError: Cannot convert Timestamp to nanoseconds without overflow

After

# both register and query cleanly
c = XarrayContext(); c.from_dataset("t", ds, chunks={"x": 2})
c.sql("SELECT label FROM t ORDER BY x").to_pandas()["label"].tolist()
# ['a', 'b']

# string coordinates, cftime calendars, and pre-1678 / post-2262 dates all register;
# out-of-int64 datetimes skip pruning instead of raising, so registration always succeeds.

…s#223)

_parse_schema handed object dtype straight to pa.from_numpy_dtype, which
raises "Unsupported numpy type 17" for string variables and coordinates.
Infer the Arrow type from a bounded sample instead (strings map to
pa.string(); an empty or all-null object column defaults to pa.string()).

_block_metadata forced datetime bounds to int64 nanoseconds, overflowing
for dates outside the datetime64[ns] range (before 1678 or after 2262).
Skip pruning for such dimensions rather than raising, so registration
still succeeds.

Also revive cftime partition pruning: the string/object skip shadowed the
cftime branch, silently disabling it. Check cftime first, and have
partition_bounds return None when the nanosecond bound overflows int64 so
that ancient dates are skipped rather than rejected by the Rust layer.

Adds regression tests for each case plus an end-to-end pruning check.

@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.

Some questions and feedback for review.

Comment thread tests/test_df.py Outdated
assert sum(tup) == ds.sizes[dim]


# -- Object-dtype and out-of-ns-range coordinate support (issue #223) -------

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.

Let's omit this GH issue.

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.

Did you self review this PR?

Comment thread tests/test_df.py
# A string variable arrives as numpy object dtype; _parse_schema must not
# hand it to pa.from_numpy_dtype (which raises "Unsupported numpy type 17").
ds = xr.Dataset(
{"label": (["x"], np.array(["a", "b"], dtype=object))},

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.

Are there types of objects that don't map to strings? Do we throw errors for these cases, or just try to convert them to pa.string()?

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.

checked the pa + xr behaviour. i don't force pa.string() anymore, pa.array infers per content: strings->string, bytes->binary, ints->int64, datetime->timestamp[us]. all-null stays null. a column mixing incompatible types (e.g. str+int) raises ArrowTypeError, so we surface a clear error instead of silently coercing.

Comment thread tests/test_df.py
assert _field_type(schema, "station") == pa.string()


def test_partition_metadata_skips_out_of_ns_datetime():

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.

What are the consequences of skipping this for cftime typed time ranges? What are the tradeoffs?

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.

skipping only drops partition pruning (a query-time optimization) for that dim, correctness is unaffected. the rust pruner treats a missing dim as "never prune", so filtered queries just scan all partitions. only bites out-of-ns-range dates (pre-1678 / post-2262), which are rare.

Comment thread tests/test_df.py Outdated
assert all("time" not in m for m in meta)


def test_parse_schema_all_null_object_var_defaults_to_string():

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.

This seems like strictly incorrect behavior. Let's let null be null.

Comment thread tests/test_df.py
assert tag == "timestamp_ns"


def test_partition_metadata_skips_ancient_cftime():

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.

Does pa have some sort of bigint type that we could use to handle these cftime cases instead of skipping pruning?

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.

I can't tell yet what the better approach is (it's probably the implemented one).

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.

had a look. the rust ScalarBound only takes timestamp_ns / int64 / float64, and bound_to_scalar only maps a TimestampNanos bound onto a Timestamp column. an int64 bound wouldn't apply to a timestamp[us] column so it can't prune it. representing the column as int64 micros would prune but breaks string-timestamp SQL filters. so skipping pruning for those rare out-of-range dates is the safe call, went with the implemented approach.

Comment thread tests/test_df.py
assert streamed.column("label").to_pylist() == ["a", "b", "c", "d"]


def test_partition_metadata_in_range_datetime_still_pruned():

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.

Happy to have this test.

Comment thread xarray_sql/cftime.py Outdated
Comment on lines +175 to +176
_INT64_MIN: int = -(2**63)
_INT64_MAX: int = 2**63 - 1

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.

Can we get this from a more authoritative place, like a numpy or pa type? Or a python std library? This is the right calculation I'm sure, but I'd rather use something more maintainable.

Comment thread xarray_sql/df.py Outdated
Comment on lines +401 to +405
Only the first ``sample_size`` elements are inspected so a huge in-memory
object column is not fully copied just to read its type. An empty or
all-null sample yields ``pa.null()``, which is meaningless as a column
type; object dtype in xarray almost always means strings, so fall back to
``pa.string()`` in that case.

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.

Let's be correct, not polite

Comment thread xarray_sql/df.py Outdated
Comment on lines +407 to +411
sample = np.asarray(values).ravel()[:sample_size]
arrow_type = pa.array(sample).type
if pa.types.is_null(arrow_type):
return pa.string()
return arrow_type

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.

I recommend reviewing the xr and pa sources to better understand what object types could mean.

Comment thread xarray_sql/df.py Outdated
columns.append(cft.arrow_field(var_name, cft.DEFAULT_UNITS, cal))
if var.dtype == np.dtype("O"):
if cft.is_cftime(var.values):
# Rare: a data variable holding cftime objects. Use same

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.

I don't think this is that rare.

Infer object-dtype columns directly with pyarrow instead of sampling and
coercing: strings map to string, other representable scalars to their Arrow
type, an all-null column stays null, and a column mixing incompatible types
raises rather than being silently coerced to string.

Derive the int64 pruning-bound limits from np.iinfo(np.int64) rather than
hardcoded literals, and drop the inaccurate "rare" comments on the cftime
data-variable path.
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.

from_dataset fails on object-dtype variables (cftime times, strings) and out-of-ns-range datetimes

2 participants