fix: accept all timestamp precisions in generate_series/range - #25173
fix: accept all timestamp precisions in generate_series/range#25173adriangb wants to merge 1 commit into
generate_series/range#25173Conversation
`call_timestamp` hard-matched `ScalarValue::TimestampNanosecond` for both bounds, so any other `TimeUnit` was rejected with "First argument must be a timestamp or NULL, got Literal(TimestampSecond(...))" -- a message that says the argument is not a timestamp while printing one. Second and millisecond precision are common in Parquet, so a series over a column read from storage failed while the same query written with literals succeeded. The output schema was already fixed at `Timestamp(Nanosecond, tz)`, so there was no question about result precision: the function already produced nanoseconds and simply refused coarser inputs. Both bounds now go through `timestamp_arg_to_nanos`, which accepts all four `TimeUnit`s and widens to nanoseconds. `Second`, `Millisecond` and `Microsecond` span far more than an i64 of nanoseconds, so the widening is a checked multiplication that reports the offending value and the representable range instead of wrapping silently in release builds. The bounds are read independently, so mixed precisions work; the output timezone still comes from the start argument, which is now documented -- a timezone does not change the instant a bound denotes, and the start's zone is what anchors the calendar arithmetic that advances the series. The rejection messages for the remaining arguments (and for the `DATE` overload) now name the offending data type rather than dumping the whole `Expr`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #25173 +/- ##
==========================================
+ Coverage 81.60% 81.91% +0.31%
==========================================
Files 1123 1132 +9
Lines 408898 420823 +11925
Branches 408898 420823 +11925
==========================================
+ Hits 333670 344722 +11052
- Misses 55625 55781 +156
- Partials 19603 20320 +717 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Self-review QA pass on my own PR. I built 1. The misleading-error family is not closed, and the PR body claims it isThis is the same defect shape that #25169 names: an error that names the wrong type and dumps a raw The cause is the dispatch in Both reference engines return an empty series here, so an error is also the wrong answer: The behaviour is pre-existing, and a precision fix does not have to fix it. But the PR body says error messages now "name the offending type", and this path proves that untrue for the function the PR is about. Pick one:
2. The
|
Which issue does this PR close?
generate_seriesover timestamps accepts only nanosecond precision, with a misleading error #25169Rationale for this change
What a user hits
Build a timestamp series from second-precision bounds. Second and millisecond precision are common in Parquet, so this is what a series over a column read from storage looks like.
On
mainthe call fails, and the message says the argument is not a timestamp while it prints a timestamp:With this PR the same call works:
What changes for a user
generate_seriesandrangeaccept all four timestamp precisions, not nanoseconds alone. The same query written with a second-precision or millisecond-precision bound now returns a series instead of an error. Queries that worked before keep working, and the result type does not change.Error messages also name the type they rejected, so a reader can act on them.
The technical detail
generate_seriesandrangeover timestamps acceptedTimestamp(Nanosecond, _)alone. Any otherTimeUnitfailed. The dispatch incall_with_argsalready routed everyDataType::Timestamp(_, _)intocall_timestamp, andcall_timestampthen hard-matchedScalarValue::TimestampNanosecond. So a coarser bound reached a match arm that could not accept it, and fell through to a message about the wrong thing.There is no design question about the result precision. The output schema is already fixed at
Timestamp(Nanosecond, tz), so the function already produced nanoseconds. It refused only coarser inputs.Field research
Both reference engines accept coarser bounds, so this change moves DataFusion toward them. I measured both rather than quote their documentation.
PostgreSQL 17.11 (
postgres:17):Mixed precisions also work there:
DuckDB 1.5.2:
Two points carry over from both engines:
timestamp, DuckDB returnsTIMESTAMP, and this PR keepsTimestamp(Nanosecond, tz).Neither engine can arbitrate the timezone question below. Both store a
timestamptzas an instant with no per-value zone, so both render the series in the session zone. Arrow puts the zone in the value, so DataFusion must pick one, and no cross-engine comparison is meaningful there.What changes are included in this PR?
Both timestamp bounds in
call_timestamp, whichgenerate_seriesandrangeshare, go through a newtimestamp_arg_to_nanoshelper instead of a hard match onScalarValue::TimestampNanosecond.All four
TimeUnits are accepted and widened to nanoseconds. The output staysTimestamp(Nanosecond, tz).Overflow is checked.
Second,MillisecondandMicrosecondspan far more than ani64of nanoseconds, which covers roughly 1677 to 2262. The widening is achecked_mul, so an out-of-range bound is a plan error that names the argument, the value and the window:An unchecked multiply wraps in silence in a release build and emits a plausible but wrong series. No panic was reachable from SQL through this path before:
call_timestampdid no arithmetic on the bounds, and theDATEoverload already usedchecked_mul. The new multiplication is checked so that stays true now that coarser units get through.Mixed precisions work. The two bounds are read independently, so
generate_series(ts_second, ts_micro, INTERVAL '1 day')is fine. A timestamp denotes an instant whatever unit holds it, and both sides reach the same nanosecond scale before the comparison.Timezone handling does not change, but a comment now explains it. The output timezone still comes from the start argument. Two different timezones on the two bounds are deliberately not an error: an Arrow timezone changes how an instant is rendered, not which instant it is. The start's zone is the one kept because it also anchors the calendar arithmetic that advances the series. Month and day steps apply in local time, so they follow that zone's DST rules.
Error messages name the type they rejected instead of a dump of the whole
Expr. This covers the second and third arguments, and the three arguments of theDATEoverload. A non-literal argument gets a separate "must be a literal ..." message.Two limits stay, and both are worth a mention.
Interval(MonthDayNano)alone. That is not reachable as a limit from SQL, becausearrow_casttoInterval(DayTime)andInterval(YearMonth)is itself unimplemented. So it is left alone rather than widened on speculation.DATEbound and aTIMESTAMPbound still cannot be mixed. PostgreSQL and DuckDB both accept that mix. It is out of scope for a precision fix, and the new message at least names the type now.What is the testing strategy for this PR?
Unit tests in
datafusion/functions-table/src/generate_series.rs:timestamp_arg_accepts_all_time_units— all four units widen to the same instant and keep their timezonetimestamp_arg_keeps_naive_timestamps_naivetimestamp_arg_handles_nulls— a NULL of each precision, plus an untypedNULLtimestamp_arg_overflow_boundary— forSecond,MillisecondandMicrosecond, the largest and smallest representable values pass, and the first value past each one fails with a message that names the value and the windowcall_timestamp_accepts_mixed_precisionscall_timestamp_range_accepts_non_nanosecond_precision— covers therangesiblingcall_timestamp_takes_timezone_from_startcall_timestamp_null_bound_is_empty_seriescall_timestamp_reports_out_of_range_boundcall_timestamp_rejects_non_timestamp_boundsqllogictest coverage grows in
datafusion/sqllogictest/test_files/table_functions.slt, in a new "Timestamp precision" section beside the existing timestamp-range tests:generate_seriesand forrangearrow_typeofassertions that the output isTimestamp(Nanosecond, tz)whatever the input unitBoth suites pass:
Are there any user-facing changes?
Yes. Both of them widen behaviour rather than break it.
generate_seriesandrangeacceptTimestamp(Second|Millisecond|Microsecond, _)bounds, which errored before. Queries that worked before keep working, and the result type does not change.🤖 Generated with Claude Code