You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datafusion.apache.org by gi...@apache.org on 2026/09/17 15:33:40 UTC
(datafusion) branch gh-readonly-queue/main/pr-25308-c4f72bad34249798182ac7de605eb63ab134f26e created (now 22f9a92178)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a change to branch gh-readonly-queue/main/pr-25308-c4f72bad34249798182ac7de605eb63ab134f26e
in repository https://gitbox.apache.org/repos/asf/datafusion.git
at 22f9a92178 perf(range): avoid eager ok_or in gen_range_timestamp (#25308)
This branch includes the following new commits:
new 22f9a92178 perf(range): avoid eager ok_or in gen_range_timestamp (#25308)
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org
(datafusion) 01/01: perf(range): avoid eager ok_or in gen_range_timestamp (#25308)
Posted by gi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-25308-c4f72bad34249798182ac7de605eb63ab134f26e
in repository https://gitbox.apache.org/repos/asf/datafusion.git
commit 22f9a921781ccfdf29be71bceac1f266e87cfa0e
Author: niebayes <ni...@gmail.com>
AuthorDate: Thu Sep 17 15:33:16 2026 +0000
perf(range): avoid eager ok_or in gen_range_timestamp (#25308)
## What I did
Replace per-row `ok_or(exec_datafusion_err!(...))` with `ok_or_else` in
`gen_range_timestamp` so the success path no longer allocates a
DataFusionError for every generated row (speed up ~5x on 100k rows). Add
a criterion bench for the timestamp range path.
``` shell
# Before, with eager ok_or
range_timestamp_100000 time: [60.762 ms 60.815 ms 60.868 ms]
# After, with lazy ok_or_else
range_timestamp_100000 time: [12.390 ms 12.401 ms 12.413 ms]
change: [−79.634% −79.608% −79.582%] (p = 0.00 < 0.05)
Performance has improved.
```
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases.
You can link an issue to this PR using the GitHub syntax. For example
`Closes #123` indicates that this PR will close issue #123.
-->
- Closes #25307 .
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly
in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand
your changes and offer better suggestions for fixes.
Please explain the problem you are trying to solve in terms of the
user-visible
behavior, rather than the implementation.
For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of
the
implementation. "COUNT(DISTINCT) returns wrong results when the column
contains
nulls" is the user-visible problem.
-->
Avoid eager ok_or on the hot path.
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here, but it
is sometimes worth providing a summary of the individual changes in this
PR.
-->
Replaces eager ok_or with lazy ok_or_else.
## What is the testing strategy for this PR?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
Briefly describe how this PR is tested, and point to the specific tests
you added. For example: 'This new feature is covered by the
`sqllogictest` cases added in `foo.slt`'.
If this PR does not add tests, explain why. For example, if the change
is already covered by existing tests, please mention it.
You should also check the `codecov` bot reply on this PR to confirm the
changed code is exercised.
-->
Added a criterion bench.
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.
If there are any breaking changes to public APIs, please add the `api
change` label.
-->
No
---------
Co-authored-by: Adam Gutglick <ad...@gmail.com>
---
datafusion/functions-nested/Cargo.toml | 4 ++
datafusion/functions-nested/benches/range.rs | 98 ++++++++++++++++++++++++++++
datafusion/functions-nested/src/range.rs | 32 +++++----
3 files changed, 121 insertions(+), 13 deletions(-)
diff --git a/datafusion/functions-nested/Cargo.toml b/datafusion/functions-nested/Cargo.toml
index 738178442d..728255cecf 100644
--- a/datafusion/functions-nested/Cargo.toml
+++ b/datafusion/functions-nested/Cargo.toml
@@ -145,3 +145,7 @@ name = "array_range"
[[bench]]
harness = false
name = "cardinality"
+
+[[bench]]
+harness = false
+name = "range"
diff --git a/datafusion/functions-nested/benches/range.rs b/datafusion/functions-nested/benches/range.rs
new file mode 100644
index 0000000000..91f5ce86af
--- /dev/null
+++ b/datafusion/functions-nested/benches/range.rs
@@ -0,0 +1,98 @@
+// 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 std::sync::Arc;
+
+use arrow::array::IntervalMonthDayNanoArray;
+use arrow::array::TimestampNanosecondArray;
+use arrow::datatypes::{
+ DataType, Field, IntervalMonthDayNanoType, IntervalUnit, TimeUnit,
+};
+use criterion::{Criterion, criterion_group, criterion_main};
+use datafusion_common::config::ConfigOptions;
+use datafusion_expr::{ColumnarValue, ScalarFunctionArgs, ScalarUDFImpl};
+use datafusion_functions_nested::range::Range;
+use rand::rngs::StdRng;
+use rand::{Rng, SeedableRng};
+
+const NUM_ROWS: usize = 100_000;
+const NANOS_PER_SECOND: i64 = 1_000_000_000;
+
+fn range_timestamp(c: &mut Criterion) {
+ let mut rng = StdRng::seed_from_u64(42);
+ // ~2016-era timestamps; each row generates a 1-second range (1 element),
+ // so the per-row timestamp-range setup dominates the measurement.
+ let start: TimestampNanosecondArray = (0..NUM_ROWS)
+ .map(|_| {
+ Some(1_452_499_200_000_000_000i64 + rng.random_range(0..NANOS_PER_SECOND))
+ })
+ .collect();
+ let stop: TimestampNanosecondArray = start
+ .iter()
+ .map(|v| v.map(|v| v + NANOS_PER_SECOND))
+ .collect();
+ let step = IntervalMonthDayNanoArray::from(vec![
+ IntervalMonthDayNanoType::make_value(
+ 0,
+ 0,
+ NANOS_PER_SECOND
+ );
+ NUM_ROWS
+ ]);
+
+ let udf = Range::new();
+ let ts_type = DataType::Timestamp(TimeUnit::Nanosecond, None);
+ let return_type = udf
+ .return_type(&[
+ ts_type.clone(),
+ ts_type.clone(),
+ DataType::Interval(IntervalUnit::MonthDayNano),
+ ])
+ .unwrap();
+ let return_field = Arc::new(Field::new("f", return_type, true));
+
+ let arg_fields = vec![
+ Field::new("a", ts_type.clone(), true).into(),
+ Field::new("b", ts_type, true).into(),
+ Field::new("c", DataType::Interval(IntervalUnit::MonthDayNano), true).into(),
+ ];
+ let config_options = Arc::new(ConfigOptions::default());
+ let args = vec![
+ ColumnarValue::Array(Arc::new(start)),
+ ColumnarValue::Array(Arc::new(stop)),
+ ColumnarValue::Array(Arc::new(step)),
+ ];
+
+ c.bench_function(&format!("range_timestamp_{NUM_ROWS}"), |b| {
+ b.iter(|| {
+ black_box(
+ udf.invoke_with_args(ScalarFunctionArgs {
+ args: args.clone(),
+ arg_fields: arg_fields.clone(),
+ number_rows: NUM_ROWS,
+ return_field: Arc::clone(&return_field),
+ config_options: Arc::clone(&config_options),
+ })
+ .expect("range should work on valid values"),
+ )
+ })
+ });
+}
+
+criterion_group!(benches, range_timestamp);
+criterion_main!(benches);
diff --git a/datafusion/functions-nested/src/range.rs b/datafusion/functions-nested/src/range.rs
index f9384586d6..f44b0480aa 100644
--- a/datafusion/functions-nested/src/range.rs
+++ b/datafusion/functions-nested/src/range.rs
@@ -461,28 +461,34 @@ impl Range {
}
let neg = TimestampNanosecondType::add_month_day_nano(start, step, start_tz)
- .ok_or(exec_datafusion_err!(
- "Cannot generate timestamp range where start + step overflows"
- ))?
+ .ok_or_else(|| {
+ exec_datafusion_err!(
+ "Cannot generate timestamp range where start + step overflows"
+ )
+ })?
.cmp(&start)
== Ordering::Less;
let stop_dt =
as_datetime_with_timezone::<TimestampNanosecondType>(stop, stop_tz)
- .ok_or(exec_datafusion_err!(
- "Cannot generate timestamp for stop: {}: {:?}",
- stop,
- stop_tz
- ))?;
+ .ok_or_else(|| {
+ exec_datafusion_err!(
+ "Cannot generate timestamp for stop: {}: {:?}",
+ stop,
+ stop_tz
+ )
+ })?;
let mut current = start;
let mut current_dt =
as_datetime_with_timezone::<TimestampNanosecondType>(current, start_tz)
- .ok_or(exec_datafusion_err!(
- "Cannot generate timestamp for start: {}: {:?}",
- current,
- start_tz
- ))?;
+ .ok_or_else(|| {
+ exec_datafusion_err!(
+ "Cannot generate timestamp for start: {}: {:?}",
+ current,
+ start_tz
+ )
+ })?;
let values = from_fn(|| {
let generate_series_should_end = self.include_upper_bound
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org