mbutrovich commented on code in PR #11187: URL: https://github.com/apache/arrow-rs/pull/11187#discussion_r4098156728
########## arrow-arith/benches/temporal.rs: ########## @@ -0,0 +1,84 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use std::hint::black_box; + +use arrow_arith::temporal::{DatePart, date_part}; +use arrow_array::PrimitiveArray; +use arrow_array::types::*; +use criterion::{Criterion, Throughput, criterion_group, criterion_main}; + +const SIZE: usize = 8192; + +const TIME_PARTS: [DatePart; 6] = [ + DatePart::Hour, + DatePart::Minute, + DatePart::Second, + DatePart::Millisecond, + DatePart::Microsecond, + DatePart::Nanosecond, +]; + +fn benchmark<T: ArrowTimestampType>(c: &mut Criterion, unit: &str, units_per_second: i64) { + // A deterministic mix of dates before and after the epoch, including subseconds. + let values: Vec<i64> = (0..SIZE) + .map(|i| { + let seconds = (i as i64 * 1_000_003) % 4_000_000_000 - 2_000_000_000; + seconds * units_per_second + (i as i64 * 7919) % units_per_second + }) + .collect(); + let with_nulls = PrimitiveArray::<T>::from_iter( + values + .iter() + .enumerate() + .map(|(i, &v)| (i % 5 != 0).then_some(v)), + ); + let array = PrimitiveArray::<T>::new(values.into(), None); + let mut group = c.benchmark_group(format!("timestamp_{unit}")); + group.throughput(Throughput::Elements(SIZE as u64)); + // Second timestamps have no fractional part, so their subsecond parts are + // constant zero and not worth measuring. + let parts = if units_per_second == 1 { + &TIME_PARTS[..3] + } else { + &TIME_PARTS[..] + }; + for &part in parts { + group.bench_function(format!("{part}/no_nulls"), |b| { + b.iter(|| black_box(date_part(black_box(&array), part).unwrap())) + }); + } + // Validity is not consulted per element, so one nullable case suffices. + group.bench_function("Hour/mixed_nulls", |b| { + b.iter(|| black_box(date_part(black_box(&with_nulls), DatePart::Hour).unwrap())) + }); + // Reference for the calendar conversion path. + group.bench_function("Year/control", |b| { + b.iter(|| black_box(date_part(black_box(&array), DatePart::Year).unwrap())) + }); + group.finish(); Review Comment: The description reports `mixed_nulls` for every part and a `timezone_control` case for `Minute` and `Nanosecond`. This bench only has `Hour/mixed_nulls` and `Year/control`, so readers can't reproduce most of the posted numbers with it. Can you add the `timezone_control` cases back? They're the only cases that show the timezone path didn't regress, and the description would then match what the bench runs. If you'd rather keep the bench small, please rerun it and update the numbers in the description to match. ########## arrow-arith/src/temporal.rs: ########## @@ -184,6 +184,10 @@ where /// Returns an [`Int32Array`] unless input was a dictionary type, in which case returns /// the dictionary but with this function applied onto its values. /// +/// Null inputs produce null outputs. A timestamp outside the supported calendar +/// range also produces null, except that for timestamps without a timezone the +/// time parts (`Hour` through `Nanosecond`) are defined for every value. Review Comment: Thanks for calling this out. I'm not sure which behavior is better here, and I'd be interested in what you and the other maintainers think. With this PR, one out-of-range row in a `TimestampSecond` array returns `Hour = 15` and `Year = NULL`. The same value with a timezone returns `NULL` for both, and casting it to `Time64` returns an error ([`as_time_res_with_timezone`](https://github.com/apache/arrow-rs/blob/03b652ea7a8e6d57f9e536d678a1ea2825458423/arrow-cast/src/cast/mod.rs#L611-L627), used by the [`Timestamp` to `Time64` cast](https://github.com/apache/arrow-rs/blob/03b652ea7a8e6d57f9e536d678a1ea2825458423/arrow-cast/src/cast/mod.rs#L1989-L2000)). If we'd like to keep the current semantics, one option is to check the range before taking the fast path. For `TimestampNanosecond` every `i64` is in Chrono's range, so it needs no check. For the other units, one branch-free pass over `values()` against the `DateTime::<Utc>::MIN_UTC` and `MAX_UTC` bounds would work, with a fallback to the existing path when a value is out of range. This is the check I tried, at the top of `timestamp_time_part`: ```rust if !matches!( part, DatePart::Hour | DatePart::Minute | DatePart::Second | DatePart::Millisecond | DatePart::Microsecond | DatePart::Nanosecond ) { return None; } if T::UNIT != TimeUnit::Nanosecond { let (min, max) = (DateTime::<Utc>::MIN_UTC, DateTime::<Utc>::MAX_UTC); let (lo, hi) = match T::UNIT { TimeUnit::Second => (min.timestamp(), max.timestamp()), TimeUnit::Millisecond => (min.timestamp_millis(), max.timestamp_millis()), _ => (min.timestamp_micros(), max.timestamp_micros()), }; if !array.values().iter().fold(true, |ok, &v| ok & (lo <= v) & (v <= hi)) { return None; } } ``` The early `matches!` keeps `Year` and the other calendar parts from paying for the scan. With this in place, `test_timestamp_time_parts_extremes` would expect `NULL` for the out-of-range values. When I tried this with your bench on an M5 Max, it added about 0.7 us per 8192 values. For example, ms `Hour` went from 5.37 to 6.08 us and ms `Millisecond` from 2.12 to 2.87 us, against about 43 us on the Chrono path. That seems like a reasonable cost to me, but you may see a better way to do it. If we keep the new behavior instead, the `api-change` label would make sure it shows up in the changelog. -- 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]
