adriangb commented on code in PR #25173:
URL: https://github.com/apache/datafusion/pull/25173#discussion_r4020071700
##########
datafusion/functions-table/src/generate_series.rs:
##########
@@ -673,38 +735,42 @@ impl GenerateSeriesFuncImpl {
);
}
- // Parse start timestamp
- let (start_ts, tz) = match &exprs[0] {
- Expr::Literal(ScalarValue::TimestampNanosecond(ts, tz), _) => {
- (*ts, tz.clone())
- }
- other => {
- return plan_err!(
- "First argument must be a timestamp or NULL, got {:?}",
- other
- );
- }
- };
-
- // Parse end timestamp
- let end_ts = match &exprs[1] {
- Expr::Literal(ScalarValue::Null, _) => None,
- Expr::Literal(ScalarValue::TimestampNanosecond(ts, _), _) => *ts,
- other => {
- return plan_err!(
- "Second argument must be a timestamp or NULL, got {:?}",
- other
- );
- }
- };
+ // Parse the start and end timestamps.
+ //
+ // Both are widened to nanoseconds, so the two arguments do not have to
+ // agree on a `TimeUnit`: an Arrow timestamp denotes an instant
+ // regardless of the unit it happens to be stored in, and the output is
+ // nanoseconds either way (see the schema below).
+ let (start_ts, tz) =
+ timestamp_arg_to_nanos(&exprs[0], "First argument", self.name)?;
+ let (end_ts, _end_tz) =
+ timestamp_arg_to_nanos(&exprs[1], "Second argument", self.name)?;
+
+ // `_end_tz` is deliberately discarded: the output timezone comes from
+ // the start argument alone. A timezone on an Arrow timestamp does not
+ // change which instant it denotes, only how that instant is rendered,
+ // so a start and end carrying different timezones are still directly
+ // comparable once both are nanoseconds since the epoch -- there is
+ // nothing to reject. The start's zone is the one that is kept because
+ // it also anchors the calendar arithmetic that advances the series:
+ // month and day components of the step are applied in local time, so
+ // they follow that zone's DST rules.
// Parse step interval
let step_interval = match &exprs[2] {
Expr::Literal(ScalarValue::Null, _) => None,
Expr::Literal(ScalarValue::IntervalMonthDayNano(interval), _) =>
*interval,
+ Expr::Literal(scalar, _) => {
+ return plan_err!(
+ "Third argument for {} must be an INTERVAL or NULL, got
{:?}",
Review Comment:
All the argument errors now say `must be a TIMESTAMP / DATE / INTERVAL or
NULL constant, got …`, including the DATE overload below.
--
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]
