adriangb opened a new pull request, #25173: URL: https://github.com/apache/datafusion/pull/25173
## Which issue does this PR close? - Closes https://github.com/apache/datafusion/issues/25169 ## Rationale for this change `generate_series` and `range` over timestamps accepted only nanosecond precision. Any other `TimeUnit` failed with an error that says the argument is not a timestamp while printing a timestamp: ```sql SELECT * FROM generate_series( arrow_cast(TIMESTAMP '2024-01-01 00:00:00','Timestamp(Second, None)'), arrow_cast(TIMESTAMP '2024-01-03 00:00:00','Timestamp(Second, None)'), INTERVAL '1 day'); Error during planning: First argument must be a timestamp or NULL, got Literal(TimestampSecond(1704067200, None), None) ``` Second and millisecond precision are common in Parquet, so a series built from a column read from storage failed while the same logic written with literals succeeded, and the message sent the reader looking in the wrong place. There was no design question about result precision here: the output schema is already fixed at `Timestamp(Nanosecond, tz)`, so the function already produced nanoseconds and simply refused coarser *inputs*. ## What changes are included in this PR? Both timestamp bounds in `call_timestamp` (shared by `generate_series` and `range`) now go through a new `timestamp_arg_to_nanos` helper instead of hard-matching `ScalarValue::TimestampNanosecond`: - **All four `TimeUnit`s are accepted** and widened to nanoseconds. The output stays `Timestamp(Nanosecond, tz)`. - **Overflow is checked.** `Second`, `Millisecond` and `Microsecond` span far more than an `i64` of nanoseconds (roughly 1677–2262), so the widening is a `checked_mul`. An out-of-range bound is a planning error naming the argument, the offending value and the representable window, rather than a debug panic or a silent wrap in release builds: ``` First argument for generate_series is out of range of nanosecond timestamps: 9223372036854775807 (Timestamp(Second, None)) is outside 1677-09-21T00:12:43.145224192 to 2262-04-11T23:47:16.854775807 ``` (No panic was reachable from SQL through this path before: the timestamp path did no arithmetic on the bounds, and the `DATE` overload already used `checked_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 regardless of the unit it is stored in, and both sides end up on the same nanosecond scale before they are compared. - **Timezone handling is unchanged but now documented.** The output timezone still comes from the start argument. Differing timezones on the two bounds are deliberately *not* an error: an Arrow timezone changes how an instant is rendered, not which instant it is, so both bounds remain directly comparable. The start's zone is the one kept because it also anchors the calendar arithmetic that advances the series (month and day steps are applied in local time, so they follow that zone's DST rules). This is spelled out in a comment at the discard site. - **Error messages name the offending type** instead of dumping the whole `Expr`, for the second and third arguments and for the `DATE` overload's three arguments — e.g. `Second argument for generate_series must be a TIMESTAMP or NULL, got Int64`. Non-literal arguments get a distinct "must be a literal ..." message. Not changed, and worth calling out: the step argument still only accepts `Interval(MonthDayNano)`. That is not currently reachable as a limitation from SQL — `arrow_cast` to `Interval(DayTime)` / `Interval(YearMonth)` is itself unimplemented — so it is left alone rather than widened speculatively. ## 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 timezone - `timestamp_arg_keeps_naive_timestamps_naive` - `timestamp_arg_handles_nulls` — NULL of each precision, plus an untyped `NULL` - `timestamp_arg_overflow_boundary` — for each of `Second`/`Millisecond`/`Microsecond`, the largest and smallest representable values are accepted and the first value past each is rejected with a message naming the value and the range - `call_timestamp_accepts_mixed_precisions`, `call_timestamp_range_accepts_non_nanosecond_precision` (covers the `range` sibling), `call_timestamp_takes_timezone_from_start`, `call_timestamp_null_bound_is_empty_series`, `call_timestamp_reports_out_of_range_bound`, `call_timestamp_rejects_non_timestamp_bound` sqllogictest coverage extended in `datafusion/sqllogictest/test_files/table_functions.slt` (new "Timestamp precision" section next to the existing timestamp-range tests): all four units for `generate_series` and for `range`, a sub-second step over second-precision bounds, mixed-precision bounds in both directions, `arrow_typeof` assertions that the output is `Timestamp(Nanosecond, tz)` regardless of input unit, timezone-aware and differing-timezone bounds, NULL bounds at non-nanosecond precision, and the overflow boundary in both directions plus the new error-message text. Both suites pass: ``` cargo test -p datafusion-functions-table # 13 passed cargo test -p datafusion-sqllogictest --test sqllogictests # 505/505 files cargo clippy --all-targets -- -D warnings # clean ``` ## Are there any user-facing changes? Yes, both of them widening rather than breaking: - `generate_series` and `range` now accept `Timestamp(Second|Millisecond|Microsecond, _)` bounds where they previously errored. Queries that worked before keep working, and the result type is unchanged. - Several planning error messages are reworded to name the offending data type. No public API changes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
