comphead commented on code in PR #26686:
URL: https://github.com/apache/datafusion/pull/26686#discussion_r4365941706
##########
datafusion/functions/src/datetime/date_part.rs:
##########
@@ -891,6 +866,38 @@ mod tests {
}
}
+ /// `epoch` is NULL only for a NULL input, but the integer parts can be
NULL
+ /// for a non-null input, so they are always nullable.
+ #[test]
+ fn return_field_nullability() {
Review Comment:
The integer-part cases here are already exercised end to end by the non-null
reproducers at the end of the slt. The ones from #26685 fail on `main` with
`declared as non-nullable`. The only new information is that `epoch` follows
the input. Could this move to the slt with the existing `DESCRIBE` idiom (see
`describe.slt`)? For example `DESCRIBE SELECT extract(epoch from ts) AS e,
extract(second from ts) AS s FROM (VALUES (timestamp '2020-01-01')) t(ts)`,
once with and once without a NULL row. That shows the declared nullability
directly.
##########
datafusion/functions/src/datetime/date_part.rs:
##########
@@ -564,48 +504,83 @@ fn epoch(array: &dyn Array) -> Result<ArrayRef> {
Ok(Arc::new(f))
}
-/// Invoke [`date_part`] on an `array` (e.g. Timestamp) and convert the
-/// result to a total number of nanoseconds as an Int64 array.
+/// Return the seconds of `array`, with their fraction, as a number of
+/// milliseconds, microseconds or nanoseconds (`part`): the seconds of the
+/// minute for timestamps, times and intervals, and the total length for
+/// durations.
///
-/// This returns an Int64 rather than Int32 because there 1 billion
+/// `nanosecond` returns an Int64 rather than Int32 because there are 1 billion
/// `nanosecond`s in each second, so representing up to 60 seconds as
/// nanoseconds can be values up to 60 billion, which does not fit in Int32.
-fn seconds_ns(array: &dyn Array) -> Result<ArrayRef> {
- // Fast path for Date32 and Date64 - no nanoseconds
- if array.data_type() == &Date32 || array.data_type() == &Date64 {
- return Ok(Arc::new(Int64Array::from_iter_values_with_nulls(
- repeat_n(0, array.len()),
- array.nulls().cloned(),
- )));
- }
+fn subsecond_part(array: &dyn Array, part: DatePart) -> Result<ArrayRef> {
+ let len = array.len();
+ let nulls = array.nulls().cloned();
+ Ok(match (array.data_type(), part) {
+ // Fast path for Date32 and Date64 - no seconds
+ (Date32 | Date64, DatePart::Nanosecond) => {
+ Arc::new(Int64Array::new(vec![0; len].into(), nulls))
+ }
+ (Date32 | Date64, _) => Arc::new(Int32Array::new(vec![0; len].into(),
nulls)),
+ // arrow's `date_part(Nanosecond)` computes these, but returns Int32,
+ // which overflows past about 2.1 seconds
+ (Interval(ArrowIntervalUnit::DayTime), DatePart::Nanosecond) =>
Arc::new(
+ as_interval_dt_array(array)?
+ .unary::<_, Int64Type>(|x| (x.milliseconds % 60_000) as i64 *
1_000_000),
+ ),
+ (Interval(ArrowIntervalUnit::MonthDayNano), DatePart::Nanosecond) =>
Arc::new(
+ as_interval_mdn_array(array)?
+ .unary::<_, Int64Type>(|x| x.nanoseconds % 60_000_000_000),
+ ),
+ (Duration(Nanosecond), DatePart::Nanosecond) => {
+
Arc::new(as_duration_nanosecond_array(array)?.reinterpret_cast::<Int64Type>())
+ }
+ // arrow's cast returns NULL on overflow, like its `date_part`
+ (Duration(_), DatePart::Nanosecond) => Arc::new(
+ as_duration_nanosecond_array(&cast(array, &Duration(Nanosecond))?)?
+ .reinterpret_cast::<Int64Type>(),
+ ),
+ // arrow's sub-second parts of intervals and durations already include
+ // the whole seconds
+ (
+ Interval(ArrowIntervalUnit::DayTime |
ArrowIntervalUnit::MonthDayNano)
+ | Duration(_),
+ _,
+ ) => date_part(array, part)?,
+ (_, DatePart::Millisecond) => {
+ seconds_with_fraction::<Int32Type, 1_000>(array, part)?
+ }
+ (_, DatePart::Microsecond) => {
+ seconds_with_fraction::<Int32Type, 1_000_000>(array, part)?
+ }
+ (_, DatePart::Nanosecond) => {
+ seconds_with_fraction::<Int64Type, 1_000_000_000>(array, part)?
+ }
+ (_, part) => return internal_err!("{part} is not a sub-second part"),
+ })
+}
+/// Combine arrow's seconds of the minute with its fraction of the second in
+/// `part` units, for types where arrow returns them separately (timestamps
+/// and times).
+fn seconds_with_fraction<O, const PER_SECOND: i64>(
+ array: &dyn Array,
+ part: DatePart,
+) -> Result<ArrayRef>
+where
+ O: ArrowPrimitiveType,
+ i64: AsPrimitive<O::Native>,
+{
let secs = date_part(array, DatePart::Second)?;
// This assumes array is primitive and not a dictionary
let secs = as_int32_array(secs.as_ref())?;
- let subsecs = date_part(array, DatePart::Nanosecond)?;
- let subsecs = as_int32_array(subsecs.as_ref())?;
-
- // Special case where there are no nulls.
- if subsecs.null_count() == 0 {
- let r: Int64Array = binary(secs, subsecs, |secs, subsecs| {
- (secs as i64) * 1_000_000_000 + (subsecs as i64)
- })?;
- Ok(Arc::new(r))
- } else {
- // Nulls in secs are preserved, nulls in subsecs are treated as zero
to account for the case
- // where the number of nanoseconds overflows.
- let r: Int64Array = secs
- .iter()
- .zip(subsecs)
- .map(|(secs, subsecs)| {
- secs.map(|secs| {
- let subsecs = subsecs.unwrap_or(0);
- (secs as i64) * 1_000_000_000 + (subsecs as i64)
- })
- })
- .collect();
- Ok(Arc::new(r))
- }
+ let fraction = date_part(array, part)?;
Review Comment:
For timestamps and times this makes three passes. Two `date_part` calls each
run a chrono conversion per row, plus a timezone lookup per row and a timezone
string parse per call when a timezone is set. Then `binary` allocates a third
values buffer and a third bitmap for the null union. This matches the bench,
where timestamps stay at about 1.1x while intervals and durations gain 3.6x to
41x.
`timezone_part_typed` above already has the one-pass shape. One `unary_opt`
over `as_datetime_with_timezone::<T>` (or `as_datetime::<T>` without a
timezone), computing `dt.second() * PER_SECOND + dt.nanosecond() /
(1_000_000_000 / PER_SECOND)`, would do one conversion per row and one
allocation. `main` already used two passes, so a follow-up is fine. I have not
measured it. The `ns_tz` case you added is where I'd expect it to show most.
##########
datafusion/functions/src/datetime/date_part.rs:
##########
@@ -564,48 +504,83 @@ fn epoch(array: &dyn Array) -> Result<ArrayRef> {
Ok(Arc::new(f))
}
-/// Invoke [`date_part`] on an `array` (e.g. Timestamp) and convert the
-/// result to a total number of nanoseconds as an Int64 array.
+/// Return the seconds of `array`, with their fraction, as a number of
+/// milliseconds, microseconds or nanoseconds (`part`): the seconds of the
+/// minute for timestamps, times and intervals, and the total length for
+/// durations.
///
-/// This returns an Int64 rather than Int32 because there 1 billion
+/// `nanosecond` returns an Int64 rather than Int32 because there are 1 billion
/// `nanosecond`s in each second, so representing up to 60 seconds as
/// nanoseconds can be values up to 60 billion, which does not fit in Int32.
-fn seconds_ns(array: &dyn Array) -> Result<ArrayRef> {
- // Fast path for Date32 and Date64 - no nanoseconds
- if array.data_type() == &Date32 || array.data_type() == &Date64 {
- return Ok(Arc::new(Int64Array::from_iter_values_with_nulls(
- repeat_n(0, array.len()),
- array.nulls().cloned(),
- )));
- }
+fn subsecond_part(array: &dyn Array, part: DatePart) -> Result<ArrayRef> {
+ let len = array.len();
+ let nulls = array.nulls().cloned();
+ Ok(match (array.data_type(), part) {
+ // Fast path for Date32 and Date64 - no seconds
+ (Date32 | Date64, DatePart::Nanosecond) => {
+ Arc::new(Int64Array::new(vec![0; len].into(), nulls))
+ }
+ (Date32 | Date64, _) => Arc::new(Int32Array::new(vec![0; len].into(),
nulls)),
+ // arrow's `date_part(Nanosecond)` computes these, but returns Int32,
+ // which overflows past about 2.1 seconds
+ (Interval(ArrowIntervalUnit::DayTime), DatePart::Nanosecond) =>
Arc::new(
+ as_interval_dt_array(array)?
+ .unary::<_, Int64Type>(|x| (x.milliseconds % 60_000) as i64 *
1_000_000),
+ ),
+ (Interval(ArrowIntervalUnit::MonthDayNano), DatePart::Nanosecond) =>
Arc::new(
+ as_interval_mdn_array(array)?
+ .unary::<_, Int64Type>(|x| x.nanoseconds % 60_000_000_000),
+ ),
+ (Duration(Nanosecond), DatePart::Nanosecond) => {
+
Arc::new(as_duration_nanosecond_array(array)?.reinterpret_cast::<Int64Type>())
+ }
+ // arrow's cast returns NULL on overflow, like its `date_part`
+ (Duration(_), DatePart::Nanosecond) => Arc::new(
+ as_duration_nanosecond_array(&cast(array, &Duration(Nanosecond))?)?
+ .reinterpret_cast::<Int64Type>(),
+ ),
Review Comment:
These two arms can be one:
```rust
(Duration(_), DatePart::Nanosecond) => {
cast(&cast(array, &Duration(Nanosecond))?, &DataType::Int64)?
}
```
arrow's `cast` returns the input as is when the types already match. It
reinterprets `Duration(Nanosecond)` as `Int64` without copying, and it already
turns overflow into NULL for the unit change. That removes an arm and the
repeated `as_duration_nanosecond_array(..)?.reinterpret_cast::<Int64Type>()`
pair.
##########
datafusion/functions/src/datetime/date_part.rs:
##########
@@ -564,48 +504,83 @@ fn epoch(array: &dyn Array) -> Result<ArrayRef> {
Ok(Arc::new(f))
}
-/// Invoke [`date_part`] on an `array` (e.g. Timestamp) and convert the
-/// result to a total number of nanoseconds as an Int64 array.
+/// Return the seconds of `array`, with their fraction, as a number of
+/// milliseconds, microseconds or nanoseconds (`part`): the seconds of the
+/// minute for timestamps, times and intervals, and the total length for
+/// durations.
///
-/// This returns an Int64 rather than Int32 because there 1 billion
+/// `nanosecond` returns an Int64 rather than Int32 because there are 1 billion
/// `nanosecond`s in each second, so representing up to 60 seconds as
/// nanoseconds can be values up to 60 billion, which does not fit in Int32.
-fn seconds_ns(array: &dyn Array) -> Result<ArrayRef> {
- // Fast path for Date32 and Date64 - no nanoseconds
- if array.data_type() == &Date32 || array.data_type() == &Date64 {
- return Ok(Arc::new(Int64Array::from_iter_values_with_nulls(
- repeat_n(0, array.len()),
- array.nulls().cloned(),
- )));
- }
+fn subsecond_part(array: &dyn Array, part: DatePart) -> Result<ArrayRef> {
+ let len = array.len();
+ let nulls = array.nulls().cloned();
+ Ok(match (array.data_type(), part) {
+ // Fast path for Date32 and Date64 - no seconds
+ (Date32 | Date64, DatePart::Nanosecond) => {
+ Arc::new(Int64Array::new(vec![0; len].into(), nulls))
+ }
+ (Date32 | Date64, _) => Arc::new(Int32Array::new(vec![0; len].into(),
nulls)),
+ // arrow's `date_part(Nanosecond)` computes these, but returns Int32,
+ // which overflows past about 2.1 seconds
+ (Interval(ArrowIntervalUnit::DayTime), DatePart::Nanosecond) =>
Arc::new(
+ as_interval_dt_array(array)?
+ .unary::<_, Int64Type>(|x| (x.milliseconds % 60_000) as i64 *
1_000_000),
+ ),
+ (Interval(ArrowIntervalUnit::MonthDayNano), DatePart::Nanosecond) =>
Arc::new(
+ as_interval_mdn_array(array)?
+ .unary::<_, Int64Type>(|x| x.nanoseconds % 60_000_000_000),
+ ),
+ (Duration(Nanosecond), DatePart::Nanosecond) => {
+
Arc::new(as_duration_nanosecond_array(array)?.reinterpret_cast::<Int64Type>())
+ }
+ // arrow's cast returns NULL on overflow, like its `date_part`
+ (Duration(_), DatePart::Nanosecond) => Arc::new(
+ as_duration_nanosecond_array(&cast(array, &Duration(Nanosecond))?)?
+ .reinterpret_cast::<Int64Type>(),
+ ),
+ // arrow's sub-second parts of intervals and durations already include
+ // the whole seconds
+ (
+ Interval(ArrowIntervalUnit::DayTime |
ArrowIntervalUnit::MonthDayNano)
+ | Duration(_),
+ _,
+ ) => date_part(array, part)?,
Review Comment:
Could this be `Interval(_) | Duration(_)`? Today `Interval(YearMonth)` with
`millisecond` or `microsecond` falls through to `seconds_with_fraction`, whose
doc says it is for timestamps and times. It then fails on its first
`date_part(.., Second)`, so `extract(millisecond ..)` reports `Second does not
support: Interval(YearMonth)` (the slt at L1288 and L1291 pins this). arrow's
`YearMonth` kernel rejects every sub-second part with the requested name, so
the message would be accurate. One of those two `query error` cases would then
be enough.
##########
datafusion/sqllogictest/test_files/datetime/date_part.slt:
##########
@@ -1289,6 +1419,124 @@ SELECT extract(day from arrow_cast(NULL,
'Duration(Second)'))
----
NULL
+# nanosecond on a duration is its total length in nanoseconds
+query IIII
+SELECT extract(nanosecond from arrow_cast(1500000000, 'Duration(Nanosecond)')),
+ extract(nanosecond from arrow_cast(1500000, 'Duration(Microsecond)')),
+ extract(nanosecond from arrow_cast(1500, 'Duration(Millisecond)')),
+ extract(nanosecond from arrow_cast(2, 'Duration(Second)'))
+----
+1500000000 1500000000 1500000000 2000000000
+
+# around i32::MAX nanoseconds
+query III
+SELECT extract(nanosecond from arrow_cast(2147483647, 'Duration(Nanosecond)')),
+ extract(nanosecond from arrow_cast(2147483648, 'Duration(Nanosecond)')),
+ extract(nanosecond from arrow_cast(62500000000, 'Duration(Nanosecond)'))
+----
+2147483647 2147483648 62500000000
+
+query II
+SELECT extract(nanosecond from arrow_cast(-1500000000,
'Duration(Nanosecond)')),
+ extract(nanosecond from arrow_cast(-62500, 'Duration(Millisecond)'))
+----
+-1500000000 -62500000000
+
+query I
+SELECT extract(nanosecond from arrow_cast(column1, 'Duration(Millisecond)'))
+FROM (VALUES (1500), (NULL), (62500), (NULL), (-2))
+----
+1500000000
+NULL
+62500000000
+NULL
+-2000000
+
+query II
+SELECT extract(nanosecond from arrow_cast(9223372036854775807,
'Duration(Nanosecond)')),
+ extract(nanosecond from arrow_cast(-9223372036854775808,
'Duration(Nanosecond)'))
+----
+9223372036854775807 -9223372036854775808
+
+# values that overflow are NULL, like the other parts of a duration
+query II
+SELECT extract(nanosecond from arrow_cast(column1, 'Duration(Second)')),
+ extract(nanosecond from arrow_cast(column2, 'Duration(Millisecond)'))
+FROM (VALUES (9223372036, -9223372036854), (9223372037, -9223372036855),
(NULL, NULL))
+----
+9223372036000000000 -9223372036854000000
+NULL NULL
+NULL NULL
+
+# millisecond and microsecond on a duration are its total length in that unit
+query II
+SELECT extract(millisecond from arrow_cast(5500000000,
'Duration(Nanosecond)')),
+ extract(microsecond from arrow_cast(5500000000, 'Duration(Nanosecond)'))
+----
+5500 5500000
+
+# truncates toward zero, like the other parts of a duration
+query III
+SELECT extract(millisecond from arrow_cast(-1500, 'Duration(Microsecond)')),
+ extract(millisecond from arrow_cast(-1500000, 'Duration(Nanosecond)')),
+ extract(second from arrow_cast(-1500, 'Duration(Millisecond)'))
+----
+-1 -1 -1
+
+query I
+SELECT extract(microsecond from arrow_cast(column1, 'Duration(Nanosecond)'))
+FROM (VALUES (5500000000), (NULL), (-62500000000), (NULL))
+----
+5500000
+NULL
+-62500000
+NULL
+
+# around i32::MAX
+query II
+SELECT extract(millisecond from arrow_cast(2147483647,
'Duration(Millisecond)')),
+ extract(microsecond from arrow_cast(2147, 'Duration(Second)'))
+----
+2147483647 2147000000
+
+# values that overflow Int32 are NULL, like the other parts of a duration
+query II
+SELECT extract(millisecond from arrow_cast(column1, 'Duration(Millisecond)')),
+ extract(microsecond from arrow_cast(column2, 'Duration(Second)'))
+FROM (VALUES (2147483648, 2148), (-2147483649, -2148), (NULL, NULL))
+----
+NULL NULL
+NULL NULL
+NULL NULL
+
+# Reproducer from https://github.com/apache/datafusion/issues/14738: 2.4e9
microseconds don't fit in Int32
+query I
+SELECT date_part('microsecond', timestamp '1970-01-01T00:40:00' - timestamp
'1970-01-01T00:00:00')
+----
Review Comment:
This is the query from #14738, which asks for the Postgres result. In
Postgres `timestamp - timestamp` is an interval and `microsecond` is the
seconds field within the minute, so the issue expects 0. Here the difference is
a `Duration`, and `microsecond` is its total length. Any duration longer than
about 35.8 minutes is then NULL, and `millisecond` is NULL past about 24.8 days.
That matches how arrow treats the other `Duration` parts, so I think it is
defensible. But it is a third answer to the issue, after the wrapped value and
the panic. Should this be `Part of #14738`, with the semantics question left
open? Or is NULL the agreed outcome? If total length is the intended meaning, a
follow-up could return `Int64` for `Duration` input, since
`return_field_from_args` sees the input type. That would avoid most of these
NULLs.
##########
datafusion/spark/src/function/datetime/date_part.rs:
##########
@@ -81,10 +81,10 @@ impl ScalarUDFImpl for SparkDatePart {
internal_err!("Use return_field_from_args in this case instead.")
}
- fn return_field_from_args(&self, args: ReturnFieldArgs) ->
Result<FieldRef> {
- let nullable = args.arg_fields.iter().any(|f| f.is_nullable());
-
- Ok(Arc::new(Field::new(self.name(), DataType::Int32, nullable)))
+ fn return_field_from_args(&self, _args: ReturnFieldArgs) ->
Result<FieldRef> {
+ // Nullable like the integer parts of `date_part`, which this
simplifies
+ // to, because they can be NULL for a non-null input
+ Ok(Arc::new(Field::new(self.name(), DataType::Int32, true)))
}
Review Comment:
With the input no longer read, this override now does exactly what the trait
default does. `ScalarUDFImpl::return_field_from_args` builds
`Field::new(self.name(), self.return_type(..)?, true)`. So `return_type` could
return `Ok(DataType::Int32)` and the override could go.
Another option is to delegate to
`datafusion_functions::datetime::date_part().return_field_from_args(args)`,
which keeps the two matching by construction. That would also fix a mismatch
this PR does not touch. `nanosecond` and `epoch` pass through `simplify`
unchanged, but Spark declares `Int32` while `date_part` returns `Int64` and
`Float64`.
Either way, the new test checks `Int32` and nullable directly and then again
against `date_part`. The comparison alone is enough. Since `DESCRIBE` shows the
planned schema before simplification, it could also be a `DESCRIBE SELECT
date_part('SECOND', ts) ...` query in `spark/datetime/date_part.slt` instead of
a unit test.
##########
datafusion/sqllogictest/test_files/datetime/date_part.slt:
##########
@@ -1143,24 +1177,120 @@ create table t (id int, i interval) as values
(4, interval '8 months'),
(5, NULL);
-query III
+query IIII
select
id,
extract(second from i),
- extract(month from i)
+ extract(month from i),
+ extract(nanosecond from i)
from t
order by id;
----
-0 0 5
-1 0 3
-2 0 0
-3 2 0
-4 0 8
-5 NULL NULL
+0 0 5 10
+1 0 3 0
+2 0 0 2000000
+3 2 0 2000000000
+4 0 8 0
+5 NULL NULL NULL
statement ok
drop table t;
+# nanosecond on an interval is the seconds field (within the minute) in
nanoseconds,
+# like microsecond and millisecond
+query III
+SELECT extract(nanosecond from interval '1.5 seconds'),
+ extract(microsecond from interval '1.5 seconds'),
+ extract(millisecond from interval '1.5 seconds')
+----
+1500000000 1500000 1500
+
+# past i32::MAX nanoseconds
+query I
+SELECT extract(nanosecond from interval '62.5 seconds')
+----
+2500000000
+
+query I
+SELECT extract(nanosecond from interval '59.999999999 seconds')
+----
+59999999999
+
+query I
+SELECT extract(nanosecond from interval '1 day 2 hours 3 minutes')
+----
+0
+
+query I
+SELECT extract(nanosecond from interval '-1.5 seconds')
+----
+-1500000000
+
+query I
+SELECT extract(nanosecond from arrow_cast('5.1 seconds', 'Interval(DayTime)'))
+----
+5100000000
+
+query I
+SELECT extract(nanosecond from arrow_cast('62.5 seconds', 'Interval(DayTime)'))
+----
+2500000000
+
+query I
+SELECT extract(nanosecond from arrow_cast(NULL, 'Interval(DayTime)'))
+----
+NULL
+
+# millisecond and microsecond keep the fraction past i32::MAX nanoseconds
+query IIII
+SELECT extract(millisecond from interval '62.5 seconds'),
+ extract(microsecond from interval '62.5 seconds'),
+ extract(microsecond from interval '5.5 seconds'),
+ extract(microsecond from interval '59.999999 seconds')
+----
+2500 2500000 5500000 59999999
+
+query II
+SELECT extract(millisecond from interval '-62.5 seconds'),
+ extract(microsecond from interval '-62.5 seconds')
+----
+-2500 -2500000
+
+query II
+SELECT extract(millisecond from arrow_cast('62.5 seconds',
'Interval(DayTime)')),
+ extract(microsecond from arrow_cast('62.5 seconds',
'Interval(DayTime)'))
+----
+2500 2500000
+
+# Reproducer from https://github.com/apache/datafusion/issues/26678
+query III
+SELECT extract(nanosecond from arrow_cast(1500000000, 'Duration(Nanosecond)')),
+ extract(nanosecond from interval '1.5 seconds'),
+ extract(microsecond from interval '1.5 seconds')
+----
+1500000000 1500000000 1500000
Review Comment:
These repeat queries added above. Column 1 is L1424, and columns 2 and 3 are
L1201-L1202. In the next reproducer, the first column is L1245. Could the issue
links go on those queries instead?
--
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]
