Repository navigation
fix: Fix SQL casts - #28986
Merged
Merged
fix: Fix SQL casts#28986
Conversation
ritchie46
requested review from
MarcoGorelli,
alexander-beedie,
c-peters and
orlp
as code owners
August 26, 2026 12:42
github-actions
Bot
added
A-sql
Contributor
|
The uncompressed lib size after this PR is 60.0713 MB. |
Extends the SQL string->temporal cast tests with literal operands, which exercise the constant-folded (eager) path rather than the streaming node. Moves the engine-consistency check out of the SQL suite into test_to_datetime.py, parametrised over both engines and all three of to_date/to_datetime/to_time, since it is about strptime rather than casts. Also includes a doc-comment trim on parse_string_as_temporal that was already present in the working tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MsiVh8nEYU5sBSXDcMgGSB
Format inference had three implementations. Two of them inferred from a
single value: `as_time`/`as_date_not_exact`/`as_datetime_not_exact` used the
first non-null value of the column, and the streaming `StrptimeInferNode`
used the first non-null value of each morsel, emitting an all-null morsel
when that value yielded no pattern.
The streaming case gave silently wrong results on the default engine. With
one unparseable value at the head of a 200k-row frame, `to_date(strict=False)`
returned 8334 nulls (one morsel) where the in-memory engine returned 1, so
the answer depended on where the bad values sat and on the morsel size.
`to_time` was wrong in both engines for the same reason.
Extracts the scan `infer.rs` already performed into `infer_from_values` and
uses it everywhere, so a value is null only when it is genuinely unparseable.
Also stops the streaming strict error reporting the internal alias assigned
during lowering ('_POLARS_TMP_0') rather than the column the user named.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MsiVh8nEYU5sBSXDcMgGSB
`convert_temporal_strings` re-inlined the same Date/Time/Datetime dispatch that `parse_string_as_temporal` performs, so both now call the helper. It returns `Option<Expr>` rather than `PolarsResult<Expr>`, which drops an unreachable error arm. The strptime dispatch repeated the same tail -- settle a failed inference, then report the values that failed under `strict` -- in `to_date`, `to_datetime` and `to_time`; that tail moves into `finish_strptime`. Also drops a `match` on `first_non_null()` whose binding is no longer used, for the early-return form the sibling module already uses. No behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MsiVh8nEYU5sBSXDcMgGSB
`first_non_null().is_some()` reads as a search when the predicate is just "not entirely null"; state that directly. `first_non_null` already answers both ends with an integer comparison, so this costs nothing. Also trims comments that narrated the previous behaviour rather than describing the code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MsiVh8nEYU5sBSXDcMgGSB
ritchie46
commented
Aug 26, 2026
| if infer_slot.is_none() { | ||
| let df = morsel.df().await; | ||
| let ca = df.columns()[0].str()?; | ||
| if let Some(idx) = ca.first_non_null() { |
Member
Author
There was a problem hiding this comment.
first_non_null may be an invalid str
Contributor
|
The uncompressed lib size after this PR is 60.3345 MB. |
Contributor
|
The uncompressed lib size after this PR is 60.3345 MB. |
dsprenkels
added a commit
to dsprenkels/polars
that referenced
this pull request
Sep 8, 2026
The claim that `::date`/`::timestamp` on a SQL string literal no longer works was true when written (pola-rs#28788), but pola-rs#28986/pola-rs#29007 made SQL parse string->temporal instead of casting, so those all succeed again. This commit was generated using Claude Opus 5.
paddymul
added a commit
to buckaroo-data/tallyman
that referenced
this pull request
Sep 24, 2026
…231) polars' CSV reader, given pl.Datetime(unit, time_zone=tz), parses text with no UTC offset as UTC and converts it into the zone. A zoned column is now read as text and parsed with str.to_datetime(time_unit, time_zone=tz), which attaches the zone to offset-less text and converts text that has an offset, as ADR-005 D9(a) says. The declared unit is kept (#145). It raises instead of shifting a value when offset-less text names a time the zone skips or repeats at a DST change, and when a column mixes text with and without an offset: the streaming engine infers one format per column from its first non-null value and parses every later batch with it. polars 1.40.1's streaming parse writes a batch as nulls, without an error, when that first value matches no format (fixed upstream in pola-rs/polars#28986). The first non-null value of each zoned column is parsed eagerly before the read, which raises in exactly that case. After a failed read, _explain_zoned_failure reads the zoned columns again as text and names the column, the row and the value: a mixed column, a skipped or repeated wall-clock time, or text that is not a timestamp. The ladder now also catches InvalidOperationError, which is how str.to_datetime reports text that does not match the column's format. Guard tests, which pass before and after: offset text converts, UTC reads offset-less text as UTC, a naive timestamp stays naive, a zoned column renamed by position beside inferred columns, and text that is not a timestamp raises as the first value or a later one. ADR-005 D9 gets an implementation note: (a) and (b) were both wrong about polars 1.40.1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Sep 24, 2026
2 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This was regressed because of the 2.0 cast semantics which don't allow for this native casts.
As a drive by, fix the streaming date infer, which took the first value, even if it was flawed.
Made with opus 5