Repository navigation
Conversation
1934550 to
aa58cd8
Compare
|
IIUC, in CI only polars 2.0 will be tested? Right after this PR, polars 1.x is tested to still work, but I am worried about this rotting, and us accidentally breaking polars 1.x compat and not catching this because we aren't testing in CI. Do you think that is worth testing? It would be great to unify this with pyarrow, eg we have a pyarrow:true and pyarrow:false CI tests, but it would also be great if we could test pyarrow:oldest-stated-compat and pyarrow:newest. Similar for pandas, etc etc it would be good to have "dep profiles" that are easy to switch between (in CI, but also ideally also on a dev machine). I think I am fine with merging this as is and doing that later, but curious if you have opinions. |
| if bound.dtype == arg_dtype: | ||
| return value | ||
| # polars doesn't support casting strings to temporal types | ||
| if bound.dtype.is_string() and arg_dtype.is_temporal(): |
There was a problem hiding this comment.
I don't love how we use polar's casting logic here, which is prone to drifting from the string->time casting we added in this pr in the def _cast(). Like why does that get a format string, and this doesn't? I would prefer if we had a singular def _cast_string_to_temporal(polars_str_expr, ibis_dest_type) and both callsites used it. But I haven't tried this, maybe this is more complex than it is worth.
There was a problem hiding this comment.
@NickCrews I gave it a shot in ac073e6, and after some guidance to keep things as aligned with the existing logic/behavior as possible, I think it pretty well matches what you suggsted.
NickCrews
left a comment
There was a problem hiding this comment.
Two comments, both optional in my opinion. At least I would love to have claude give a spin on my str->temporal casting unification, and see if that isn't too complex. But I would be happy to see this merged in its current state as well.
Agree there's some risk in it rotting, although I don't know how concerned I am right now for a few reasons:
That said, happy to add the matrix--like you said, perhaps in a followup. |
I and/or can take a look at this tomorrow; doesn't hurt to give it a spin, like you say, and report back! |
## Description of changes `google.api_core` emits a `FutureWarning` at import time once Google's support window for the running Python version has ended, which is now the case for Python 3.10. Our `filterwarnings = ["error", ...]` turns that into a collection error, failing every Python 3.10 job on every branch: > FutureWarning: You are using a non-supported Python version (3.10.22). Google will not post any further updates to google.api_core supporting this Python version. Please upgrade to the latest Python version, or at least Python 3.11, and then update google.api_core. Ignore it alongside the other google-api-core warning filters. With this change, all 29 Python 3.10 jobs passed on #12147, where it was first carried. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
## Description of changes Replace Polars APIs that were removed or behave differently in Polars 2.0 with spellings that work identically on 1.x, so no version branching is needed in source. - `ArrayFlatten`: `Expr.flatten()` was removed. Filter out null and empty inner arrays, then `explode()`. This sidesteps `explode`'s version-dependent defaults and also fixes empty inner arrays being flattened to a null. - `Between`: casting strings to temporal types was removed. Use `str.strptime` for string bounds. - `DayOfWeekName`: the 7-branch `when/then` chain returns zero rows on Polars >=1.44.0 due to an upstream bug, fixed in pola-rs/polars#29754. Map `dt.weekday()` to `calendar.day_name` with `replace_strict` instead, which is also ~4x faster. - `test_unnest_empty_array`: Polars 2.0 drops empty lists on `explode`, matching other backends; xfail only for `polars<2`. Assisted-by: Claude Opus 5.5 <noreply@anthropic.com>
## Description of changes Parse strings with `str.strptime(pl.Time, "%H:%M:%S%.f")` when casting to time. Polars 2.0 removed string-to-time casts, and on Polars 1.x the plain cast raised on valid input while `try_cast` returned all nulls. The explicit format keeps `try_cast` from raising when no value parses on Polars 1.x. Add `string_to_time` cases to `test_cast`, `test_try_cast`, and `test_try_cast_null`. Assisted-by: Claude Opus 5.5 <noreply@anthropic.com>
Parse strings into dates, timestamps, and times in one helper shared by `Cast`/`TryCast` and `Between`, so string bounds in `between` are parsed exactly like an explicit cast instead of drifting from it. Timestamps are now parsed at the precision `PolarsType` maps the target type to. This fixes casting strings to `timestamp(0)`, which raised because Polars has no second-precision datetime type; such values are now parsed at nanosecond precision and truncated to seconds. Add tests for casting a string to `timestamp(0)` and for `between` with string timestamp bounds at several precisions. Assisted-by: Claude Opus 5.5 <noreply@anthropic.com>
Assisted-by: Claude Opus 5.5 <noreply@anthropic.com>
NickCrews
left a comment
There was a problem hiding this comment.
I have one more idea I can send in the morning when I'm at my computer but here are a few
| assert isinstance(result, pl.LazyFrame) | ||
|
|
||
|
|
||
| def test_cast_string_to_timestamp_seconds(con): |
There was a problem hiding this comment.
Both of these new tests should be in generic backend tests, not just for polars?
| @@ -146,32 +146,40 @@ def _cast(op, strict=True, **kw): | |||
| elif to.is_date(): | |||
| if not strict: | |||
There was a problem hiding this comment.
Is this being overly restrictive? It looks like below the polars strptime API accepts a strict:bool arg, which makes me think we could support string try casting to dates and times and timestamps?
|
|
||
| if dtype.is_numeric(): | ||
| typ = pl.Datetime(time_unit="us", time_zone=time_zone) | ||
| time_unit = _TIMESTAMP_SCALE_TO_UNITS.get(to.scale, "us") |
There was a problem hiding this comment.
Shouldnt all this stuff be inside the _cast_string_to_temporal()?
Description of changes
Make Ibis compatible with Polars 2.0.0 while keeping Polars 1.x support, without version branching in source. This lands in five commits, so that the first three are validated in CI against the currently locked Polars 1.37.1 before the lock is bumped:
fix(polars): use APIs that also work on Polars 2.0ArrayFlatten:Expr.flatten()was removed in Polars 2.0. The removal error recommendsExpr.list.explode(keep_nulls=False, empty_as_null=False), and we follow that recommendation; however, those keyword arguments only exist from Polars 1.36 onward, andexplode's defaults differ between 1.x and 2.0. To get the recommended semantics on every supported version, we filter out null and empty inner arrays first and then callexplode()with its defaults, which is equivalent to the recommended call. This also fixes empty inner arrays previously being flattened to a null, so the correspondingnotyetmarker is removed.Between: casting strings to temporal types was removed in Polars 2.0. Usestr.strptimefor string bounds.DayOfWeekName: the 7-branchwhen/thenchain returns zero rows on Polars >=1.44.0 due to an upstream bug, fixed in fix: Fix small bugs in SQL aggregates and in group contexts pola-rs/polars#29754. Mapdt.weekday() - 1tocalendar.day_namewithreplace_strictinstead, which is also ~4x faster.test_unnest_empty_array: Polars 2.0 drops empty lists onexplode, matching other backends; xfail only forpolars<2viaxfail_version.fix(polars): correctly cast strings to time values: parse withstr.strptime(pl.Time, "%H:%M:%S%.f"). Polars 2.0 removed string-to-time casts, and on Polars 1.x the plain cast raised on valid input whiletry_castreturned all nulls. Adds cross-backendstring_to_timecases totest_cast,test_try_cast, andtest_try_cast_null.fix(polars): centralize string-to-temporal parsing: casts andbetweennow parse strings into dates, timestamps, and times with one shared helper, so string bounds inbetweenare parsed exactly like an explicit cast. Timestamps are parsed at the precisionPolarsTypemaps the target type to. This also fixes casting strings totimestamp(0), which raised because Polars has no second-precision datetime type. Adds Polars tests for both.chore(deps): update locked Polars version to 2.0.0ci: ignore the google-api-core Python 3.10 warning (#12158):google.api_corenow warns at import that Python 3.10 is past Google's support window, whichfilterwarnings = ["error"]turns into a collection error in every Python 3.10 job. Ignore it alongside the other google-api-core filters. Also opened separately as ci: ignore the google-api-core Python 3.10 warning #12158; if that merges first, this commit drops out on rebase.🤖 Generated with Claude Code