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/10/13 16:07:25 UTC
(datafusion) branch gh-readonly-queue/main/pr-26251-1d165c5ccadc3a02a5e482b32315ed757b7dca91 created (now bd0da7664c)
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-26251-1d165c5ccadc3a02a5e482b32315ed757b7dca91
in repository https://gitbox.apache.org/repos/asf/datafusion.git
at bd0da7664c refactor: simplify redundant iterator chains (#26251)
This branch includes the following new commits:
new bd0da7664c refactor: simplify redundant iterator chains (#26251)
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: refactor: simplify redundant iterator chains (#26251)
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-26251-1d165c5ccadc3a02a5e482b32315ed757b7dca91
in repository https://gitbox.apache.org/repos/asf/datafusion.git
commit bd0da7664c1a8695a96401c563f4cd5f763c3f6a
Author: Oleks V <co...@users.noreply.github.com>
AuthorDate: Tue Oct 13 15:11:58 2026 +0000
refactor: simplify redundant iterator chains (#26251)
## Which issue does this PR close?
- N/A
## Rationale for this change
A few iterator chains do more work than needed. Some build an
intermediate `Vec` only to iterate it again, some use several adaptors
where one would do, and some reimplement `rfind`, `rposition`, `last` or
`to_vec` by hand. Each case is small, but the simpler forms avoid
allocations and clones and are easier to read.
## What changes are included in this PR?
- `invariants.rs`: a `map`, `collect::<Result<Vec>>`, `into_iter`,
`flatten` chain becomes one `flat_map` over `column_refs()`. Columns are
no longer cloned.
- `window_agg_exec.rs`: drop a
`collect::<Vec<_>>().into_iter().collect::<Result<_>>()` round trip.
- `sql/utils.rs`: `take_while`, `to_owned`, `cloned` and `map(unwrap)`
become a single `map_while`.
- `sql/statement.rs`: `iter().last()` becomes `last()`.
`rev().position()` with manual index math becomes `rposition`.
- `aggregates/mod.rs`: merge two consecutive `filter` calls.
- `bytes_view.rs`: `iter().rev().find()` becomes `rfind()`.
- `information_schema.rs` and `ffi/config/mod.rs`: fuse `filter_map`
followed by `map`.
- `file_scan_config/proto.rs`: `iter().cloned().collect::<Vec<_>>()`
becomes `to_vec()`.
## What is the testing strategy for this PR?
No behavior change, so existing tests cover it. Passed locally:
- `cargo fmt --all` and `cargo clippy -D warnings` on the touched
crates.
- Lib and integration tests for `datafusion-sql`, `datafusion-expr`,
`datafusion-ffi`, `datafusion-datasource`, and the
`datafusion-physical-plan` lib tests.
- sqllogictests matching `information_schema`, `window` and `aggregate`.
I did not run the full workspace suite or any benchmarks.
## Are there any user-facing changes?
No.
---
datafusion/catalog/src/information_schema.rs | 7 +++++--
datafusion/datasource/src/file_scan_config/proto.rs | 2 +-
datafusion/expr/src/logical_plan/invariants.rs | 13 ++++---------
datafusion/ffi/src/config/mod.rs | 6 ++----
.../group_values/multi_group_by/bytes_view.rs | 6 ++----
datafusion/physical-plan/src/aggregates/mod.rs | 8 ++++----
.../physical-plan/src/windows/window_agg_exec.rs | 2 --
datafusion/sql/src/statement.rs | 20 ++++++--------------
datafusion/sql/src/utils.rs | 5 +----
9 files changed, 25 insertions(+), 44 deletions(-)
diff --git a/datafusion/catalog/src/information_schema.rs b/datafusion/catalog/src/information_schema.rs
index 9e3a1d8743..3d21c12ccf 100644
--- a/datafusion/catalog/src/information_schema.rs
+++ b/datafusion/catalog/src/information_schema.rs
@@ -498,8 +498,11 @@ fn get_args_and_return_types(
let return_types = arg_fields
.into_iter()
.multi_cartesian_product()
- .filter_map(|arg_fields| return_field(&arg_fields).ok())
- .map(|f| Some(remove_native_type_prefix(&f.data_type().into())))
+ .filter_map(|arg_fields| {
+ return_field(&arg_fields)
+ .ok()
+ .map(|f| Some(remove_native_type_prefix(&f.data_type().into())))
+ })
.collect::<BTreeSet<_>>();
let return_types = if return_types.is_empty() {
// Indicate `None` if the return type cannot be represented from a signature,
diff --git a/datafusion/datasource/src/file_scan_config/proto.rs b/datafusion/datasource/src/file_scan_config/proto.rs
index 9370a0cfb7..706784a170 100644
--- a/datafusion/datasource/src/file_scan_config/proto.rs
+++ b/datafusion/datasource/src/file_scan_config/proto.rs
@@ -125,7 +125,7 @@ impl FileScanConfig {
// Partition fields must be added to the schema so they can persist in
// protobuf and then be removed again in `parse_table_schema_from_proto`.
- let mut fields = file_schema.fields().iter().cloned().collect::<Vec<_>>();
+ let mut fields = file_schema.fields().to_vec();
fields.extend(table_partition_cols.iter().cloned());
let schema = Schema::new(fields).with_metadata(file_schema.metadata.clone());
diff --git a/datafusion/expr/src/logical_plan/invariants.rs b/datafusion/expr/src/logical_plan/invariants.rs
index 375c7dfe08..0614a8602c 100644
--- a/datafusion/expr/src/logical_plan/invariants.rs
+++ b/datafusion/expr/src/logical_plan/invariants.rs
@@ -383,15 +383,10 @@ fn check_aggregation_in_scalar_subquery(
let correlated_exprs = get_correlated_expressions(inner_plan)?;
let inner_subquery_cols =
collect_subquery_cols(&correlated_exprs, agg.input.schema())?;
- let mut group_columns = agg
- .group_expr
- .iter()
- .map(|group| Ok(group.column_refs().into_iter().cloned().collect::<Vec<_>>()))
- .collect::<Result<Vec<_>>>()?
- .into_iter()
- .flatten();
-
- if !group_columns.all(|group| inner_subquery_cols.contains(&group)) {
+ let mut group_columns =
+ agg.group_expr.iter().flat_map(|group| group.column_refs());
+
+ if !group_columns.all(|group| inner_subquery_cols.contains(group)) {
// Group BY columns must be a subset of columns in the correlated expressions
return plan_err!(
"A GROUP BY clause in a scalar correlated subquery cannot contain non-correlated columns"
diff --git a/datafusion/ffi/src/config/mod.rs b/datafusion/ffi/src/config/mod.rs
index 14e4ca8dea..a06c0a242e 100644
--- a/datafusion/ffi/src/config/mod.rs
+++ b/datafusion/ffi/src/config/mod.rs
@@ -46,8 +46,7 @@ impl From<&ConfigOptions> for FFI_ConfigOptions {
let base_options: SVec<(SString, SString)> = options
.entries()
.into_iter()
- .filter_map(|entry| entry.value.map(|value| (entry.key, value)))
- .map(|(key, value)| (key.into(), value.into()))
+ .filter_map(|entry| entry.value.map(|value| (entry.key.into(), value.into())))
.collect();
let mut extensions = FFI_ExtensionOptions::default();
@@ -137,8 +136,7 @@ impl From<&TableOptions> for FFI_TableOptions {
let base_options: SVec<(SString, SString)> = options
.entries()
.into_iter()
- .filter_map(|entry| entry.value.map(|value| (entry.key, value)))
- .map(|(key, value)| (key.into(), value.into()))
+ .filter_map(|entry| entry.value.map(|value| (entry.key.into(), value.into())))
.collect();
let mut extensions = FFI_ExtensionOptions::default();
diff --git a/datafusion/physical-plan/src/aggregates/group_values/multi_group_by/bytes_view.rs b/datafusion/physical-plan/src/aggregates/group_values/multi_group_by/bytes_view.rs
index 24eb944763..0303dab10b 100644
--- a/datafusion/physical-plan/src/aggregates/group_values/multi_group_by/bytes_view.rs
+++ b/datafusion/physical-plan/src/aggregates/group_values/multi_group_by/bytes_view.rs
@@ -441,10 +441,8 @@ impl<B: ByteViewType> ByteViewGroupValueBuilder<B> {
//
let first_n_views = split_vec_min_alloc(&mut self.views, n);
- let last_non_inlined_view = first_n_views
- .iter()
- .rev()
- .find(|view| ((**view) as u32) > 12);
+ let last_non_inlined_view =
+ first_n_views.iter().rfind(|view| ((**view) as u32) > 12);
// All taken views inlined
let Some(view) = last_non_inlined_view else {
diff --git a/datafusion/physical-plan/src/aggregates/mod.rs b/datafusion/physical-plan/src/aggregates/mod.rs
index 31ce7899d3..abf7f4d593 100644
--- a/datafusion/physical-plan/src/aggregates/mod.rs
+++ b/datafusion/physical-plan/src/aggregates/mod.rs
@@ -1335,11 +1335,11 @@ impl AggregateExec {
// it is not null in every group
let indices: Vec<usize> = indices
.into_iter()
- .filter(|idx| group_by.groups.iter().all(|group| !group[*idx]))
.filter(|idx| {
- input_eq_properties
- .is_expr_constant(&groupby_exprs[*idx])
- .is_none()
+ group_by.groups.iter().all(|group| !group[*idx])
+ && input_eq_properties
+ .is_expr_constant(&groupby_exprs[*idx])
+ .is_none()
})
.collect();
diff --git a/datafusion/physical-plan/src/windows/window_agg_exec.rs b/datafusion/physical-plan/src/windows/window_agg_exec.rs
index 89fafd054a..425af3a3c0 100644
--- a/datafusion/physical-plan/src/windows/window_agg_exec.rs
+++ b/datafusion/physical-plan/src/windows/window_agg_exec.rs
@@ -567,8 +567,6 @@ impl WindowAggStream {
let columns = transpose(partition_results)
.iter()
.map(|elems| concat(&elems.iter().map(|x| x.as_ref()).collect::<Vec<_>>()))
- .collect::<Vec<_>>()
- .into_iter()
.collect::<Result<Vec<ArrayRef>, ArrowError>>()?;
// combine with the original cols
diff --git a/datafusion/sql/src/statement.rs b/datafusion/sql/src/statement.rs
index f2fe369508..125483ea7a 100644
--- a/datafusion/sql/src/statement.rs
+++ b/datafusion/sql/src/statement.rs
@@ -1422,18 +1422,12 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
None => None,
};
// Validate default arguments
- let first_default = match args.as_ref() {
- Some(arg) => arg.iter().position(|t| t.default_expr.is_some()),
- None => None,
- };
- let last_non_default = match args.as_ref() {
- Some(arg) => arg
- .iter()
- .rev()
- .position(|t| t.default_expr.is_none())
- .map(|reverse_pos| arg.len() - reverse_pos - 1),
- None => None,
- };
+ let first_default = args
+ .as_ref()
+ .and_then(|arg| arg.iter().position(|t| t.default_expr.is_some()));
+ let last_non_default = args
+ .as_ref()
+ .and_then(|arg| arg.iter().rposition(|t| t.default_expr.is_none()));
if let (Some(pos_default), Some(pos_non_default)) =
(first_default, last_non_default)
&& pos_non_default > pos_default
@@ -2420,7 +2414,6 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
};
let col_name: &Ident = cols
.0
- .iter()
.last()
.ok_or_else(|| plan_datafusion_err!("Empty column id"))?
.as_ident()
@@ -2637,7 +2630,6 @@ impl<S: ContextProvider> SqlToRel<'_, S> {
) -> Result<String> {
let part = name
.0
- .iter()
.last()
.ok_or_else(|| plan_datafusion_err!("Empty column name"))?;
let ident = part
diff --git a/datafusion/sql/src/utils.rs b/datafusion/sql/src/utils.rs
index 882d6fa681..164e0e4196 100644
--- a/datafusion/sql/src/utils.rs
+++ b/datafusion/sql/src/utils.rs
@@ -441,10 +441,7 @@ impl RecursiveUnnestRewriter<'_> {
.iter()
.rev()
.skip_while(|item| item.is_none())
- .take_while(|item| item.is_some())
- .to_owned()
- .cloned()
- .map(|item| item.unwrap())
+ .map_while(|item| item.clone())
.collect()
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org