-
Notifications
You must be signed in to change notification settings - Fork 16
fix: register object-dtype and out-of-ns-range coordinates (#223) #224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
|
|
||
| from xarray_sql.df import ( | ||
| DEFAULT_BATCH_SIZE, | ||
| _ensure_default_indexes, | ||
| _parse_schema, | ||
| block_slices, | ||
| compute_chunks, | ||
|
|
@@ -16,6 +17,7 @@ | |
| from_map, | ||
| from_map_batched, | ||
| iter_record_batches, | ||
| partition_metadata, | ||
| pivot, | ||
| ) | ||
| from xarray_sql.reader import read_xarray, read_xarray_table | ||
|
|
@@ -532,3 +534,138 @@ def test_compute_chunks_tuples_sum_to_dim_size(): | |
| result = compute_chunks(ds, {"a": 3, "b": 4, "c": 5}) | ||
| for dim, tup in result.items(): | ||
| assert sum(tup) == ds.sizes[dim] | ||
|
|
||
|
|
||
| # -- Object-dtype and out-of-ns-range coordinate support -------------------- | ||
|
|
||
|
|
||
| def _field_type(schema, name): | ||
| return schema.field(name).type | ||
|
|
||
|
|
||
| def test_parse_schema_maps_object_string_data_var_to_string(): | ||
| # 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))}, | ||
| coords={"x": [1, 2]}, | ||
| ) | ||
| schema = _parse_schema(_ensure_default_indexes(ds)) | ||
| assert _field_type(schema, "label") == pa.string() | ||
|
|
||
|
|
||
| def test_parse_schema_maps_object_string_coord_to_string(): | ||
| # A string dimension coordinate (e.g. station names) is object dtype too. | ||
| ds = xr.Dataset( | ||
| {"v": (["station"], [1.0, 2.0])}, | ||
| coords={"station": np.array(["A", "B"], dtype=object)}, | ||
| ) | ||
| schema = _parse_schema(_ensure_default_indexes(ds)) | ||
| assert _field_type(schema, "station") == pa.string() | ||
|
|
||
|
|
||
| def test_partition_metadata_skips_out_of_ns_datetime(): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| # datetime64 coordinates outside the datetime64[ns] range (pre-1678 / | ||
| # post-2262) cannot be represented as int64 nanoseconds, so partition | ||
| # pruning must be skipped for that dimension rather than raising | ||
| # OverflowError. Registration must still succeed. | ||
| times = xr.date_range( | ||
| "0001-01-01", periods=3, freq="100YS", use_cftime=True | ||
| ).to_datetimeindex(time_unit="us", unsafe=True) | ||
| ds = _ensure_default_indexes( | ||
| xr.Dataset({"v": (["time"], np.arange(3.0))}, coords={"time": times}) | ||
| ) | ||
| blocks = list(block_slices(ds, chunks={"time": 2})) | ||
|
|
||
| meta = partition_metadata(ds, blocks) # must not raise | ||
|
|
||
| assert len(meta) == len(blocks) | ||
| # "time" is unpruneable here, so it is omitted from every partition. | ||
| assert all("time" not in m for m in meta) | ||
|
|
||
|
|
||
| def test_parse_schema_all_null_object_var_stays_null(): | ||
| # An all-null object column has no data to infer a type from; let null be | ||
| # null rather than coercing it to a string column. | ||
| ds = _ensure_default_indexes( | ||
| xr.Dataset( | ||
| {"label": (["x"], np.array([None, None], dtype=object))}, | ||
| coords={"x": [1, 2]}, | ||
| ) | ||
| ) | ||
| schema = _parse_schema(ds) | ||
| assert pa.types.is_null(schema.field("label").type) | ||
|
|
||
|
|
||
| def test_partition_metadata_prunes_cftime_coord(): | ||
| # cftime dimension coordinates must produce pruning bounds; previously the | ||
| # object-dtype skip shadowed the cftime branch, silently disabling pruning. | ||
| times = xr.date_range( | ||
| "2000-01-01", periods=4, freq="1D", calendar="noleap", use_cftime=True | ||
| ) | ||
| ds = _ensure_default_indexes( | ||
| xr.Dataset({"v": (["time"], np.arange(4.0))}, coords={"time": times}) | ||
| ) | ||
| blocks = list(block_slices(ds, chunks={"time": 2})) | ||
|
|
||
| meta = partition_metadata(ds, blocks) | ||
|
|
||
| assert all("time" in m for m in meta) | ||
| for m in meta: | ||
| _, _, tag = m["time"] | ||
| assert tag == "timestamp_ns" | ||
|
|
||
|
|
||
| def test_partition_metadata_skips_ancient_cftime(): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| # Ancient gregorian cftime dates overflow the int64 nanosecond range, so | ||
| # pruning must be skipped for that dim (no raise, dim omitted). | ||
| times = xr.date_range( | ||
| "0001-01-01", periods=3, freq="100YS", use_cftime=True | ||
| ) | ||
| ds = _ensure_default_indexes( | ||
| xr.Dataset({"v": (["time"], np.arange(3.0))}, coords={"time": times}) | ||
| ) | ||
| blocks = list(block_slices(ds, chunks={"time": 2})) | ||
|
|
||
| meta = partition_metadata(ds, blocks) # must not raise | ||
|
|
||
| assert all("time" not in m for m in meta) | ||
|
|
||
|
|
||
| def test_string_dataset_round_trips_through_record_batch(): | ||
| # The schema fix must also flow through the batch builders: a string | ||
| # column has to materialize as an Arrow string array, not error out. | ||
| ds = _ensure_default_indexes( | ||
| xr.Dataset( | ||
| {"label": (["x"], np.array(["a", "b", "c", "d"], dtype=object))}, | ||
| coords={"x": [10, 20, 30, 40]}, | ||
| ) | ||
| ) | ||
| schema = _parse_schema(ds) | ||
|
|
||
| batch = dataset_to_record_batch(ds, schema) | ||
| assert batch.schema.field("label").type == pa.string() | ||
| assert batch.column("label").to_pylist() == ["a", "b", "c", "d"] | ||
|
|
||
| # The streaming path must agree with the one-shot path. | ||
| streamed = pa.Table.from_batches( | ||
| list(iter_record_batches(ds, schema, batch_size=2)), schema=schema | ||
| ) | ||
| assert streamed.column("label").to_pylist() == ["a", "b", "c", "d"] | ||
|
|
||
|
|
||
| def test_partition_metadata_in_range_datetime_still_pruned(): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Happy to have this test. |
||
| # Regression guard: ordinary datetimes must keep producing timestamp_ns | ||
| # bounds so filter pushdown still works after the overflow fix. | ||
| times = pd.date_range("2000-01-01", periods=4, freq="D") | ||
| ds = _ensure_default_indexes( | ||
| xr.Dataset({"v": (["time"], np.arange(4.0))}, coords={"time": times}) | ||
| ) | ||
| blocks = list(block_slices(ds, chunks={"time": 2})) | ||
|
|
||
| meta = partition_metadata(ds, blocks) | ||
|
|
||
| assert all("time" in m for m in meta) | ||
| for m in meta: | ||
| _, _, tag = m["time"] | ||
| assert tag == "timestamp_ns" | ||
There was a problem hiding this comment.
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()?
There was a problem hiding this comment.
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.