From 8b250e6efaa69106c9afbdc112437c01632edfe3 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 09:27:51 +0200 Subject: [PATCH 01/42] feat: enable 19 more clippy lints that need no code changes All of these are `allow` by default and currently have zero hits across the workspace (`--all-targets --all-features`), so they act purely as guards against future regressions: * Bug catchers: `same_functions_in_if_condition`, `self_only_used_in_recursion`, `unchecked_time_subtraction`, `expl_impl_clone_on_copy`, `into_iter_without_iter`, `iter_without_into_iter`, `unnecessary_safety_doc` * Performance: `large_stack_arrays`, `large_stack_frames`, `linkedlist`, `set_contains_or_insert`, `string_lit_chars_any` * Simplification / API hygiene: `empty_enums`, `fn_params_excessive_bools`, `iter_not_returning_iterator`, `non_std_lazy_statics`, `ptr_cast_constness`, `pub_without_shorthand`, `trait_duplication_in_bounds` Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 440b5ae389637..ba1a58a8e61b2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -224,21 +224,31 @@ decimal_bitwise_operands = "warn" default_union_representation = "warn" doc_include_without_cfg = "warn" empty_enum_variants_with_brackets = "warn" +empty_enums = "warn" exit = "warn" +expl_impl_clone_on_copy = "warn" flat_map_option = "warn" +fn_params_excessive_bools = "warn" fn_to_numeric_cast_any = "warn" imprecise_flops = "warn" index_refutable_slice = "warn" inefficient_to_string = "warn" infinite_loop = "warn" +into_iter_without_iter = "warn" invalid_upcast_comparisons = "warn" ip_constant = "warn" iter_filter_is_ok = "warn" iter_filter_is_some = "warn" +iter_not_returning_iterator = "warn" iter_on_empty_collections = "warn" +iter_without_into_iter = "warn" # Detects large stack-allocated futures that may cause stack overflow crashes (see threshold in clippy.toml) large_futures = "warn" large_include_file = "warn" +# Like `large_futures`, these guard against stack overflows +large_stack_arrays = "warn" +large_stack_frames = "warn" +linkedlist = "warn" macro_use_imports = "warn" manual_ilog2 = "warn" manual_instant_elapsed = "warn" @@ -250,24 +260,39 @@ mut_mut = "warn" # https://github.com/apache/datafusion/issues/18503 needless_pass_by_value = "warn" negative_feature_names = "warn" +# Prefer `std::sync::LazyLock` over the `lazy_static`/`once_cell` crates +non_std_lazy_statics = "warn" non_zero_suggestions = "warn" nonstandard_macro_braces = "warn" or_fun_call = "warn" path_buf_push_overwrite = "warn" pathbuf_init_then_push = "warn" precedence_bits = "warn" +ptr_cast_constness = "warn" pub_underscore_fields = "warn" +pub_without_shorthand = "warn" rc_mutex = "warn" ref_option_ref = "warn" +# Catches copy-paste bugs in `if`/`else if` chains +same_functions_in_if_condition = "warn" same_length_and_capacity = "warn" +# Catches a `&self` argument that is only threaded through recursive calls +self_only_used_in_recursion = "warn" +# Avoids hashing the key twice +set_contains_or_insert = "warn" str_split_at_newline = "warn" string_add_assign = "warn" +string_lit_chars_any = "warn" suspicious_xor_used_as_pow = "warn" trailing_empty_array = "warn" +trait_duplication_in_bounds = "warn" transmute_ptr_to_ptr = "warn" +# Subtracting `Instant`s panics on overflow; use `saturating_duration_since` +unchecked_time_subtraction = "warn" uninhabited_references = "warn" uninlined_format_args = "warn" unnecessary_lazy_evaluations = "warn" +unnecessary_safety_doc = "warn" unnecessary_self_imports = "warn" unused_async = "warn" unused_rounding = "warn" From 5f53e4c0303ac95979fe232cd5261148113bb389 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 09:38:08 +0200 Subject: [PATCH 02/42] feat: enable clippy::unused_peekable The `.peekable()` in the sqllogictest Postgres engine was never peeked, so it only added an extra layer of indirection. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/sqllogictest/src/engines/postgres_engine/mod.rs | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index ba1a58a8e61b2..746ed4b9b9af8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -295,6 +295,7 @@ unnecessary_lazy_evaluations = "warn" unnecessary_safety_doc = "warn" unnecessary_self_imports = "warn" unused_async = "warn" +unused_peekable = "warn" unused_rounding = "warn" used_underscore_binding = "warn" verbose_file_reads = "warn" diff --git a/datafusion/sqllogictest/src/engines/postgres_engine/mod.rs b/datafusion/sqllogictest/src/engines/postgres_engine/mod.rs index f085fb5708875..5d01befbf9041 100644 --- a/datafusion/sqllogictest/src/engines/postgres_engine/mod.rs +++ b/datafusion/sqllogictest/src/engines/postgres_engine/mod.rs @@ -171,7 +171,7 @@ impl Postgres { debug!("Handling COPY command: {sql}"); // Hacky way to find the 'filename' in the statement - let mut tokens = canonical_sql.split_whitespace().peekable(); + let mut tokens = canonical_sql.split_whitespace(); let mut filename = None; // COPY FROM '/opt/data/csv/aggregate_test_100.csv' ... From 068d3ade028e30fbd362a2b3e401a91da2d3d9e2 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 09:39:13 +0200 Subject: [PATCH 03/42] feat: enable clippy::fallible_impl_from Catches `From` implementations that can panic, where `TryFrom` would be the honest signature. All three existing hits would need a breaking API change to fix, so they get `#[expect]` for now. Two of them (`Constraint`) panic on a protobuf message with an unset `constraint_mode`, i.e. on malformed input. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 2 ++ datafusion/common/src/scalar/mod.rs | 4 ++++ datafusion/proto-common/src/from_proto/mod.rs | 8 ++++++++ 3 files changed, 14 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 746ed4b9b9af8..a06fce4717f40 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -227,6 +227,8 @@ empty_enum_variants_with_brackets = "warn" empty_enums = "warn" exit = "warn" expl_impl_clone_on_copy = "warn" +# Catches `From` impls that can panic +fallible_impl_from = "warn" flat_map_option = "warn" fn_params_excessive_bools = "warn" fn_to_numeric_cast_any = "warn" diff --git a/datafusion/common/src/scalar/mod.rs b/datafusion/common/src/scalar/mod.rs index cb0442392ad21..8d02836b246ac 100644 --- a/datafusion/common/src/scalar/mod.rs +++ b/datafusion/common/src/scalar/mod.rs @@ -5353,6 +5353,10 @@ impl From> for ScalarValue { } /// Wrapper to create ScalarValue::Struct for convenience +#[expect( + clippy::fallible_impl_from, + reason = "Making this fallible would be a breaking API change" +)] impl From> for ScalarValue { fn from(value: Vec<(&str, ScalarValue)>) -> Self { value diff --git a/datafusion/proto-common/src/from_proto/mod.rs b/datafusion/proto-common/src/from_proto/mod.rs index 169ff7f3d9ff2..67ff95a40d0a7 100644 --- a/datafusion/proto-common/src/from_proto/mod.rs +++ b/datafusion/proto-common/src/from_proto/mod.rs @@ -750,6 +750,10 @@ impl From for Constraints { } } +#[expect( + clippy::fallible_impl_from, + reason = "Making this fallible would be a breaking API change" +)] impl From for Constraint { fn from(value: protobuf::Constraint) -> Self { match value.constraint_mode.unwrap() { @@ -882,6 +886,10 @@ impl From for JoinSide { } } +#[expect( + clippy::fallible_impl_from, + reason = "Making this fallible would be a breaking API change" +)] impl From<&protobuf::Constraint> for Constraint { fn from(value: &protobuf::Constraint) -> Self { match &value.constraint_mode { From 6be523424a59952b744abf38ff299777e0703986 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 09:39:58 +0200 Subject: [PATCH 04/42] feat: enable clippy::float_cmp_const Catches exact float comparisons against constants, e.g. `x == 0.0`. The two existing hits in `value_transition!` really do want an exact comparison against `f32::MIN`/`MAX`, so they get `#[expect]`. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/expr-common/src/interval_arithmetic.rs | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index a06fce4717f40..316bb3ceacf47 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -230,6 +230,7 @@ expl_impl_clone_on_copy = "warn" # Catches `From` impls that can panic fallible_impl_from = "warn" flat_map_option = "warn" +float_cmp_const = "warn" fn_params_excessive_bools = "warn" fn_to_numeric_cast_any = "warn" imprecise_flops = "warn" diff --git a/datafusion/expr-common/src/interval_arithmetic.rs b/datafusion/expr-common/src/interval_arithmetic.rs index 68541e1e6b32c..0f18a591bb369 100644 --- a/datafusion/expr-common/src/interval_arithmetic.rs +++ b/datafusion/expr-common/src/interval_arithmetic.rs @@ -110,7 +110,15 @@ macro_rules! value_transition { Int16(Some(value)) if value == i16::$bound => Int16(None), Int32(Some(value)) if value == i32::$bound => Int32(None), Int64(Some(value)) if value == i64::$bound => Int64(None), + #[expect( + clippy::float_cmp_const, + reason = "We really do want to detect the exact bound here" + )] Float32(Some(value)) if value == f32::$bound => Float32(None), + #[expect( + clippy::float_cmp_const, + reason = "We really do want to detect the exact bound here" + )] Float64(Some(value)) if value == f64::$bound => Float64(None), DurationSecond(Some(value)) if value == i64::$bound => DurationSecond(None), DurationMillisecond(Some(value)) if value == i64::$bound => { From ac0937030990ab892439ec9ebb50c4b88798bad9 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 09:41:46 +0200 Subject: [PATCH 05/42] feat: enable clippy::lossy_float_literal Catches float literals that silently round, e.g. `let x: f32 = 0.1234567890123;`. The three existing hits are false positives: they spell out exact powers of two (2^64 and 2^64-2^41), which float `Display` renders with fewer digits, so the lint thinks precision was lost. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/common/src/scalar/mod.rs | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 316bb3ceacf47..d8a3360b6ff96 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -252,6 +252,7 @@ large_include_file = "warn" large_stack_arrays = "warn" large_stack_frames = "warn" linkedlist = "warn" +lossy_float_literal = "warn" macro_use_imports = "warn" manual_ilog2 = "warn" manual_instant_elapsed = "warn" diff --git a/datafusion/common/src/scalar/mod.rs b/datafusion/common/src/scalar/mod.rs index 8d02836b246ac..bbd12d3ff1ae3 100644 --- a/datafusion/common/src/scalar/mod.rs +++ b/datafusion/common/src/scalar/mod.rs @@ -9628,6 +9628,10 @@ mod tests { } #[test] + #[expect( + clippy::lossy_float_literal, + reason = "The literals below spell out exact powers of two, which float `Display` renders differently" + )] fn test_scalar_distance_u64_boundaries() { // 1. Full-domain integer ranges // i64::MIN to i64::MAX -> distance is u64::MAX From 8a2bdc3a6ce91c58b809327d371d2855f7716b36 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 09:43:27 +0200 Subject: [PATCH 06/42] feat: enable clippy::manual_midpoint `(a + b) / 2` overflows when `a + b` exceeds the type's range; `a.midpoint(b)` does not. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 2 ++ datafusion/physical-plan/src/joins/sort_merge_join/tests.rs | 2 +- datafusion/physical-plan/src/sorts/merge.rs | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index d8a3360b6ff96..7a619bb6a5d1e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -257,6 +257,8 @@ macro_use_imports = "warn" manual_ilog2 = "warn" manual_instant_elapsed = "warn" manual_is_power_of_two = "warn" +# `(a + b) / 2` can overflow; `a.midpoint(b)` cannot +manual_midpoint = "warn" match_wild_err_arm = "warn" mem_forget = "warn" mismatching_type_param_order = "warn" diff --git a/datafusion/physical-plan/src/joins/sort_merge_join/tests.rs b/datafusion/physical-plan/src/joins/sort_merge_join/tests.rs index 175a9c0ea7198..1d29db20dce1b 100644 --- a/datafusion/physical-plan/src/joins/sort_merge_join/tests.rs +++ b/datafusion/physical-plan/src/joins/sort_merge_join/tests.rs @@ -2537,7 +2537,7 @@ async fn spill_join_arrays_memory_accounting() -> Result<()> { // Memory limit: too small for a full batch, large enough for join_arrays. // Every batch hits the Err arm → spills → grow(join_arrays_mem). - let memory_limit = (size_estimation + join_arrays_mem) / 2; + let memory_limit = usize::midpoint(size_estimation, join_arrays_mem); assert!( memory_limit < size_estimation && memory_limit > join_arrays_mem, "limit {memory_limit} must be between join_arrays_mem {join_arrays_mem} \ diff --git a/datafusion/physical-plan/src/sorts/merge.rs b/datafusion/physical-plan/src/sorts/merge.rs index aa85a07b0adcd..bdfdbcf38b839 100644 --- a/datafusion/physical-plan/src/sorts/merge.rs +++ b/datafusion/physical-plan/src/sorts/merge.rs @@ -499,7 +499,7 @@ impl SortPreservingMergeStream { /// it takes as input the next item at (S0) and the loser of (S3, S4). #[inline] fn lt_leaf_node_index(&self, cursor_index: usize) -> usize { - (self.cursors.len() + cursor_index) / 2 + usize::midpoint(self.cursors.len(), cursor_index) } /// Find the parent node index for the given node index From 999c2da7a9a8bef940b655d8af8f8bc4a3c01f12 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 09:48:17 +0200 Subject: [PATCH 07/42] feat: enable clippy::literal_string_with_formatting_args MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Catches strings that contain `{...}` but are never actually formatted. This found two real bugs where the placeholder was silently printed verbatim: * `benchmarks/src/nlj.rs`: `"NLJ benchmark Q{query_id} failed…".to_string()` * `parquet_advanced_index.rs`: `.expect("metadata for file not found: {filename}")` The remaining hits are intentional: shell-style `${VAR:-default}` placeholders, `{rows}`-style templates substituted with `str::replace`, and braces inside expected struct output. Those get `#[expect]`. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 2 ++ benchmarks/src/bin/benchmark_runner.rs | 8 ++++++++ benchmarks/src/nlj.rs | 2 +- benchmarks/src/sql_benchmark.rs | 8 ++++++++ .../examples/data_io/parquet_advanced_index.rs | 2 +- datafusion/core/benches/topk_aggregate.rs | 4 ++++ datafusion/core/tests/dataframe/mod.rs | 4 ++++ datafusion/physical-plan/src/joins/hash_join/exec.rs | 4 ++++ .../substrait/tests/cases/roundtrip_logical_plan.rs | 4 ++++ 9 files changed, 36 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 7a619bb6a5d1e..1eeb43c16100e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -252,6 +252,8 @@ large_include_file = "warn" large_stack_arrays = "warn" large_stack_frames = "warn" linkedlist = "warn" +# Catches `"{foo}"` where the string is never actually formatted +literal_string_with_formatting_args = "warn" lossy_float_literal = "warn" macro_use_imports = "warn" manual_ilog2 = "warn" diff --git a/benchmarks/src/bin/benchmark_runner.rs b/benchmarks/src/bin/benchmark_runner.rs index c1700c42ba2aa..fc980a63bf260 100644 --- a/benchmarks/src/bin/benchmark_runner.rs +++ b/benchmarks/src/bin/benchmark_runner.rs @@ -1970,6 +1970,10 @@ description = "Run query one against CSV data." } #[tokio::test] + #[expect( + clippy::literal_string_with_formatting_args, + reason = "The `${VAR:-default}` braces are shell-style placeholders, not format args" + )] async fn cli_subgroup_filter_is_used_for_benchmark_replacements() { let temp = tempfile::tempdir().unwrap(); @@ -1992,6 +1996,10 @@ description = "Run query one against CSV data." } #[tokio::test] + #[expect( + clippy::literal_string_with_formatting_args, + reason = "The `${VAR:-default}` braces are shell-style placeholders, not format args" + )] async fn benchmark_replacements_use_explicit_data_dir() { let temp = tempfile::tempdir().unwrap(); diff --git a/benchmarks/src/nlj.rs b/benchmarks/src/nlj.rs index 485ee069d1bba..87bfbaf4dd50d 100644 --- a/benchmarks/src/nlj.rs +++ b/benchmarks/src/nlj.rs @@ -226,7 +226,7 @@ impl RunOpt { } Err(e) => { return Err(DataFusionError::Context( - "NLJ benchmark Q{query_id} failed with error:".to_string(), + format!("NLJ benchmark Q{query_id} failed with error:"), Box::new(e), )); } diff --git a/benchmarks/src/sql_benchmark.rs b/benchmarks/src/sql_benchmark.rs index f69012402a3c2..24db7e0a0fb2e 100644 --- a/benchmarks/src/sql_benchmark.rs +++ b/benchmarks/src/sql_benchmark.rs @@ -1761,6 +1761,10 @@ mod tests { } #[test] + #[expect( + clippy::literal_string_with_formatting_args, + reason = "The `${VAR:-default}` braces are shell-style placeholders, not format args" + )] fn process_replacements_uses_default_for_missing_variable() { let replacements = HashMap::new(); @@ -2300,6 +2304,10 @@ NULL|(empty) } #[tokio::test] + #[expect( + clippy::literal_string_with_formatting_args, + reason = "The `${VAR:-default}` braces are shell-style placeholders, not format args" + )] async fn parser_applies_data_dir_replacement_in_load_query_file() { let temp_dir = tempdir().expect("failed to create benchmark test directory"); let data_dir = temp_dir.path().join("non_default_data"); diff --git a/datafusion-examples/examples/data_io/parquet_advanced_index.rs b/datafusion-examples/examples/data_io/parquet_advanced_index.rs index b6440eb3e2078..39423648db715 100644 --- a/datafusion-examples/examples/data_io/parquet_advanced_index.rs +++ b/datafusion-examples/examples/data_io/parquet_advanced_index.rs @@ -583,7 +583,7 @@ impl ParquetFileReaderFactory for CachedParquetFileReaderFactory { let metadata = self .metadata .get(&filename) - .expect("metadata for file not found: {filename}"); + .unwrap_or_else(|| panic!("metadata for file not found: {filename}")); Ok(Box::new(ParquetReaderWithCache { filename, metadata: Arc::clone(metadata), diff --git a/datafusion/core/benches/topk_aggregate.rs b/datafusion/core/benches/topk_aggregate.rs index d8ca0d58b8d21..8fc6c954caa9c 100644 --- a/datafusion/core/benches/topk_aggregate.rs +++ b/datafusion/core/benches/topk_aggregate.rs @@ -335,6 +335,10 @@ fn assert_string_results_match( } } +#[expect( + clippy::literal_string_with_formatting_args, + reason = "The `{rows}`/`{limit}` placeholders are substituted with `str::replace`" +)] fn criterion_benchmark(c: &mut Criterion) { let rt = Runtime::new().unwrap(); let limit = LIMIT; diff --git a/datafusion/core/tests/dataframe/mod.rs b/datafusion/core/tests/dataframe/mod.rs index 4966c7aa9ae73..44ab14e6cc137 100644 --- a/datafusion/core/tests/dataframe/mod.rs +++ b/datafusion/core/tests/dataframe/mod.rs @@ -3303,6 +3303,10 @@ async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_with_reparti Ok(()) } +#[expect( + clippy::literal_string_with_formatting_args, + reason = "The `{testdata}` placeholder is substituted with `str::replace`" +)] async fn union_with_mix_of_presorted_and_explicitly_resorted_inputs_impl( repartition_sorts: bool, ) -> Result { diff --git a/datafusion/physical-plan/src/joins/hash_join/exec.rs b/datafusion/physical-plan/src/joins/hash_join/exec.rs index 08d209003ad91..0936685206708 100644 --- a/datafusion/physical-plan/src/joins/hash_join/exec.rs +++ b/datafusion/physical-plan/src/joins/hash_join/exec.rs @@ -6126,6 +6126,10 @@ mod tests { } #[tokio::test] + #[expect( + clippy::literal_string_with_formatting_args, + reason = "The braces are part of the expected struct output, not format args" + )] async fn join_on_struct() -> Result<()> { let task_ctx = Arc::new(TaskContext::default()); let left = diff --git a/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs b/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs index f084d3170edcc..747dfeb877d44 100644 --- a/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs +++ b/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs @@ -1477,6 +1477,10 @@ async fn roundtrip_literal_named_struct() -> Result<()> { } #[tokio::test] +#[expect( + clippy::literal_string_with_formatting_args, + reason = "The braces are part of the expected struct output, not format args" +)] async fn roundtrip_literal_renamed_struct() -> Result<()> { // This test aims to hit a case where the struct column itself has the expected name, but its // inner field needs to be renamed. From 1dbde56c4d2ec3350ecb156760746e3ffab2c945 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 09:53:07 +0200 Subject: [PATCH 08/42] feat: enable clippy::ref_as_ptr `&x as *const T` silently picks a pointer type; `std::ptr::from_ref(&x)` keeps the referent type explicit and cannot accidentally change it. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/ffi/src/lib.rs | 2 +- datafusion/ffi/src/udaf/accumulator.rs | 10 ++++++---- datafusion/ffi/src/udaf/groups_accumulator.rs | 10 ++++++---- datafusion/ffi/src/udwf/partition_evaluator.rs | 10 ++++++---- .../physical-expr-adapter/src/schema_rewriter.rs | 4 ++-- datafusion/physical-plan/src/statistics.rs | 4 ++-- 7 files changed, 24 insertions(+), 17 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 1eeb43c16100e..992e406cc1351 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -280,6 +280,7 @@ ptr_cast_constness = "warn" pub_underscore_fields = "warn" pub_without_shorthand = "warn" rc_mutex = "warn" +ref_as_ptr = "warn" ref_option_ref = "warn" # Catches copy-paste bugs in `if`/`else if` chains same_functions_in_if_condition = "warn" diff --git a/datafusion/ffi/src/lib.rs b/datafusion/ffi/src/lib.rs index de8f8cba9ca9b..43624e21882c3 100644 --- a/datafusion/ffi/src/lib.rs +++ b/datafusion/ffi/src/lib.rs @@ -85,7 +85,7 @@ static LIBRARY_MARKER: u8 = 0; /// /// See the crate's `README.md` for additional information. pub extern "C" fn get_library_marker_id() -> usize { - &LIBRARY_MARKER as *const u8 as usize + std::ptr::from_ref::(&LIBRARY_MARKER) as usize } /// For unit testing in this crate we need to trick the providers diff --git a/datafusion/ffi/src/udaf/accumulator.rs b/datafusion/ffi/src/udaf/accumulator.rs index 9dae09291f13c..d08567d369476 100644 --- a/datafusion/ffi/src/udaf/accumulator.rs +++ b/datafusion/ffi/src/udaf/accumulator.rs @@ -417,8 +417,9 @@ mod tests { // Verify local libraries can be downcast to their original let foreign_accum: Box = ffi_accum.into(); unsafe { - let concrete = &*(foreign_accum.as_ref() as *const dyn Accumulator - as *const AvgAccumulator); + let concrete = + &*(std::ptr::from_ref::(foreign_accum.as_ref()) + as *const AvgAccumulator); assert_eq!(original_size, concrete.size()); } @@ -429,8 +430,9 @@ mod tests { ffi_accum.library_marker_id = crate::mock_foreign_marker_id; let foreign_accum: Box = ffi_accum.into(); unsafe { - let concrete = &*(foreign_accum.as_ref() as *const dyn Accumulator - as *const ForeignAccumulator); + let concrete = + &*(std::ptr::from_ref::(foreign_accum.as_ref()) + as *const ForeignAccumulator); assert_eq!(original_size, concrete.size()); } diff --git a/datafusion/ffi/src/udaf/groups_accumulator.rs b/datafusion/ffi/src/udaf/groups_accumulator.rs index 4d1b0b4be0a2b..840787126d90c 100644 --- a/datafusion/ffi/src/udaf/groups_accumulator.rs +++ b/datafusion/ffi/src/udaf/groups_accumulator.rs @@ -540,8 +540,9 @@ mod tests { // Verify local libraries can be downcast to their original let foreign_accum: Box = ffi_accum.into(); unsafe { - let concrete = &*(foreign_accum.as_ref() as *const dyn GroupsAccumulator - as *const StddevGroupsAccumulator); + let concrete = + &*(std::ptr::from_ref::(foreign_accum.as_ref()) + as *const StddevGroupsAccumulator); assert_eq!(original_size, concrete.size()); } @@ -552,8 +553,9 @@ mod tests { ffi_accum.library_marker_id = crate::mock_foreign_marker_id; let foreign_accum: Box = ffi_accum.into(); unsafe { - let concrete = &*(foreign_accum.as_ref() as *const dyn GroupsAccumulator - as *const ForeignGroupsAccumulator); + let concrete = + &*(std::ptr::from_ref::(foreign_accum.as_ref()) + as *const ForeignGroupsAccumulator); assert_eq!(original_size, concrete.size()); } diff --git a/datafusion/ffi/src/udwf/partition_evaluator.rs b/datafusion/ffi/src/udwf/partition_evaluator.rs index c4c43f00d81fa..2e6243ddd1650 100644 --- a/datafusion/ffi/src/udwf/partition_evaluator.rs +++ b/datafusion/ffi/src/udwf/partition_evaluator.rs @@ -394,8 +394,9 @@ mod tests { // Verify local libraries can be downcast to their original let foreign_accum: Box = ffi_accum.into(); unsafe { - let concrete = &*(foreign_accum.as_ref() as *const dyn PartitionEvaluator - as *const TestPartitionEvaluator); + let concrete = + &*(std::ptr::from_ref::(foreign_accum.as_ref()) + as *const TestPartitionEvaluator); assert!(!concrete.uses_window_frame()); } @@ -406,8 +407,9 @@ mod tests { ffi_accum.library_marker_id = crate::mock_foreign_marker_id; let foreign_accum: Box = ffi_accum.into(); unsafe { - let concrete = &*(foreign_accum.as_ref() as *const dyn PartitionEvaluator - as *const ForeignPartitionEvaluator); + let concrete = + &*(std::ptr::from_ref::(foreign_accum.as_ref()) + as *const ForeignPartitionEvaluator); assert!(!concrete.uses_window_frame()); } diff --git a/datafusion/physical-expr-adapter/src/schema_rewriter.rs b/datafusion/physical-expr-adapter/src/schema_rewriter.rs index ef25af7d920fb..152859b1b78fb 100644 --- a/datafusion/physical-expr-adapter/src/schema_rewriter.rs +++ b/datafusion/physical-expr-adapter/src/schema_rewriter.rs @@ -1017,8 +1017,8 @@ mod tests { // Should be the same expression (no transformation needed) // We compare the underlying pointer through the trait object assert!(std::ptr::eq( - column_expr.as_ref() as *const dyn PhysicalExpr, - result.as_ref() as *const dyn PhysicalExpr + std::ptr::from_ref::(column_expr.as_ref()), + std::ptr::from_ref::(result.as_ref()) )); Ok(()) diff --git a/datafusion/physical-plan/src/statistics.rs b/datafusion/physical-plan/src/statistics.rs index 9246d7d9f5a9c..91fa20ce30e7f 100644 --- a/datafusion/physical-plan/src/statistics.rs +++ b/datafusion/physical-plan/src/statistics.rs @@ -47,7 +47,7 @@ impl StatsCache { partition: Option, ) -> Option<&Arc> { let key = ( - plan as *const dyn ExecutionPlan as *const () as usize, + std::ptr::from_ref::(plan) as *const () as usize, partition, ); self.0.get(&key) @@ -60,7 +60,7 @@ impl StatsCache { stats: Arc, ) { let key = ( - plan as *const dyn ExecutionPlan as *const () as usize, + std::ptr::from_ref::(plan) as *const () as usize, partition, ); self.0.insert(key, stats); From 913310ac9cb90314b625d55b8ea9018f66ebde0b Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 09:57:38 +0200 Subject: [PATCH 09/42] feat: enable clippy::unnecessary_safety_comment Catches `// SAFETY:` comments that do not sit in front of anything unsafe, so that a `SAFETY:` comment reliably means "an unsafe block follows, and here is why it is sound". * Three comments documented an `unwrap` or a safe copy rather than unsafe code, so they lose the `SAFETY:` prefix. * Three sat in front of an `if` while the `unsafe` block was inside it, so they move next to the block they justify. * Two were prose false positives, where the lint matched "safety:" in the middle of a doc comment. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/datasource-json/src/source.rs | 2 +- datafusion/functions-aggregate/src/percentile_cont.rs | 2 +- datafusion/functions/src/string/repeat.rs | 2 +- datafusion/functions/src/unicode/character_length.rs | 2 +- datafusion/optimizer/src/extract_equijoin_predicate.rs | 2 +- datafusion/physical-expr-common/src/binary_map.rs | 2 +- datafusion/physical-plan/src/sorts/cursor.rs | 5 ++++- datafusion/spark/src/function/string/length.rs | 2 +- 9 files changed, 12 insertions(+), 8 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 992e406cc1351..0e1ad685a3d0c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -301,6 +301,7 @@ unchecked_time_subtraction = "warn" uninhabited_references = "warn" uninlined_format_args = "warn" unnecessary_lazy_evaluations = "warn" +unnecessary_safety_comment = "warn" unnecessary_safety_doc = "warn" unnecessary_self_imports = "warn" unused_async = "warn" diff --git a/datafusion/datasource-json/src/source.rs b/datafusion/datasource-json/src/source.rs index 47241c9d99ab5..e8dc41cff3c57 100644 --- a/datafusion/datasource-json/src/source.rs +++ b/datafusion/datasource-json/src/source.rs @@ -66,7 +66,7 @@ const JSON_CONVERTER_BUFFER_SIZE: usize = 2 * 1024 * 1024; /// A stream wrapper that holds SpawnedTask handles to keep them alive /// until the stream is fully consumed or dropped. /// -/// This ensures cancel-safety: when the stream is dropped, the tasks +/// This ensures cancel-safety. When the stream is dropped, the tasks /// are properly aborted via SpawnedTask's Drop implementation. struct JsonArrayStream { inner: ReceiverStream>, diff --git a/datafusion/functions-aggregate/src/percentile_cont.rs b/datafusion/functions-aggregate/src/percentile_cont.rs index 3a98900bbb446..fae6fcf2d2007 100644 --- a/datafusion/functions-aggregate/src/percentile_cont.rs +++ b/datafusion/functions-aggregate/src/percentile_cont.rs @@ -63,7 +63,7 @@ use crate::utils::validate_percentile_expr; /// Precision multiplier for linear interpolation calculations. /// -/// This value of 1,000,000 was chosen to balance precision with overflow safety: +/// This value of 1,000,000 was chosen to balance precision against overflow: /// - Provides 6 decimal places of precision for the fractional component /// - Small enough to avoid overflow when multiplied with typical numeric values /// - Sufficient precision for most statistical applications diff --git a/datafusion/functions/src/string/repeat.rs b/datafusion/functions/src/string/repeat.rs index a53f1e2e4fc42..877ff98b7a92f 100644 --- a/datafusion/functions/src/string/repeat.rs +++ b/datafusion/functions/src/string/repeat.rs @@ -306,7 +306,7 @@ where // Doubling strategy: copy what we have so far until we reach the target while buffer.len() < src.len() * count { let copy_len = buffer.len().min(src.len() * count - buffer.len()); - // SAFETY: we're copying valid UTF-8 bytes that we already verified + // We're copying valid UTF-8 bytes that we already verified buffer.extend_from_within(..copy_len); } } diff --git a/datafusion/functions/src/unicode/character_length.rs b/datafusion/functions/src/unicode/character_length.rs index 9f0d952a02636..85d9595c4605a 100644 --- a/datafusion/functions/src/unicode/character_length.rs +++ b/datafusion/functions/src/unicode/character_length.rs @@ -158,10 +158,10 @@ where } else { let values: Vec<_> = (0..array.len()) .map(|i| { - // Safety: we are iterating with array.len() so the index is always valid if array.is_null(i) { T::default_value() } else { + // SAFETY: we are iterating with array.len() so the index is always valid let value = unsafe { array.value_unchecked(i) }; if value.is_empty() { T::default_value() diff --git a/datafusion/optimizer/src/extract_equijoin_predicate.rs b/datafusion/optimizer/src/extract_equijoin_predicate.rs index 0a50761e8a9f7..58f9a4cd42a2d 100644 --- a/datafusion/optimizer/src/extract_equijoin_predicate.rs +++ b/datafusion/optimizer/src/extract_equijoin_predicate.rs @@ -95,7 +95,7 @@ impl OptimizerRule for ExtractEquijoinPredicate { && equijoin_predicates.is_empty() && non_equijoin_expr.is_some() { - // SAFETY: checked in the outer `if` + // Checked in the outer `if` let expr = non_equijoin_expr.clone().unwrap(); let (equijoin_predicates, non_equijoin_expr) = split_is_not_distinct_from_and_other_join_predicate( diff --git a/datafusion/physical-expr-common/src/binary_map.rs b/datafusion/physical-expr-common/src/binary_map.rs index 44ca35c7f8708..0fe810f96188b 100644 --- a/datafusion/physical-expr-common/src/binary_map.rs +++ b/datafusion/physical-expr-common/src/binary_map.rs @@ -557,7 +557,7 @@ fn single_null_buffer(num_values: usize, null_index: usize) -> NullBuffer { null_builder.append_n_non_nulls(null_index); null_builder.append_null(); null_builder.append_n_non_nulls(num_values - null_index - 1); - // SAFETY: inner builder must be constructed + // The inner builder must be constructed null_builder.finish().unwrap() } diff --git a/datafusion/physical-plan/src/sorts/cursor.rs b/datafusion/physical-plan/src/sorts/cursor.rs index 003de2375ad3f..22037620e2efa 100644 --- a/datafusion/physical-plan/src/sorts/cursor.rs +++ b/datafusion/physical-plan/src/sorts/cursor.rs @@ -460,16 +460,19 @@ impl CursorValues for StringViewArray { #[inline(always)] fn compare(l: &Self, l_idx: usize, r: &Self, r_idx: usize) -> Ordering { - // SAFETY: Prior assertions guarantee that l_idx and r_idx are valid indices. + // Prior assertions guarantee that l_idx and r_idx are valid indices. // Null-checks are assumed to have been handled in the wrapper (e.g., ArrayValues). // And the bound is checked in is_finished, it is safe to call get_unchecked if l.data_buffers().is_empty() && r.data_buffers().is_empty() { + // SAFETY: see above let l_view = unsafe { l.views().get_unchecked(l_idx) }; + // SAFETY: see above let r_view = unsafe { r.views().get_unchecked(r_idx) }; return StringViewArray::inline_key_fast(*l_view) .cmp(&StringViewArray::inline_key_fast(*r_view)); } + // SAFETY: see above unsafe { GenericByteViewArray::compare_unchecked(l, l_idx, r, r_idx) } } diff --git a/datafusion/spark/src/function/string/length.rs b/datafusion/spark/src/function/string/length.rs index 8c5539a0577d8..57f40583d4f26 100644 --- a/datafusion/spark/src/function/string/length.rs +++ b/datafusion/spark/src/function/string/length.rs @@ -154,10 +154,10 @@ where } else { let values: Vec<_> = (0..array.len()) .map(|i| { - // Safety: we are iterating with array.len() so the index is always valid if array.is_null(i) { i32::default() } else { + // SAFETY: we are iterating with array.len() so the index is always valid let value = unsafe { array.value_unchecked(i) }; if value.is_empty() { i32::default() From 20f445307b72eda6d93348e286c0b2bbcf39a81a Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 09:58:34 +0200 Subject: [PATCH 10/42] feat: enable clippy::large_types_passed_by_value Catches large types passed by value, which forces a memcpy at every call. The single existing hit is `HyperLogLog::new_with_registers`, whose 16 KiB array is moved into the returned struct, so a reference would only add a copy. It gets `#[expect]`. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/functions-aggregate/src/hyperloglog.rs | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 0e1ad685a3d0c..24c7cb7301ac4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -251,6 +251,7 @@ large_include_file = "warn" # Like `large_futures`, these guard against stack overflows large_stack_arrays = "warn" large_stack_frames = "warn" +large_types_passed_by_value = "warn" linkedlist = "warn" # Catches `"{foo}"` where the string is never actually formatted literal_string_with_formatting_args = "warn" diff --git a/datafusion/functions-aggregate/src/hyperloglog.rs b/datafusion/functions-aggregate/src/hyperloglog.rs index 9968e5a98194f..ce374c3106160 100644 --- a/datafusion/functions-aggregate/src/hyperloglog.rs +++ b/datafusion/functions-aggregate/src/hyperloglog.rs @@ -79,6 +79,10 @@ where /// Creates a HyperLogLog from already populated registers /// note that this method should not be invoked in untrusted environment /// because the internal structure of registers are not examined. + #[expect( + clippy::large_types_passed_by_value, + reason = "The registers are moved into the returned `Self`, so taking a reference would only add a copy" + )] pub(crate) fn new_with_registers(registers: [u8; NUM_REGISTERS]) -> Self { Self { registers, From 19eaa185081d2835fabd536afde27f78a18006e1 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 10:00:03 +0200 Subject: [PATCH 11/42] feat: enable clippy::unnecessary_box_returns Catches `-> Box` where the caller gains nothing from the indirection. The single existing hit is a test helper that both takes and returns `Box` so it can hand back the same allocation, so it gets `#[expect]`. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/optimizer/src/analyzer/type_coercion.rs | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 24c7cb7301ac4..5786d139b2a2b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -301,6 +301,7 @@ transmute_ptr_to_ptr = "warn" unchecked_time_subtraction = "warn" uninhabited_references = "warn" uninlined_format_args = "warn" +unnecessary_box_returns = "warn" unnecessary_lazy_evaluations = "warn" unnecessary_safety_comment = "warn" unnecessary_safety_doc = "warn" diff --git a/datafusion/optimizer/src/analyzer/type_coercion.rs b/datafusion/optimizer/src/analyzer/type_coercion.rs index d11c3e7435fde..ca1527fac8078 100644 --- a/datafusion/optimizer/src/analyzer/type_coercion.rs +++ b/datafusion/optimizer/src/analyzer/type_coercion.rs @@ -2660,6 +2660,10 @@ mod test { ) } + #[expect( + clippy::unnecessary_box_returns, + reason = "`Case` stores boxed expressions, so returning the box reuses the allocation" + )] fn cast_if_not_same_type( expr: Box, data_type: &DataType, From 32dd685f3adce228b233ca2d22932643fc892e65 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 10:01:05 +0200 Subject: [PATCH 12/42] feat: enable clippy::option_as_ref_cloned `opt.as_ref().cloned()` is just `opt.clone()` with two extra steps. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/datasource/src/sink.rs | 2 +- datafusion/pruning/src/pruning_predicate.rs | 8 ++++---- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 5786d139b2a2b..423a862de91d7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -273,6 +273,7 @@ negative_feature_names = "warn" non_std_lazy_statics = "warn" non_zero_suggestions = "warn" nonstandard_macro_braces = "warn" +option_as_ref_cloned = "warn" or_fun_call = "warn" path_buf_push_overwrite = "warn" pathbuf_init_then_push = "warn" diff --git a/datafusion/datasource/src/sink.rs b/datafusion/datasource/src/sink.rs index 4bf04133b7843..1557e41531ab7 100644 --- a/datafusion/datasource/src/sink.rs +++ b/datafusion/datasource/src/sink.rs @@ -290,7 +290,7 @@ impl ExecutionPlan for DataSinkExec { fn required_input_ordering(&self) -> Vec> { // The required input ordering is set externally (e.g. by a `ListingTable`). // Otherwise, there is no specific requirement (i.e. `sort_order` is `None`). - vec![self.sort_order.as_ref().cloned().map(Into::into)] + vec![self.sort_order.clone().map(Into::into)] } fn maintains_input_order(&self) -> Vec { diff --git a/datafusion/pruning/src/pruning_predicate.rs b/datafusion/pruning/src/pruning_predicate.rs index 3a63451495e4c..b292861f8f43c 100644 --- a/datafusion/pruning/src/pruning_predicate.rs +++ b/datafusion/pruning/src/pruning_predicate.rs @@ -2304,10 +2304,10 @@ mod tests { .map(|(_values, contained)| Arc::new(contained.clone()) as ArrayRef); [ - self.min.as_ref().cloned(), - self.max.as_ref().cloned(), - self.null_counts.as_ref().cloned(), - self.row_counts.as_ref().cloned(), + self.min.clone(), + self.max.clone(), + self.null_counts.clone(), + self.row_counts.clone(), ] .into_iter() .flatten() From a31db08c512ee17a74375e36d8a1d4e25fe767c5 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 10:12:43 +0200 Subject: [PATCH 13/42] feat: enable clippy::iter_on_single_items `[x].into_iter()` builds and iterates a one-element array where `std::iter::once(x)` is a purpose-built iterator that optimizes better. Most of the existing hits are in tests, so the win there is small; the point is to keep the pattern out of hot code. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/core/src/dataframe/parquet.rs | 3 +-- datafusion/core/tests/sql/joins.rs | 6 ++---- .../tests/user_defined/user_defined_aggregates.rs | 12 ++++-------- .../user_defined/user_defined_scalar_functions.rs | 12 ++++-------- .../user_defined/user_defined_window_functions.rs | 6 ++---- datafusion/expr/src/logical_plan/builder.rs | 8 ++------ datafusion/ffi/src/udaf/mod.rs | 4 +--- datafusion/functions/src/core/union_tag.rs | 6 +++--- datafusion/optimizer/src/optimizer.rs | 2 +- 10 files changed, 21 insertions(+), 39 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 423a862de91d7..1cde48b85df86 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -244,6 +244,7 @@ iter_filter_is_ok = "warn" iter_filter_is_some = "warn" iter_not_returning_iterator = "warn" iter_on_empty_collections = "warn" +iter_on_single_items = "warn" iter_without_into_iter = "warn" # Detects large stack-allocated futures that may cause stack overflow crashes (see threshold in clippy.toml) large_futures = "warn" diff --git a/datafusion/core/src/dataframe/parquet.rs b/datafusion/core/src/dataframe/parquet.rs index 83ffbb151773b..1685dff23dff1 100644 --- a/datafusion/core/src/dataframe/parquet.rs +++ b/datafusion/core/src/dataframe/parquet.rs @@ -221,8 +221,7 @@ mod tests { // relative to datafusion.execution.batch_size does not panic let ctx = SessionContext::new_with_config(SessionConfig::from_string_hash_map( &HashMap::from_iter( - [("datafusion.execution.batch_size", "10")] - .iter() + std::iter::once(&("datafusion.execution.batch_size", "10")) .map(|(s1, s2)| ((*s1).to_string(), (*s2).to_string())), ), )?); diff --git a/datafusion/core/tests/sql/joins.rs b/datafusion/core/tests/sql/joins.rs index 7c0e89ee96418..1afa6af74bedf 100644 --- a/datafusion/core/tests/sql/joins.rs +++ b/datafusion/core/tests/sql/joins.rs @@ -39,8 +39,7 @@ async fn join_change_in_planner() -> Result<()> { ])); // Specify the ordering: let file_sort_order = vec![ - [col("a1")] - .into_iter() + std::iter::once(col("a1")) .map(|e| { let ascending = true; let nulls_first = false; @@ -98,8 +97,7 @@ async fn join_no_order_on_filter() -> Result<()> { ])); // Specify the ordering: let file_sort_order = vec![ - [col("a1")] - .into_iter() + std::iter::once(col("a1")) .map(|e| { let ascending = true; let nulls_first = false; diff --git a/datafusion/core/tests/user_defined/user_defined_aggregates.rs b/datafusion/core/tests/user_defined/user_defined_aggregates.rs index 323925bcfaf82..d035fa25e1d41 100644 --- a/datafusion/core/tests/user_defined/user_defined_aggregates.rs +++ b/datafusion/core/tests/user_defined/user_defined_aggregates.rs @@ -1019,8 +1019,7 @@ async fn test_metadata_based_aggregate() -> Result<()> { let schema = Arc::new(Schema::new(vec![ Field::new("no_metadata", DataType::UInt64, true), Field::new("with_metadata", DataType::UInt64, true).with_metadata( - [("modify_values".to_string(), "double_output".to_string())] - .into_iter() + std::iter::once(("modify_values".to_string(), "double_output".to_string())) .collect(), ), ])); @@ -1037,8 +1036,7 @@ async fn test_metadata_based_aggregate() -> Result<()> { let no_output_meta_udf = AggregateUDF::from(MetadataBasedAggregateUdf::new(HashMap::new())); let with_output_meta_udf = AggregateUDF::from(MetadataBasedAggregateUdf::new( - [("output_metatype".to_string(), "custom_value".to_string())] - .into_iter() + std::iter::once(("output_metatype".to_string(), "custom_value".to_string())) .collect(), )); @@ -1094,8 +1092,7 @@ async fn test_metadata_based_aggregate_as_window() -> Result<()> { let schema = Arc::new(Schema::new(vec![ Field::new("no_metadata", DataType::UInt64, true), Field::new("with_metadata", DataType::UInt64, true).with_metadata( - [("modify_values".to_string(), "double_output".to_string())] - .into_iter() + std::iter::once(("modify_values".to_string(), "double_output".to_string())) .collect(), ), ])); @@ -1114,8 +1111,7 @@ async fn test_metadata_based_aggregate_as_window() -> Result<()> { )); let with_output_meta_udf = Arc::new(AggregateUDF::from(MetadataBasedAggregateUdf::new( - [("output_metatype".to_string(), "custom_value".to_string())] - .into_iter() + std::iter::once(("output_metatype".to_string(), "custom_value".to_string())) .collect(), ))); diff --git a/datafusion/core/tests/user_defined/user_defined_scalar_functions.rs b/datafusion/core/tests/user_defined/user_defined_scalar_functions.rs index b758aeb5209e8..0ebf00d065c54 100644 --- a/datafusion/core/tests/user_defined/user_defined_scalar_functions.rs +++ b/datafusion/core/tests/user_defined/user_defined_scalar_functions.rs @@ -1646,8 +1646,7 @@ async fn test_metadata_based_udf() -> Result<()> { let schema = Arc::new(Schema::new(vec![ Field::new("no_metadata", DataType::UInt64, true), Field::new("with_metadata", DataType::UInt64, true).with_metadata( - [("modify_values".to_string(), "double_output".to_string())] - .into_iter() + std::iter::once(("modify_values".to_string(), "double_output".to_string())) .collect(), ), ])); @@ -1661,8 +1660,7 @@ async fn test_metadata_based_udf() -> Result<()> { let t = ctx.table("t").await?; let no_output_meta_udf = ScalarUDF::from(MetadataBasedUdf::new(HashMap::new())); let with_output_meta_udf = ScalarUDF::from(MetadataBasedUdf::new( - [("output_metatype".to_string(), "custom_value".to_string())] - .into_iter() + std::iter::once(("output_metatype".to_string(), "custom_value".to_string())) .collect(), )); @@ -1716,8 +1714,7 @@ async fn test_metadata_based_udf() -> Result<()> { async fn test_metadata_based_udf_with_literal() -> Result<()> { let ctx = SessionContext::new(); let input_metadata: HashMap = - [("modify_values".to_string(), "double_output".to_string())] - .into_iter() + std::iter::once(("modify_values".to_string(), "double_output".to_string())) .collect(); let input_metadata = FieldMetadata::from(input_metadata); let df = ctx.sql("select 0;").await?.select(vec![ @@ -1728,8 +1725,7 @@ async fn test_metadata_based_udf_with_literal() -> Result<()> { ])?; let output_metadata: HashMap = - [("output_metatype".to_string(), "custom_value".to_string())] - .into_iter() + std::iter::once(("output_metatype".to_string(), "custom_value".to_string())) .collect(); let custom_udf = ScalarUDF::from(MetadataBasedUdf::new(output_metadata.clone())); diff --git a/datafusion/core/tests/user_defined/user_defined_window_functions.rs b/datafusion/core/tests/user_defined/user_defined_window_functions.rs index afaf269ca1200..fc14b9a1c6bfe 100644 --- a/datafusion/core/tests/user_defined/user_defined_window_functions.rs +++ b/datafusion/core/tests/user_defined/user_defined_window_functions.rs @@ -869,8 +869,7 @@ async fn test_metadata_based_window_fn() -> Result<()> { let schema = Arc::new(Schema::new(vec![ Field::new("no_metadata", DataType::UInt64, true), Field::new("with_metadata", DataType::UInt64, true).with_metadata( - [("modify_values".to_string(), "double_output".to_string())] - .into_iter() + std::iter::once(("modify_values".to_string(), "double_output".to_string())) .collect(), ), ])); @@ -886,8 +885,7 @@ async fn test_metadata_based_window_fn() -> Result<()> { let no_output_meta_udf = WindowUDF::from(MetadataBasedWindowUdf::new(HashMap::new())); let with_output_meta_udf = WindowUDF::from(MetadataBasedWindowUdf::new( - [("output_metatype".to_string(), "custom_value".to_string())] - .into_iter() + std::iter::once(("output_metatype".to_string(), "custom_value".to_string())) .collect(), )); diff --git a/datafusion/expr/src/logical_plan/builder.rs b/datafusion/expr/src/logical_plan/builder.rs index 4b728fe4474bc..ef5e496b0d7de 100644 --- a/datafusion/expr/src/logical_plan/builder.rs +++ b/datafusion/expr/src/logical_plan/builder.rs @@ -2972,9 +2972,7 @@ mod tests { #[test] fn test_values_metadata() -> Result<()> { let metadata: HashMap = - [("ARROW:extension:metadata".to_string(), "test".to_string())] - .into_iter() - .collect(); + once(("ARROW:extension:metadata".to_string(), "test".to_string())).collect(); let metadata = FieldMetadata::from(metadata); let values = LogicalPlanBuilder::values(vec![ vec![lit_with_metadata(1, Some(metadata.clone()))], @@ -2985,9 +2983,7 @@ mod tests { // Do not allow VALUES with different metadata mixed together let metadata2: HashMap = - [("ARROW:extension:metadata".to_string(), "test2".to_string())] - .into_iter() - .collect(); + once(("ARROW:extension:metadata".to_string(), "test2".to_string())).collect(); let metadata2 = FieldMetadata::from(metadata2); assert!( LogicalPlanBuilder::values(vec![ diff --git a/datafusion/ffi/src/udaf/mod.rs b/datafusion/ffi/src/udaf/mod.rs index b3a087e5d0022..28d2221ac188c 100644 --- a/datafusion/ffi/src/udaf/mod.rs +++ b/datafusion/ffi/src/udaf/mod.rs @@ -777,9 +777,7 @@ mod tests { let foreign_udaf = AggregateUDF::new_from_shared_impl(foreign_udaf); let metadata: HashMap = - [("a_key".to_string(), "a_value".to_string())] - .into_iter() - .collect(); + std::iter::once(("a_key".to_string(), "a_value".to_string())).collect(); let input_field = Arc::new( Field::new("a", DataType::Float64, false).with_metadata(metadata.clone()), ); diff --git a/datafusion/functions/src/core/union_tag.rs b/datafusion/functions/src/core/union_tag.rs index 9a349a4b9a8eb..7c36f6bf0b6f1 100644 --- a/datafusion/functions/src/core/union_tag.rs +++ b/datafusion/functions/src/core/union_tag.rs @@ -160,9 +160,9 @@ mod tests { // when it becomes possible to construct union scalars in SQL, this should go to sqllogictests #[test] fn union_scalar() { - let fields = [(0, Arc::new(Field::new("a", DataType::UInt32, false)))] - .into_iter() - .collect(); + let fields = + std::iter::once((0, Arc::new(Field::new("a", DataType::UInt32, false)))) + .collect(); let scalar = ScalarValue::Union( Some((0, Box::new(ScalarValue::UInt32(Some(0))))), diff --git a/datafusion/optimizer/src/optimizer.rs b/datafusion/optimizer/src/optimizer.rs index db7ad8475273a..d784f296500d0 100644 --- a/datafusion/optimizer/src/optimizer.rs +++ b/datafusion/optimizer/src/optimizer.rs @@ -963,7 +963,7 @@ mod tests { .enumerate() .map(|(i, (qualifier, field))| { let metadata = - [("key".into(), format!("value {i}"))].into_iter().collect(); + std::iter::once(("key".into(), format!("value {i}"))).collect(); let new_arrow_field = field.as_ref().clone().with_metadata(metadata); (qualifier.cloned(), Arc::new(new_arrow_field)) From 72b745674a94e35bb0e690b26783cc12fa10f0da Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 10:45:18 +0200 Subject: [PATCH 14/42] feat: enable clippy::string_lit_as_bytes `"foo".as_bytes()` goes through a UTF-8 str; `b"foo"` is already the byte literal. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion-cli/src/helper.rs | 6 ++---- .../examples/proto/composed_extension_codec.rs | 8 ++++---- datafusion/core/tests/sql/joins.rs | 2 +- datafusion/datasource-avro/src/source.rs | 12 ++++++------ datafusion/functions/src/string/concat.rs | 14 ++++---------- datafusion/proto/tests/cases/plans/sources.rs | 4 ++-- .../tests/cases/roundtrip_logical_plan.rs | 2 +- 8 files changed, 21 insertions(+), 28 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 1cde48b85df86..674d7ab59315a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -294,6 +294,7 @@ self_only_used_in_recursion = "warn" set_contains_or_insert = "warn" str_split_at_newline = "warn" string_add_assign = "warn" +string_lit_as_bytes = "warn" string_lit_chars_any = "warn" suspicious_xor_used_as_pow = "warn" trailing_empty_array = "warn" diff --git a/datafusion-cli/src/helper.rs b/datafusion-cli/src/helper.rs index 67e203cf7987b..db3e80efad8ac 100644 --- a/datafusion-cli/src/helper.rs +++ b/datafusion-cli/src/helper.rs @@ -308,16 +308,14 @@ mod tests { let mut validator = CliHelper::default(); // should be invalid in generic dialect - let result = - readline_direct(Cursor::new(r"select 1 # 2;".as_bytes()), &validator)?; + let result = readline_direct(Cursor::new(br"select 1 # 2;"), &validator)?; assert!( matches!(result, ValidationResult::Invalid(Some(e)) if e.contains("Invalid statement")) ); // valid in postgresql dialect validator.set_dialect(&Dialect::PostgreSQL); - let result = - readline_direct(Cursor::new(r"select 1 # 2;".as_bytes()), &validator)?; + let result = readline_direct(Cursor::new(br"select 1 # 2;"), &validator)?; assert!(matches!(result, ValidationResult::Valid(None))); Ok(()) diff --git a/datafusion-examples/examples/proto/composed_extension_codec.rs b/datafusion-examples/examples/proto/composed_extension_codec.rs index 51c1bc7c5518b..d3bcdc73df7f8 100644 --- a/datafusion-examples/examples/proto/composed_extension_codec.rs +++ b/datafusion-examples/examples/proto/composed_extension_codec.rs @@ -160,7 +160,7 @@ impl PhysicalExtensionCodec for ParentPhysicalExtensionCodec { _ctx: &TaskContext, _proto_converter: &dyn PhysicalProtoConverterExtension, ) -> Result> { - if buf == "ParentExec".as_bytes() { + if buf == b"ParentExec" { Ok(Arc::new(ParentExec { input: inputs[0].clone(), })) @@ -176,7 +176,7 @@ impl PhysicalExtensionCodec for ParentPhysicalExtensionCodec { _proto_converter: &dyn PhysicalProtoConverterExtension, ) -> Result<()> { if node.is::() { - buf.extend_from_slice("ParentExec".as_bytes()); + buf.extend_from_slice(b"ParentExec"); Ok(()) } else { internal_err!("Not supported") @@ -258,7 +258,7 @@ impl PhysicalExtensionCodec for ChildPhysicalExtensionCodec { _ctx: &TaskContext, _proto_converter: &dyn PhysicalProtoConverterExtension, ) -> Result> { - if buf == "ChildExec".as_bytes() { + if buf == b"ChildExec" { Ok(Arc::new(ChildExec {})) } else { internal_err!("Not supported") @@ -272,7 +272,7 @@ impl PhysicalExtensionCodec for ChildPhysicalExtensionCodec { _proto_converter: &dyn PhysicalProtoConverterExtension, ) -> Result<()> { if node.is::() { - buf.extend_from_slice("ChildExec".as_bytes()); + buf.extend_from_slice(b"ChildExec"); Ok(()) } else { internal_err!("Not supported") diff --git a/datafusion/core/tests/sql/joins.rs b/datafusion/core/tests/sql/joins.rs index 1afa6af74bedf..bb526895b6b12 100644 --- a/datafusion/core/tests/sql/joins.rs +++ b/datafusion/core/tests/sql/joins.rs @@ -223,7 +223,7 @@ async fn join_using_uppercase_column() -> Result<()> { let tmp_dir = TempDir::new()?; let file_path = tmp_dir.path().join("uppercase-column.csv"); let mut file = File::create(file_path.clone())?; - file.write_all("0".as_bytes())?; + file.write_all(b"0")?; drop(file); let ctx = SessionContext::new(); diff --git a/datafusion/datasource-avro/src/source.rs b/datafusion/datasource-avro/src/source.rs index fcc50b559f00b..db98e167abb11 100644 --- a/datafusion/datasource-avro/src/source.rs +++ b/datafusion/datasource-avro/src/source.rs @@ -412,14 +412,14 @@ mod tests { assert_eq!(8, date_string_col.0); assert_eq!(&DataType::Binary, date_string_col.1.data_type()); let col = get_col::(&batch, date_string_col).unwrap(); - assert_eq!("01/01/09".as_bytes(), col.value(0)); - assert_eq!("01/01/09".as_bytes(), col.value(1)); + assert_eq!(b"01/01/09", col.value(0)); + assert_eq!(b"01/01/09", col.value(1)); let string_col = schema.column_with_name("string_col").unwrap(); assert_eq!(9, string_col.0); assert_eq!(&DataType::Binary, string_col.1.data_type()); let col = get_col::(&batch, string_col).unwrap(); - assert_eq!("0".as_bytes(), col.value(0)); - assert_eq!("1".as_bytes(), col.value(1)); + assert_eq!(b"0", col.value(0)); + assert_eq!(b"1", col.value(1)); let timestamp_col = schema.column_with_name("timestamp_col").unwrap(); assert_eq!(10, timestamp_col.0); assert_eq!( @@ -456,8 +456,8 @@ mod tests { .as_any() .downcast_ref::() .unwrap(); - assert_eq!("0".as_bytes(), col.value(0)); - assert_eq!("1".as_bytes(), col.value(1)); + assert_eq!(b"0", col.value(0)); + assert_eq!(b"1", col.value(1)); // Second column should be double_col (was at index 7 in original) assert_eq!("double_col", schema.field(1).name()); diff --git a/datafusion/functions/src/string/concat.rs b/datafusion/functions/src/string/concat.rs index 1c1f6d640798a..aa42d918eb4b7 100644 --- a/datafusion/functions/src/string/concat.rs +++ b/datafusion/functions/src/string/concat.rs @@ -495,7 +495,7 @@ mod tests { ColumnarValue::Scalar(ScalarValue::Binary(Some( "Café".as_bytes().into() ))), - ColumnarValue::Scalar(ScalarValue::Binary(Some("cc".as_bytes().into()))), + ColumnarValue::Scalar(ScalarValue::Binary(Some(b"cc".into()))), ], Ok(Some("Cafécc".as_bytes())), &[u8], @@ -508,9 +508,7 @@ mod tests { ColumnarValue::Scalar(ScalarValue::Binary(Some( "Café".as_bytes().into() ))), - ColumnarValue::Scalar(ScalarValue::LargeBinary(Some( - "cc".as_bytes().into() - ))), + ColumnarValue::Scalar(ScalarValue::LargeBinary(Some(b"cc".into()))), ], Ok(Some("Cafécc".as_bytes())), &[u8], @@ -523,9 +521,7 @@ mod tests { ColumnarValue::Scalar(ScalarValue::Binary(Some( "Café".as_bytes().into() ))), - ColumnarValue::Scalar(ScalarValue::BinaryView(Some( - "cc".as_bytes().into() - ))), + ColumnarValue::Scalar(ScalarValue::BinaryView(Some(b"cc".into()))), ], Ok(Some("Cafécc".as_bytes())), &[u8], @@ -538,9 +534,7 @@ mod tests { ColumnarValue::Scalar(ScalarValue::BinaryView(Some( "Café".as_bytes().into() ))), - ColumnarValue::Scalar(ScalarValue::BinaryView(Some( - "cc".as_bytes().into() - ))), + ColumnarValue::Scalar(ScalarValue::BinaryView(Some(b"cc".into()))), ], Ok(Some("Cafécc".as_bytes())), &[u8], diff --git a/datafusion/proto/tests/cases/plans/sources.rs b/datafusion/proto/tests/cases/plans/sources.rs index 04708dec6439c..e17e8ebfbd128 100644 --- a/datafusion/proto/tests/cases/plans/sources.rs +++ b/datafusion/proto/tests/cases/plans/sources.rs @@ -405,7 +405,7 @@ fn roundtrip_parquet_exec_with_custom_predicate_expr() -> Result<()> { inputs: &[Arc], _ctx: &PhysicalExprDecodeCtx<'_>, ) -> Result> { - if buf == "CustomPredicateExpr".as_bytes() { + if buf == b"CustomPredicateExpr" { Ok(Arc::new(CustomPredicateExpr { inner: inputs[0].clone(), })) @@ -421,7 +421,7 @@ fn roundtrip_parquet_exec_with_custom_predicate_expr() -> Result<()> { _ctx: &PhysicalExprEncodeCtx<'_>, ) -> Result<()> { if node.downcast_ref::().is_some() { - buf.extend_from_slice("CustomPredicateExpr".as_bytes()); + buf.extend_from_slice(b"CustomPredicateExpr"); Ok(()) } else { internal_err!("Not supported") diff --git a/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs b/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs index 747dfeb877d44..6ecad2390238f 100644 --- a/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs +++ b/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs @@ -1683,7 +1683,7 @@ async fn new_test_grammar() -> Result<()> { #[tokio::test] async fn extension_logical_plan() -> Result<()> { let ctx = create_context().await?; - let validation_bytes = "MockUserDefinedLogicalPlan".as_bytes().to_vec(); + let validation_bytes = b"MockUserDefinedLogicalPlan".to_vec(); let ext_plan = LogicalPlan::Extension(Extension { node: Arc::new(MockUserDefinedLogicalPlan { validation_bytes, From 5b80ab121245b8f0f7be6724ed3681a5e8eaa164 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 10:48:23 +0200 Subject: [PATCH 15/42] feat: enable clippy::unnecessary_debug_formatting `{:?}` on a `Path`/`PathBuf` prints it quoted and with escapes, which is not what these user-facing messages want. `{}` on `.display()` is also one less formatting layer. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 2 ++ .../examples/data_io/parquet_advanced_index.rs | 3 ++- .../examples/data_io/parquet_index.rs | 12 ++++++++---- datafusion/proto-common/gen/src/main.rs | 2 +- datafusion/proto-models/gen/src/main.rs | 2 +- datafusion/sqllogictest/bin/sqllogictests.rs | 10 +++++----- datafusion/sqllogictest/src/util.rs | 2 +- 7 files changed, 20 insertions(+), 13 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 674d7ab59315a..284c4cb9e53a3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -305,6 +305,8 @@ unchecked_time_subtraction = "warn" uninhabited_references = "warn" uninlined_format_args = "warn" unnecessary_box_returns = "warn" +# `{:?}` on a `Path` quotes and escapes it; `{}` on `.display()` does not +unnecessary_debug_formatting = "warn" unnecessary_lazy_evaluations = "warn" unnecessary_safety_comment = "warn" unnecessary_safety_doc = "warn" diff --git a/datafusion-examples/examples/data_io/parquet_advanced_index.rs b/datafusion-examples/examples/data_io/parquet_advanced_index.rs index 39423648db715..d12737619d069 100644 --- a/datafusion-examples/examples/data_io/parquet_advanced_index.rs +++ b/datafusion-examples/examples/data_io/parquet_advanced_index.rs @@ -406,7 +406,8 @@ impl IndexedFile { let file_size = path.metadata()?.len(); let file = File::open(path).map_err(|e| { - DataFusionError::from(e).context(format!("Error opening file {path:?}")) + DataFusionError::from(e) + .context(format!("Error opening file {}", path.display())) })?; let options = ArrowReaderOptions::new() diff --git a/datafusion-examples/examples/data_io/parquet_index.rs b/datafusion-examples/examples/data_io/parquet_index.rs index 753d1b30fc0e8..8ca7d1da891ea 100644 --- a/datafusion-examples/examples/data_io/parquet_index.rs +++ b/datafusion-examples/examples/data_io/parquet_index.rs @@ -501,7 +501,8 @@ impl ParquetMetadataIndexBuilder { let file_size = file.metadata()?.len(); let file = File::open(file).map_err(|e| { - DataFusionError::from(e).context(format!("Error opening file {file:?}")) + DataFusionError::from(e) + .context(format!("Error opening file {}", file.display())) })?; let reader = ParquetRecordBatchReaderBuilder::try_new(file)?; @@ -621,12 +622,15 @@ fn read_dir(dir: &Path) -> Result> { let mut files = dir .read_dir() .map_err(|e| { - DataFusionError::from(e).context(format!("Error reading directory {dir:?}")) + DataFusionError::from(e) + .context(format!("Error reading directory {}", dir.display())) })? .map(|entry| { entry.map_err(|e| { - DataFusionError::from(e) - .context(format!("Error reading directory entry in {dir:?}")) + DataFusionError::from(e).context(format!( + "Error reading directory entry in {}", + dir.display() + )) }) }) .collect::>>()?; diff --git a/datafusion/proto-common/gen/src/main.rs b/datafusion/proto-common/gen/src/main.rs index d672832d43897..3623a6357d87a 100644 --- a/datafusion/proto-common/gen/src/main.rs +++ b/datafusion/proto-common/gen/src/main.rs @@ -33,7 +33,7 @@ fn main() -> Result<(), String> { .map_err(|e| format!("protobuf compilation failed: {e}"))?; let descriptor_set = std::fs::read(&descriptor_path) - .unwrap_or_else(|e| panic!("Cannot read {descriptor_path:?}: {e}")); + .unwrap_or_else(|e| panic!("Cannot read {}: {e}", descriptor_path.display())); pbjson_build::Builder::new() .out_dir("src") diff --git a/datafusion/proto-models/gen/src/main.rs b/datafusion/proto-models/gen/src/main.rs index b9cbf81bb11c8..057912227675d 100644 --- a/datafusion/proto-models/gen/src/main.rs +++ b/datafusion/proto-models/gen/src/main.rs @@ -35,7 +35,7 @@ fn main() -> Result<(), String> { .map_err(|e| format!("protobuf compilation failed: {e}"))?; let descriptor_set = std::fs::read(&descriptor_path) - .unwrap_or_else(|e| panic!("Cannot read {descriptor_path:?}: {e}")); + .unwrap_or_else(|e| panic!("Cannot read {}: {e}", descriptor_path.display())); pbjson_build::Builder::new() .out_dir(out_dir) diff --git a/datafusion/sqllogictest/bin/sqllogictests.rs b/datafusion/sqllogictest/bin/sqllogictests.rs index da0beb0c29a28..ae0eee432ab4e 100644 --- a/datafusion/sqllogictest/bin/sqllogictests.rs +++ b/datafusion/sqllogictest/bin/sqllogictests.rs @@ -453,7 +453,7 @@ async fn run_test_file_substrait_round_trip( let pb = mp.add(ProgressBar::new(count)); pb.set_style(mp_style); - pb.set_message(format!("{relative_path:?}")); + pb.set_message(relative_path.display().to_string()); let mut runner = sqllogictest::Runner::new(|| async { Ok(DataFusionSubstraitRoundTrip::new( @@ -512,7 +512,7 @@ async fn run_test_file( let pb = mp.add(ProgressBar::new(count)); pb.set_style(mp_style); - pb.set_message(format!("{relative_path:?}")); + pb.set_message(relative_path.display().to_string()); // If DataFusion configuration has changed during test file runs, errors will be // pushed to this vec. @@ -631,7 +631,7 @@ async fn run_test_file_with_postgres( let pb = mp.add(ProgressBar::new(count)); pb.set_style(mp_style); - pb.set_message(format!("{relative_path:?}")); + pb.set_message(relative_path.display().to_string()); let mut runner = sqllogictest::Runner::new(|| { Postgres::connect_with_tracked_sql( @@ -690,7 +690,7 @@ async fn run_complete_file( let pb = mp.add(ProgressBar::new(count)); pb.set_style(mp_style); - pb.set_message(format!("{relative_path:?}")); + pb.set_message(relative_path.display().to_string()); let config_change_errors = Arc::new(Mutex::new(Vec::new())); let mut runner = sqllogictest::Runner::new(|| async { @@ -746,7 +746,7 @@ async fn run_complete_file_with_postgres( let pb = mp.add(ProgressBar::new(count)); pb.set_style(mp_style); - pb.set_message(format!("{relative_path:?}")); + pb.set_message(relative_path.display().to_string()); let mut runner = sqllogictest::Runner::new(|| { Postgres::connect_with_tracked_sql( diff --git a/datafusion/sqllogictest/src/util.rs b/datafusion/sqllogictest/src/util.rs index b0cf32266ea31..b0713917ef49a 100644 --- a/datafusion/sqllogictest/src/util.rs +++ b/datafusion/sqllogictest/src/util.rs @@ -33,7 +33,7 @@ pub fn setup_scratch_dir(name: &Path) -> Result<()> { let file_stem = name.file_stem().expect("File should have a stem"); let path = PathBuf::from("test_files").join("scratch").join(file_stem); - info!("Creating scratch dir in {path:?}"); + info!("Creating scratch dir in {}", path.display()); if path.exists() { fs::remove_dir_all(&path)?; } From 316ea0240c1d3d2a1a4f4625b5863a3fa38952f9 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 10:52:21 +0200 Subject: [PATCH 16/42] feat: enable clippy::option_option `Option>` usually means a plain `Option` or a small enum would be clearer. All three existing hits use the nesting deliberately (unset vs. set-to-none, null list vs. null element, not-a-literal vs. literal-without-span), so they get `#[expect]`. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/core/tests/datasource/object_store_access.rs | 4 ++++ .../aggregates/group_values/multi_group_by/row_backed.rs | 4 ++++ datafusion/sql/src/expr/mod.rs | 6 ++++++ 4 files changed, 15 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index 284c4cb9e53a3..9e504501063e1 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -275,6 +275,7 @@ non_std_lazy_statics = "warn" non_zero_suggestions = "warn" nonstandard_macro_braces = "warn" option_as_ref_cloned = "warn" +option_option = "warn" or_fun_call = "warn" path_buf_push_overwrite = "warn" pathbuf_init_then_push = "warn" diff --git a/datafusion/core/tests/datasource/object_store_access.rs b/datafusion/core/tests/datasource/object_store_access.rs index 2503de862e06a..2ed97abb02213 100644 --- a/datafusion/core/tests/datasource/object_store_access.rs +++ b/datafusion/core/tests/datasource/object_store_access.rs @@ -929,6 +929,10 @@ struct Test { /// * `None`: uses the default (does not set a size_hint) /// * `Some(None)`L: set prefetch hint to None (prefetching) /// * `Some(Some(size))`: set prefetch hint to size + #[expect( + clippy::option_option, + reason = "The nesting is meaningful, see the doc comment above" + )] parquet_metadata_size_hint: Option>, } diff --git a/datafusion/physical-plan/src/aggregates/group_values/multi_group_by/row_backed.rs b/datafusion/physical-plan/src/aggregates/group_values/multi_group_by/row_backed.rs index 1445a81f2189b..377977029a402 100644 --- a/datafusion/physical-plan/src/aggregates/group_values/multi_group_by/row_backed.rs +++ b/datafusion/physical-plan/src/aggregates/group_values/multi_group_by/row_backed.rs @@ -337,6 +337,10 @@ mod tests { /// marks a null list. Variable-length string payloads give retained rows /// distinct encoded lengths, which is what `take_n`'s byte preallocation /// depends on. + #[expect( + clippy::option_option, + reason = "The outer `None` marks a null list, the inner one a null string" + )] fn fsl_utf8(rows: Vec>>) -> ArrayRef { let child = StringArray::from( rows.iter() diff --git a/datafusion/sql/src/expr/mod.rs b/datafusion/sql/src/expr/mod.rs index b1de4e95fd8a2..22520d86bd753 100644 --- a/datafusion/sql/src/expr/mod.rs +++ b/datafusion/sql/src/expr/mod.rs @@ -57,6 +57,12 @@ mod substring; mod unary_op; mod value; +/// Returns `None` if `expr` is not a NULL literal, and `Some(span)` where +/// `span` is the literal's span, if it has one. +#[expect( + clippy::option_option, + reason = "The two levels mean different things, see above" +)] fn null_value_span(expr: &SQLExpr) -> Option> { if let SQLExpr::Value(ValueWithSpan { value: Value::Null, From e534900764f89ecc58e8ab22672fbe1c7d38a54b Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 10:57:06 +0200 Subject: [PATCH 17/42] feat: enable clippy::single_option_map A function that takes a single `Option` only to `map` over it is easier to reuse if it takes `T` and the caller does the mapping. All five hits are fixed that way rather than suppressed. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion-cli/src/functions.rs | 28 ++++++++----------- .../user_defined_scalar_functions.rs | 15 +++++----- .../src/aggregates/grouped_hash_stream.rs | 7 +++-- .../physical-plan/src/aggregates/mod.rs | 15 ++++++---- .../spark/src/function/bitmap/bitmap_count.rs | 11 +++++--- 6 files changed, 41 insertions(+), 36 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9e504501063e1..ac74e65b547de 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -293,6 +293,7 @@ same_length_and_capacity = "warn" self_only_used_in_recursion = "warn" # Avoids hashing the key twice set_contains_or_insert = "warn" +single_option_map = "warn" str_split_at_newline = "warn" string_add_assign = "warn" string_lit_as_bytes = "warn" diff --git a/datafusion-cli/src/functions.rs b/datafusion-cli/src/functions.rs index 164af2559d2f6..00e1ff80ef774 100644 --- a/datafusion-cli/src/functions.rs +++ b/datafusion-cli/src/functions.rs @@ -283,16 +283,16 @@ fn convert_parquet_statistics( val.max_opt().map(|v| v.to_string()), ), (Statistics::ByteArray(val), ConvertedType::UTF8) => ( - byte_array_to_string(val.min_opt()), - byte_array_to_string(val.max_opt()), + val.min_opt().map(byte_array_to_string), + val.max_opt().map(byte_array_to_string), ), (Statistics::ByteArray(val), _) => ( val.min_opt().map(|v| v.to_string()), val.max_opt().map(|v| v.to_string()), ), (Statistics::FixedLenByteArray(val), ConvertedType::UTF8) => ( - fixed_len_byte_array_to_string(val.min_opt()), - fixed_len_byte_array_to_string(val.max_opt()), + val.min_opt().map(fixed_len_byte_array_to_string), + val.max_opt().map(fixed_len_byte_array_to_string), ), (Statistics::FixedLenByteArray(val), _) => ( val.min_opt().map(|v| v.to_string()), @@ -302,21 +302,17 @@ fn convert_parquet_statistics( } /// Convert to a string if it has utf8 encoding, otherwise print bytes directly -fn byte_array_to_string(val: Option<&ByteArray>) -> Option { - val.map(|v| { - v.as_utf8() - .map(|s| s.to_string()) - .unwrap_or_else(|_e| v.to_string()) - }) +fn byte_array_to_string(val: &ByteArray) -> String { + val.as_utf8() + .map(|s| s.to_string()) + .unwrap_or_else(|_e| val.to_string()) } /// Convert to a string if it has utf8 encoding, otherwise print bytes directly -fn fixed_len_byte_array_to_string(val: Option<&FixedLenByteArray>) -> Option { - val.map(|v| { - v.as_utf8() - .map(|s| s.to_string()) - .unwrap_or_else(|_e| v.to_string()) - }) +fn fixed_len_byte_array_to_string(val: &FixedLenByteArray) -> String { + val.as_utf8() + .map(|s| s.to_string()) + .unwrap_or_else(|_e| val.to_string()) } #[derive(Debug)] diff --git a/datafusion/core/tests/user_defined/user_defined_scalar_functions.rs b/datafusion/core/tests/user_defined/user_defined_scalar_functions.rs index 0ebf00d065c54..59cb336bb4fdb 100644 --- a/datafusion/core/tests/user_defined/user_defined_scalar_functions.rs +++ b/datafusion/core/tests/user_defined/user_defined_scalar_functions.rs @@ -1816,11 +1816,11 @@ impl ScalarUDFImpl for ExtensionBasedUdf { // If we have the extension type set, we are outputting a boolean value. // Otherwise we output a string representation of the numeric value. - fn print_value(v: Option, as_bool: bool) -> Option { - v.map(|x| match as_bool { + fn print_value(x: i8, as_bool: bool) -> String { + match as_bool { true => format!("{}", x != 0), false => format!("{x}"), - }) + } } match &args.args[0] { @@ -1830,7 +1830,7 @@ impl ScalarUDFImpl for ExtensionBasedUdf { .downcast_ref::() .unwrap() .iter() - .map(|v| print_value(v, output_as_bool)) + .map(|v| v.map(|v| print_value(v, output_as_bool))) .collect(); let array_ref = Arc::new(StringArray::from(array_values)) as ArrayRef; Ok(ColumnarValue::Array(array_ref)) @@ -1840,10 +1840,9 @@ impl ScalarUDFImpl for ExtensionBasedUdf { return exec_err!("incorrect data type"); }; - Ok(ColumnarValue::Scalar(ScalarValue::Utf8(print_value( - *value, - output_as_bool, - )))) + Ok(ColumnarValue::Scalar(ScalarValue::Utf8( + value.map(|v| print_value(v, output_as_bool)), + ))) } } } diff --git a/datafusion/physical-plan/src/aggregates/grouped_hash_stream.rs b/datafusion/physical-plan/src/aggregates/grouped_hash_stream.rs index 99c101199459f..78c905d43ecb0 100644 --- a/datafusion/physical-plan/src/aggregates/grouped_hash_stream.rs +++ b/datafusion/physical-plan/src/aggregates/grouped_hash_stream.rs @@ -481,8 +481,11 @@ impl GroupedHashAggregateStream { let agg_fn_names = aggregate_exprs .iter() .map(|expr| { - format_human_display(expr.human_display(), expr.human_display_alias()) - .map(|display| display.into_owned()) + expr.human_display() + .map(|display| { + format_human_display(display, expr.human_display_alias()) + .into_owned() + }) .unwrap_or_else(|| expr.name().to_string()) }) .collect::>() diff --git a/datafusion/physical-plan/src/aggregates/mod.rs b/datafusion/physical-plan/src/aggregates/mod.rs index f9dd90f6f98fe..c82263f98226c 100644 --- a/datafusion/physical-plan/src/aggregates/mod.rs +++ b/datafusion/physical-plan/src/aggregates/mod.rs @@ -1985,25 +1985,28 @@ impl DisplayAs for AggregateExec { fn format_aggregate_exec_expr(agg: &AggregateFunctionExpr) -> Cow<'_, str> { match agg.human_display_alias() { - Some(_) => format_human_display(agg.human_display(), agg.human_display_alias()) + Some(_) => agg + .human_display() + .map(|display| format_human_display(display, agg.human_display_alias())) .unwrap_or_else(|| Cow::Borrowed(agg.name())), None => Cow::Borrowed(agg.name()), } } fn format_tree_aggregate_expr(agg: &AggregateFunctionExpr) -> Cow<'_, str> { - format_human_display(agg.human_display(), agg.human_display_alias()) + agg.human_display() + .map(|display| format_human_display(display, agg.human_display_alias())) .unwrap_or_else(|| Cow::Borrowed(agg.name())) } fn format_human_display<'a>( - human_display: Option<&'a str>, + human_display: &'a str, alias: Option<&'a str>, -) -> Option> { - human_display.map(|human_display| match alias { +) -> Cow<'a, str> { + match alias { Some(alias) => Cow::Owned(format!("{human_display} as {alias}")), None => Cow::Borrowed(human_display), - }) + } } impl ExecutionPlan for AggregateExec { diff --git a/datafusion/spark/src/function/bitmap/bitmap_count.rs b/datafusion/spark/src/function/bitmap/bitmap_count.rs index 18d584868830b..95e6ec220a0cb 100644 --- a/datafusion/spark/src/function/bitmap/bitmap_count.rs +++ b/datafusion/spark/src/function/bitmap/bitmap_count.rs @@ -90,14 +90,17 @@ impl ScalarUDFImpl for BitmapCount { } } -fn binary_count_ones(opt: Option<&[u8]>) -> Option { - opt.map(|value| value.iter().map(|b| b.count_ones() as i64).sum()) +fn binary_count_ones(value: &[u8]) -> i64 { + value.iter().map(|b| b.count_ones() as i64).sum() } macro_rules! downcast_and_count_ones { ($input_array:expr, $array_type:ident) => {{ let arr = downcast_arg!($input_array, $array_type); - Ok(arr.iter().map(binary_count_ones).collect::()) + Ok(arr + .iter() + .map(|v| v.map(binary_count_ones)) + .collect::()) }}; } @@ -107,7 +110,7 @@ macro_rules! downcast_dict_and_count_ones { let array = dict_array.downcast_dict::().unwrap(); Ok(array .into_iter() - .map(binary_count_ones) + .map(|v| v.map(binary_count_ones)) .collect::()) }}; } From 663f66a70240dbecd9342f727531f0c1fbcba18b Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 11:00:13 +0200 Subject: [PATCH 18/42] feat: enable clippy::ignore_without_reason `#[ignore]` with no reason leaves the next reader guessing whether the test is broken, slow, or obsolete. The reasons for the six existing ones were already nearby, in a comment or in the test body, so they move into the attribute. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + .../memory_limit_validation/sort_mem_validation.rs | 4 +--- datafusion/physical-expr/src/expressions/cast.rs | 2 +- datafusion/substrait/tests/cases/consumer_integration.rs | 8 ++++---- 4 files changed, 7 insertions(+), 8 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index ac74e65b547de..1b060013925b7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -233,6 +233,7 @@ flat_map_option = "warn" float_cmp_const = "warn" fn_params_excessive_bools = "warn" fn_to_numeric_cast_any = "warn" +ignore_without_reason = "warn" imprecise_flops = "warn" index_refutable_slice = "warn" inefficient_to_string = "warn" diff --git a/datafusion/core/tests/memory_limit/memory_limit_validation/sort_mem_validation.rs b/datafusion/core/tests/memory_limit/memory_limit_validation/sort_mem_validation.rs index b55a3039ec9d4..766312567ce41 100644 --- a/datafusion/core/tests/memory_limit/memory_limit_validation/sort_mem_validation.rs +++ b/datafusion/core/tests/memory_limit/memory_limit_validation/sort_mem_validation.rs @@ -161,9 +161,7 @@ async fn sort_with_mem_limit_2_cols_1() { .await; } -// TODO: Query fails, fix it -// Issue: https://github.com/apache/datafusion/issues/14143 -#[ignore] +#[ignore = "Query fails, see https://github.com/apache/datafusion/issues/14143"] #[tokio::test] async fn sort_with_mem_limit_2_cols_2() { let memory_usage_in_theory = 80_000_000 * 2; // 2 columns diff --git a/datafusion/physical-expr/src/expressions/cast.rs b/datafusion/physical-expr/src/expressions/cast.rs index dbb91e365af90..cb3103d38c52a 100644 --- a/datafusion/physical-expr/src/expressions/cast.rs +++ b/datafusion/physical-expr/src/expressions/cast.rs @@ -1183,7 +1183,7 @@ mod tests { } #[test] - #[ignore] // TODO: https://github.com/apache/datafusion/issues/5396 + #[ignore = "TODO: https://github.com/apache/datafusion/issues/5396"] fn test_cast_decimal() -> Result<()> { let schema = Schema::new(vec![Field::new("a", Int64, false)]); let a = Int64Array::from(vec![100]); diff --git a/datafusion/substrait/tests/cases/consumer_integration.rs b/datafusion/substrait/tests/cases/consumer_integration.rs index 1f30a753772cb..b06230f71d05d 100644 --- a/datafusion/substrait/tests/cases/consumer_integration.rs +++ b/datafusion/substrait/tests/cases/consumer_integration.rs @@ -214,7 +214,7 @@ mod tests { Ok(()) } - #[ignore] + #[ignore = "Missing support for enum function arguments"] #[tokio::test] async fn tpch_test_07() -> Result<()> { let plan_str = tpch_plan_to_string(7).await?; @@ -222,7 +222,7 @@ mod tests { Ok(()) } - #[ignore] + #[ignore = "Missing support for enum function arguments"] #[tokio::test] async fn tpch_test_08() -> Result<()> { let plan_str = tpch_plan_to_string(8).await?; @@ -230,7 +230,7 @@ mod tests { Ok(()) } - #[ignore] + #[ignore = "Missing support for enum function arguments"] #[tokio::test] async fn tpch_test_09() -> Result<()> { let plan_str = tpch_plan_to_string(9).await?; @@ -352,7 +352,7 @@ mod tests { Ok(()) } - #[ignore] + #[ignore = "Test file is empty"] #[tokio::test] async fn tpch_test_15() -> Result<()> { let plan_str = tpch_plan_to_string(15).await?; From 0f7e1ab8eb0fa3ee1fe23f557af91fc198dd9a90 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 11:06:27 +0200 Subject: [PATCH 19/42] feat: enable clippy::branches_sharing_code Catches code duplicated at the start or end of every branch of an `if`/`else`, which can be hoisted out. All eight hits are hoisted rather than suppressed. Three of them touch behaviour-sensitive code (the sort spill path, the StringView case conversion, and the array-map join probe), so the tests for the affected crates were run: 3495 tests, all passing. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/common/src/scalar/mod.rs | 5 +--- .../expr/src/logical_plan/invariants.rs | 3 +-- datafusion/functions/src/string/common.rs | 3 +-- .../physical-plan/src/joins/array_map.rs | 3 +-- datafusion/physical-plan/src/sorts/sort.rs | 13 +++++------ datafusion/physical-plan/src/windows/mod.rs | 6 ++--- datafusion/sql/src/expr/function.rs | 23 +++++++------------ datafusion/sql/src/utils.rs | 4 +--- 9 files changed, 22 insertions(+), 39 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 1b060013925b7..d4e4030228415 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -217,6 +217,7 @@ zstd = { version = "0.13", default-features = false } allow_attributes = "warn" as_ptr_cast_mut = "warn" assigning_clones = "warn" +branches_sharing_code = "warn" clear_with_drain = "warn" coerce_container_to_any = "warn" debug_assert_with_mut_call = "warn" diff --git a/datafusion/common/src/scalar/mod.rs b/datafusion/common/src/scalar/mod.rs index bbd12d3ff1ae3..4e7d48dd67437 100644 --- a/datafusion/common/src/scalar/mod.rs +++ b/datafusion/common/src/scalar/mod.rs @@ -10335,22 +10335,19 @@ mod tests { const SECS_IN_ONE_DAY: i32 = 86_400; const MICROSECS_IN_ONE_DAY: i64 = 86_400_000_000; for i in 0..vector_size { + let days = rng.random_range(0..5000); if i % 4 == 0 { - let days = rng.random_range(0..5000); // to not break second precision let millis = rng.random_range(0..SECS_IN_ONE_DAY) * 1000; intervals.push(ScalarValue::new_interval_dt(days, millis)); } else if i % 4 == 1 { - let days = rng.random_range(0..5000); let millisec = rng.random_range(0..(MILLISECS_IN_ONE_DAY as i32)); intervals.push(ScalarValue::new_interval_dt(days, millisec)); } else if i % 4 == 2 { - let days = rng.random_range(0..5000); // to not break microsec precision let nanosec = rng.random_range(0..MICROSECS_IN_ONE_DAY) * 1000; intervals.push(ScalarValue::new_interval_mdn(0, days, nanosec)); } else { - let days = rng.random_range(0..5000); let nanosec = rng.random_range(0..NANOSECS_IN_ONE_DAY); intervals.push(ScalarValue::new_interval_mdn(0, days, nanosec)); } diff --git a/datafusion/expr/src/logical_plan/invariants.rs b/datafusion/expr/src/logical_plan/invariants.rs index d6867d1ceb112..d75f9140d795d 100644 --- a/datafusion/expr/src/logical_plan/invariants.rs +++ b/datafusion/expr/src/logical_plan/invariants.rs @@ -221,7 +221,6 @@ pub fn check_subquery_expr( ), }?; } - check_correlations_in_subquery(inner_plan) } else { if let Expr::InSubquery(subquery) = expr { // InSubquery should only return one column @@ -265,8 +264,8 @@ pub fn check_subquery_expr( outer_plan.display() ), }?; - check_correlations_in_subquery(inner_plan) } + check_correlations_in_subquery(inner_plan) } // Recursively check the unsupported outer references in the sub query plan. diff --git a/datafusion/functions/src/string/common.rs b/datafusion/functions/src/string/common.rs index 11ebf7d3d62dd..e37e7ad36278e 100644 --- a/datafusion/functions/src/string/common.rs +++ b/datafusion/functions/src/string/common.rs @@ -624,7 +624,6 @@ fn case_conversion_utf8view_ascii_inner u8>( for b in &mut bytes[4..4 + len] { *b = convert(b); } - new_views.push(u128::from_le_bytes(bytes)); } else { // Long: input view points into shared `data_buffers` we can't // mutate, so copy-convert into our own buffer and rewrite the @@ -676,8 +675,8 @@ fn case_conversion_utf8view_ascii_inner u8>( bytes[4..8].copy_from_slice(&prefix); bytes[8..12].copy_from_slice(&buffer_index.to_le_bytes()); bytes[12..16].copy_from_slice(&new_offset.to_le_bytes()); - new_views.push(u128::from_le_bytes(bytes)); } + new_views.push(u128::from_le_bytes(bytes)); } if !in_progress.is_empty() { diff --git a/datafusion/physical-plan/src/joins/array_map.rs b/datafusion/physical-plan/src/joins/array_map.rs index 4e56cf013c8f7..1a1cd8d32970b 100644 --- a/datafusion/physical-plan/src/joins/array_map.rs +++ b/datafusion/physical-plan/src/joins/array_map.rs @@ -319,7 +319,6 @@ impl ArrayMap { build_indices.push((build_value - 1) as u64); probe_indices.push(prob_idx as u32); } - Ok(None) } else { let mut remaining_output = limit; let to_skip = match current_offset { @@ -376,8 +375,8 @@ impl ArrayMap { return Ok(Some(offset)); } } - Ok(None) } + Ok(None) } pub fn contain_keys(&self, probe_side_keys: &[ArrayRef]) -> Result { diff --git a/datafusion/physical-plan/src/sorts/sort.rs b/datafusion/physical-plan/src/sorts/sort.rs index 6c782f5134484..f4f1a5e5b4e1a 100644 --- a/datafusion/physical-plan/src/sorts/sort.rs +++ b/datafusion/physical-plan/src/sorts/sort.rs @@ -497,14 +497,13 @@ impl ExternalSorter { while let Some(batch) = sorted_stream.next().await { let batch = batch?; let sorted_size = get_reserved_bytes_for_record_batch(&batch)?; - if self.reservation.try_grow(sorted_size).is_err() { - // Although the reservation is not enough, the batch is - // already in memory, so it's okay to combine it with previously - // sorted batches, and spill together. - globally_sorted_batches.push(batch); + let reservation_failed = self.reservation.try_grow(sorted_size).is_err(); + // Even if the reservation is not enough, the batch is already in + // memory, so it's okay to combine it with previously sorted + // batches, and spill together. + globally_sorted_batches.push(batch); + if reservation_failed { self.consume_and_spill_append(&mut globally_sorted_batches)?; // reservation is freed in spill() - } else { - globally_sorted_batches.push(batch); } } diff --git a/datafusion/physical-plan/src/windows/mod.rs b/datafusion/physical-plan/src/windows/mod.rs index 089bdc23ee2c4..b031476b35c0d 100644 --- a/datafusion/physical-plan/src/windows/mod.rs +++ b/datafusion/physical-plan/src/windows/mod.rs @@ -78,15 +78,13 @@ pub fn schema_add_window_field( .map(|f| f.as_ref().clone()) .collect_vec(); // Skip extending schema for UDAF - if let WindowFunctionDefinition::AggregateUDF(_) = window_fn { - Ok(Arc::new(Schema::new(window_fields))) - } else { + if !matches!(window_fn, WindowFunctionDefinition::AggregateUDF(_)) { window_fields.extend_from_slice(&[window_expr_return_field .as_ref() .clone() .with_name(fn_name)]); - Ok(Arc::new(Schema::new(window_fields))) } + Ok(Arc::new(Schema::new(window_fields))) } /// Create a physical expression for window function diff --git a/datafusion/sql/src/expr/function.rs b/datafusion/sql/src/expr/function.rs index e6bee31fbf106..4ad022134aed0 100644 --- a/datafusion/sql/src/expr/function.rs +++ b/datafusion/sql/src/expr/function.rs @@ -48,24 +48,17 @@ pub fn suggest_valid_function( is_window_func: bool, ctx: &dyn ContextProvider, ) -> Option { - let valid_funcs = if is_window_func { + let mut valid_funcs = Vec::new(); + if is_window_func { // All aggregate functions and builtin window functions - let mut funcs = Vec::new(); - - funcs.extend(ctx.udaf_names()); - funcs.extend(ctx.udwf_names()); - - funcs + valid_funcs.extend(ctx.udaf_names()); + valid_funcs.extend(ctx.udwf_names()); } else { // All scalar functions and aggregate functions - let mut funcs = Vec::new(); - - funcs.extend(ctx.udf_names()); - funcs.extend(ctx.higher_order_function_names()); - funcs.extend(ctx.udaf_names()); - - funcs - }; + valid_funcs.extend(ctx.udf_names()); + valid_funcs.extend(ctx.higher_order_function_names()); + valid_funcs.extend(ctx.udaf_names()); + } find_closest_match(valid_funcs, input_function_name) } diff --git a/datafusion/sql/src/utils.rs b/datafusion/sql/src/utils.rs index 3b571eed279dd..0f0b5fe44c77e 100644 --- a/datafusion/sql/src/utils.rs +++ b/datafusion/sql/src/utils.rs @@ -563,12 +563,10 @@ impl TreeNodeRewriter for RecursiveUnnestRewriter<'_> { if self.top_most_unnest.is_none() { self.top_most_unnest = Some(unnest_expr.clone()); } - - Ok(Transformed::no(expr)) } else { self.consecutive_unnest.push(None); - Ok(Transformed::no(expr)) } + Ok(Transformed::no(expr)) } /// The rewriting only happens when the traversal has reached the top-most unnest expr From 20fc834fdea62fe3129ccac31fa6a3e3450f6578 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 11:12:57 +0200 Subject: [PATCH 20/42] feat: enable clippy::needless_type_cast Catches a binding whose type annotation forces a cast that a better annotation would remove. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/core/tests/sql/aggregates/basic.rs | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index d4e4030228415..43bb402148862 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -271,6 +271,7 @@ mismatching_type_param_order = "warn" mut_mut = "warn" # https://github.com/apache/datafusion/issues/18503 needless_pass_by_value = "warn" +needless_type_cast = "warn" negative_feature_names = "warn" # Prefer `std::sync::LazyLock` over the `lazy_static`/`once_cell` crates non_std_lazy_statics = "warn" diff --git a/datafusion/core/tests/sql/aggregates/basic.rs b/datafusion/core/tests/sql/aggregates/basic.rs index 3e5dc6a0b1872..0cf40a9172a7e 100644 --- a/datafusion/core/tests/sql/aggregates/basic.rs +++ b/datafusion/core/tests/sql/aggregates/basic.rs @@ -401,8 +401,8 @@ async fn count_distinct_dictionary_all_null_values() -> Result<()> { /// Test COUNT(DISTINCT) with mixed null and non-null dictionary values #[tokio::test] async fn count_distinct_dictionary_mixed_values() -> Result<()> { - let n: usize = 6; - let num = Arc::new(Int32Array::from_iter(0..n as i32)) as ArrayRef; + let n: i32 = 6; + let num = Arc::new(Int32Array::from_iter(0..n)) as ArrayRef; // Dictionary values array with nulls and non-nulls let dict_values = StringArray::from(vec![None, Some("abc"), Some("def"), None]); From c4bbf00126cb952b166b463aecf8c3fae1dcbadf Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 11:14:03 +0200 Subject: [PATCH 21/42] feat: enable clippy::checked_conversions `n <= i32::MAX as usize` restates what `i32::try_from(n).is_ok()` says directly, and the cast form is easy to get wrong for signed types. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/functions/src/unicode/lpad.rs | 2 +- datafusion/functions/src/unicode/rpad.rs | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 43bb402148862..b9a3479cc63ea 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -218,6 +218,7 @@ allow_attributes = "warn" as_ptr_cast_mut = "warn" assigning_clones = "warn" branches_sharing_code = "warn" +checked_conversions = "warn" clear_with_drain = "warn" coerce_container_to_any = "warn" debug_assert_with_mut_call = "warn" diff --git a/datafusion/functions/src/unicode/lpad.rs b/datafusion/functions/src/unicode/lpad.rs index 40bffeecf422a..0ffd02714957c 100644 --- a/datafusion/functions/src/unicode/lpad.rs +++ b/datafusion/functions/src/unicode/lpad.rs @@ -118,7 +118,7 @@ impl ScalarUDFImpl for LPadFunc { // fast path. if let Some(target_len) = try_as_scalar_i64(&args[1]) { let target_len: usize = match usize::try_from(target_len) { - Ok(n) if n <= i32::MAX as usize => n, + Ok(n) if i32::try_from(n).is_ok() => n, Ok(n) => { return exec_err!( "lpad requested length {n} too large, maximum allowed length is {}", diff --git a/datafusion/functions/src/unicode/rpad.rs b/datafusion/functions/src/unicode/rpad.rs index 784a2037cfbe1..d89ebd0057f3c 100644 --- a/datafusion/functions/src/unicode/rpad.rs +++ b/datafusion/functions/src/unicode/rpad.rs @@ -118,7 +118,7 @@ impl ScalarUDFImpl for RPadFunc { // scalar fast path. if let Some(target_len) = try_as_scalar_i64(&args[1]) { let target_len: usize = match usize::try_from(target_len) { - Ok(n) if n <= i32::MAX as usize => n, + Ok(n) if i32::try_from(n).is_ok() => n, Ok(n) => { return exec_err!( "rpad requested length {n} too large, maximum allowed length is {}", From 2758248e5e59f14bb9de21657e1a70598424439c Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 11:16:15 +0200 Subject: [PATCH 22/42] feat: enable clippy::filter_map_next `.filter_map(f).next()` is `.find_map(f)`, which stops at the first hit without building the intermediate adapter. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/expr/src/expr_schema.rs | 83 ++++++++--------- .../physical-expr/src/expressions/case.rs | 93 +++++++++---------- 3 files changed, 84 insertions(+), 93 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index b9a3479cc63ea..a607504a3ac14 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -231,6 +231,7 @@ exit = "warn" expl_impl_clone_on_copy = "warn" # Catches `From` impls that can panic fallible_impl_from = "warn" +filter_map_next = "warn" flat_map_option = "warn" float_cmp_const = "warn" fn_params_excessive_bools = "warn" diff --git a/datafusion/expr/src/expr_schema.rs b/datafusion/expr/src/expr_schema.rs index 8927fcf4d0bbe..04715a911a99d 100644 --- a/datafusion/expr/src/expr_schema.rs +++ b/datafusion/expr/src/expr_schema.rs @@ -284,56 +284,51 @@ impl ExprSchemable for Expr { Expr::OuterReferenceColumn(field, _) => Ok(field.is_nullable()), Expr::Literal(value, _) => Ok(value.is_null()), Expr::Case(case) => { - let nullable_then = case - .when_then_expr - .iter() - .filter_map(|(w, t)| { - let is_nullable = match t.nullable(input_schema) { - Err(e) => return Some(Err(e)), - Ok(n) => n, - }; - - // Branches with a then expression that is not nullable do not impact the - // nullability of the case expression. - if !is_nullable { - return None; - } - - // For case-with-expression assume all 'then' expressions are reachable - if case.expr.is_some() { - return Some(Ok(())); - } + let nullable_then = case.when_then_expr.iter().find_map(|(w, t)| { + let is_nullable = match t.nullable(input_schema) { + Err(e) => return Some(Err(e)), + Ok(n) => n, + }; + + // Branches with a then expression that is not nullable do not impact the + // nullability of the case expression. + if !is_nullable { + return None; + } - // For branches with a nullable 'then' expression, try to determine - // if the 'then' expression is ever reachable in the situation where - // it would evaluate to null. - let bounds = match predicate_bounds::evaluate_bounds( - w, - Some(unwrap_certainly_null_expr(t)), - input_schema, - ) { - Err(e) => return Some(Err(e)), - Ok(b) => b, - }; + // For case-with-expression assume all 'then' expressions are reachable + if case.expr.is_some() { + return Some(Ok(())); + } - let can_be_true = match bounds - .contains_value(ScalarValue::Boolean(Some(true))) - { + // For branches with a nullable 'then' expression, try to determine + // if the 'then' expression is ever reachable in the situation where + // it would evaluate to null. + let bounds = match predicate_bounds::evaluate_bounds( + w, + Some(unwrap_certainly_null_expr(t)), + input_schema, + ) { + Err(e) => return Some(Err(e)), + Ok(b) => b, + }; + + let can_be_true = + match bounds.contains_value(ScalarValue::Boolean(Some(true))) { Err(e) => return Some(Err(e)), Ok(b) => b, }; - if !can_be_true { - // If the derived 'when' expression can never evaluate to true, the - // 'then' expression is not reachable when it would evaluate to NULL. - // The most common pattern for this is `WHEN x IS NOT NULL THEN x`. - None - } else { - // The branch might be taken - Some(Ok(())) - } - }) - .next(); + if !can_be_true { + // If the derived 'when' expression can never evaluate to true, the + // 'then' expression is not reachable when it would evaluate to NULL. + // The most common pattern for this is `WHEN x IS NOT NULL THEN x`. + None + } else { + // The branch might be taken + Some(Ok(())) + } + }); if let Some(nullable_then) = nullable_then { // There is at least one reachable nullable 'then' expression, so the case diff --git a/datafusion/physical-expr/src/expressions/case.rs b/datafusion/physical-expr/src/expressions/case.rs index 17288a9737699..ae0812e0fd827 100644 --- a/datafusion/physical-expr/src/expressions/case.rs +++ b/datafusion/physical-expr/src/expressions/case.rs @@ -1253,57 +1253,52 @@ impl PhysicalExpr for CaseExpr { } fn nullable(&self, input_schema: &Schema) -> Result { - let nullable_then = self - .body - .when_then_expr - .iter() - .filter_map(|(w, t)| { - let is_nullable = match t.nullable(input_schema) { - // Pass on error determining nullability verbatim - Err(e) => return Some(Err(e)), - Ok(n) => n, - }; - - // Branches with a then expression that is not nullable do not impact the - // nullability of the case expression. - if !is_nullable { - return None; - } - - // For case-with-expression assume all 'then' expressions are reachable - if self.body.expr.is_some() { - return Some(Ok(())); - } - - // For branches with a nullable 'then' expression, try to determine - // if the 'then' expression is ever reachable in the situation where - // it would evaluate to null. - - // Replace the `then` expression with `NULL` in the `when` expression - let with_null = match replace_with_null( - w, - unwrap_certainly_null_expr(t.as_ref()), - input_schema, - ) { - Err(e) => return Some(Err(e)), - Ok(e) => e, - }; + let nullable_then = self.body.when_then_expr.iter().find_map(|(w, t)| { + let is_nullable = match t.nullable(input_schema) { + // Pass on error determining nullability verbatim + Err(e) => return Some(Err(e)), + Ok(n) => n, + }; + + // Branches with a then expression that is not nullable do not impact the + // nullability of the case expression. + if !is_nullable { + return None; + } - // Try to const evaluate the modified `when` expression. - let predicate_result = match evaluate_predicate(&with_null) { - Err(e) => return Some(Err(e)), - Ok(b) => b, - }; + // For case-with-expression assume all 'then' expressions are reachable + if self.body.expr.is_some() { + return Some(Ok(())); + } - match predicate_result { - // Evaluation was inconclusive or true, so the 'then' expression is reachable - None | Some(true) => Some(Ok(())), - // Evaluation proves the branch will never be taken. - // The most common pattern for this is `WHEN x IS NOT NULL THEN x`. - Some(false) => None, - } - }) - .next(); + // For branches with a nullable 'then' expression, try to determine + // if the 'then' expression is ever reachable in the situation where + // it would evaluate to null. + + // Replace the `then` expression with `NULL` in the `when` expression + let with_null = match replace_with_null( + w, + unwrap_certainly_null_expr(t.as_ref()), + input_schema, + ) { + Err(e) => return Some(Err(e)), + Ok(e) => e, + }; + + // Try to const evaluate the modified `when` expression. + let predicate_result = match evaluate_predicate(&with_null) { + Err(e) => return Some(Err(e)), + Ok(b) => b, + }; + + match predicate_result { + // Evaluation was inconclusive or true, so the 'then' expression is reachable + None | Some(true) => Some(Ok(())), + // Evaluation proves the branch will never be taken. + // The most common pattern for this is `WHEN x IS NOT NULL THEN x`. + Some(false) => None, + } + }); if let Some(nullable_then) = nullable_then { // There is at least one reachable nullable 'then' expression, so the case From fddce2011ecab52053ade2ea6faf2d36aaaeec74 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 11:17:12 +0200 Subject: [PATCH 23/42] feat: enable clippy::large_digit_groups Enforces uniform digit grouping in long literals, where an odd group is usually a typo. The three existing hits are deliberate: the grouping spells out the decimal scale, so `180_00000000` reads as 180 with scale 8. They get `#[expect]`. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + .../src/aggregate/avg_distinct/decimal.rs | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/Cargo.toml b/Cargo.toml index a607504a3ac14..77ea246da0c6d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -250,6 +250,7 @@ iter_not_returning_iterator = "warn" iter_on_empty_collections = "warn" iter_on_single_items = "warn" iter_without_into_iter = "warn" +large_digit_groups = "warn" # Detects large stack-allocated futures that may cause stack overflow crashes (see threshold in clippy.toml) large_futures = "warn" large_include_file = "warn" diff --git a/datafusion/functions-aggregate-common/src/aggregate/avg_distinct/decimal.rs b/datafusion/functions-aggregate-common/src/aggregate/avg_distinct/decimal.rs index 0394a8391ad70..780835fd10046 100644 --- a/datafusion/functions-aggregate-common/src/aggregate/avg_distinct/decimal.rs +++ b/datafusion/functions-aggregate-common/src/aggregate/avg_distinct/decimal.rs @@ -173,6 +173,10 @@ mod tests { } #[test] + #[expect( + clippy::large_digit_groups, + reason = "The grouping spells out the decimal scale, e.g. 180_00000000 is 180 with scale 8" + )] fn test_decimal64_distinct_avg_accumulator() -> Result<()> { let precision = 10_u8; let scale = 4_i8; @@ -202,6 +206,10 @@ mod tests { } #[test] + #[expect( + clippy::large_digit_groups, + reason = "The grouping spells out the decimal scale, e.g. 180_00000000 is 180 with scale 8" + )] fn test_decimal128_distinct_avg_accumulator() -> Result<()> { let precision = 10_u8; let scale = 4_i8; @@ -231,6 +239,10 @@ mod tests { } #[test] + #[expect( + clippy::large_digit_groups, + reason = "The grouping spells out the decimal scale, e.g. 180_00000000 is 180 with scale 8" + )] fn test_decimal256_distinct_avg_accumulator() -> Result<()> { let precision = 50_u8; let scale = 2_i8; From 627205303e2607016648294656024840cc9a73ba Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 11:20:06 +0200 Subject: [PATCH 24/42] feat: enable clippy::manual_is_variant_and `opt.map(f).unwrap_or_default()` and `opt.filter(f).is_some()` are both `opt.is_some_and(f)`. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/expr/src/logical_plan/invariants.rs | 6 +----- .../physical-plan/src/repartition/distributor_channels.rs | 7 +------ datafusion/pruning/src/pruning_predicate.rs | 6 ++---- datafusion/sql/src/unparser/plan.rs | 6 +++--- 5 files changed, 8 insertions(+), 18 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 77ea246da0c6d..25f4d28ced801 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -266,6 +266,7 @@ macro_use_imports = "warn" manual_ilog2 = "warn" manual_instant_elapsed = "warn" manual_is_power_of_two = "warn" +manual_is_variant_and = "warn" # `(a + b) / 2` can overflow; `a.midpoint(b)` cannot manual_midpoint = "warn" match_wild_err_arm = "warn" diff --git a/datafusion/expr/src/logical_plan/invariants.rs b/datafusion/expr/src/logical_plan/invariants.rs index d75f9140d795d..f36653694c21d 100644 --- a/datafusion/expr/src/logical_plan/invariants.rs +++ b/datafusion/expr/src/logical_plan/invariants.rs @@ -185,11 +185,7 @@ pub fn check_subquery_expr( } } _ => { - if inner_plan - .max_rows() - .filter(|max_row| *max_row <= 1) - .is_some() - { + if inner_plan.max_rows().is_some_and(|max_row| max_row <= 1) { Ok(()) } else { plan_err!( diff --git a/datafusion/physical-plan/src/repartition/distributor_channels.rs b/datafusion/physical-plan/src/repartition/distributor_channels.rs index 22872d1e32d49..4880561c12134 100644 --- a/datafusion/physical-plan/src/repartition/distributor_channels.rs +++ b/datafusion/physical-plan/src/repartition/distributor_channels.rs @@ -173,12 +173,7 @@ impl Drop for DistributionSender { // senders and it will decrement the `empty_channels` counter. It will also set `data` to `None`. The sender // side will then see that `data` is `None` and can therefore infer that the receiver end was dropped, and // hence it MUST NOT decrement the `empty_channels` counter. - if state - .data - .as_ref() - .map(|data| data.is_empty()) - .unwrap_or_default() - { + if state.data.as_ref().is_some_and(|data| data.is_empty()) { // channel is gone, so we need to clear our signal self.gate.decr_empty_channels(); } diff --git a/datafusion/pruning/src/pruning_predicate.rs b/datafusion/pruning/src/pruning_predicate.rs index b292861f8f43c..692735e4e1e44 100644 --- a/datafusion/pruning/src/pruning_predicate.rs +++ b/datafusion/pruning/src/pruning_predicate.rs @@ -795,14 +795,12 @@ impl BoolVecBuilder { fn is_always_true(expr: &Arc) -> bool { expr.downcast_ref::() - .map(|l| matches!(l.value(), ScalarValue::Boolean(Some(true)))) - .unwrap_or_default() + .is_some_and(|l| matches!(l.value(), ScalarValue::Boolean(Some(true)))) } fn is_always_false(expr: &Arc) -> bool { expr.downcast_ref::() - .map(|l| matches!(l.value(), ScalarValue::Boolean(Some(false)))) - .unwrap_or_default() + .is_some_and(|l| matches!(l.value(), ScalarValue::Boolean(Some(false)))) } /// Describes which columns statistics are necessary to evaluate a diff --git a/datafusion/sql/src/unparser/plan.rs b/datafusion/sql/src/unparser/plan.rs index f4b60176cfba9..9922509a0e609 100644 --- a/datafusion/sql/src/unparser/plan.rs +++ b/datafusion/sql/src/unparser/plan.rs @@ -823,9 +823,9 @@ impl Unparser<'_> { "derived_projection", plan, relation, - unnest_input_type - .filter(|t| matches!(t, UnnestInputType::OuterReference)) - .is_some(), + unnest_input_type.is_some_and(|t| { + matches!(t, UnnestInputType::OuterReference) + }), columns, ); } From 084783a3c5b9f327da6a3d4dba81537ad5a81751 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 11:26:53 +0200 Subject: [PATCH 25/42] feat: enable clippy::duration_suboptimal_units `Duration::from_secs(60)` says "60 seconds" where the code means one minute. `from_mins(1)` / `from_hours(..)` say it directly. Both constructors are stable since Rust 1.91, below our 1.94 MSRV. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/core/src/execution/context/mod.rs | 6 +++--- datafusion/core/tests/sql/runtime_config.rs | 2 +- datafusion/execution/src/cache/cache_manager.rs | 4 ++-- .../physical-plan/src/windows/bounded_window_agg_exec.rs | 2 +- 5 files changed, 8 insertions(+), 7 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 25f4d28ced801..96bf5f86bc446 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -225,6 +225,7 @@ debug_assert_with_mut_call = "warn" decimal_bitwise_operands = "warn" default_union_representation = "warn" doc_include_without_cfg = "warn" +duration_suboptimal_units = "warn" empty_enum_variants_with_brackets = "warn" empty_enums = "warn" exit = "warn" diff --git a/datafusion/core/src/execution/context/mod.rs b/datafusion/core/src/execution/context/mod.rs index 5b287f103abdd..b9691f4f72b01 100644 --- a/datafusion/core/src/execution/context/mod.rs +++ b/datafusion/core/src/execution/context/mod.rs @@ -2959,8 +2959,8 @@ mod tests { // Valid durations for (duration, want) in [ ("1s", Duration::from_secs(1)), - ("1m", Duration::from_secs(60)), - ("1m0s", Duration::from_secs(60)), + ("1m", Duration::from_mins(1)), + ("1m0s", Duration::from_mins(1)), ("1m1s", Duration::from_secs(61)), ] { let have = @@ -2996,7 +2996,7 @@ mod tests { ), ( "307445734561825860m", - Duration::from_secs(307445734561825860 * 60), + Duration::from_hours(5124095576030431), ), ( "307445734561825860m10s", diff --git a/datafusion/core/tests/sql/runtime_config.rs b/datafusion/core/tests/sql/runtime_config.rs index b0e4bccf30aba..1275edc6d8b0c 100644 --- a/datafusion/core/tests/sql/runtime_config.rs +++ b/datafusion/core/tests/sql/runtime_config.rs @@ -368,7 +368,7 @@ async fn test_list_files_cache_ttl() { }; update_limit(&ctx, "1m").await; - assert_eq!(get_limit(&ctx), Duration::from_secs(60)); + assert_eq!(get_limit(&ctx), Duration::from_mins(1)); update_limit(&ctx, "30s").await; assert_eq!(get_limit(&ctx), Duration::from_secs(30)); diff --git a/datafusion/execution/src/cache/cache_manager.rs b/datafusion/execution/src/cache/cache_manager.rs index 83dcf70975e2b..5b8c098e3a814 100644 --- a/datafusion/execution/src/cache/cache_manager.rs +++ b/datafusion/execution/src/cache/cache_manager.rs @@ -562,7 +562,7 @@ mod tests { // Put cache in config WITH a different TTL set let config = CacheManagerConfig::default() .with_list_files_cache(Some(Arc::new(list_file_cache))) - .with_list_files_cache_ttl(Some(Duration::from_secs(60))); + .with_list_files_cache_ttl(Some(Duration::from_mins(1))); // Create CacheManager from config let cache_manager = CacheManager::try_new(&config).unwrap(); @@ -572,7 +572,7 @@ mod tests { assert_eq!( cache_ttl, - Some(Duration::from_secs(60)), + Some(Duration::from_mins(1)), "TTL should be overridden to 60 seconds when set in config" ); } diff --git a/datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs b/datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs index d4c98009ba70d..11d0f677600ea 100644 --- a/datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs +++ b/datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs @@ -2266,7 +2266,7 @@ mod tests { let chunk_length = 2; let n_future_range = 1; - let timeout_duration = Duration::from_millis(2000); + let timeout_duration = Duration::from_secs(2); let source = generate_never_ending_source(n_rows, chunk_length, 1, true, false, 5)?; From 215f20a5b257b6b061b7395e317eda408695069b Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 11:35:07 +0200 Subject: [PATCH 26/42] feat: enable clippy::elidable_lifetime_names A named lifetime that is used only once carries no information; `'_` makes it obvious that nothing is being tied together. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/core/tests/parquet/encryption.rs | 4 ++-- datafusion/functions-nested/src/array_has.rs | 4 ++-- datafusion/optimizer/src/utils.rs | 2 +- datafusion/physical-plan/src/column_rewriter.rs | 2 +- datafusion/sql/src/relation/mod.rs | 4 +--- 6 files changed, 8 insertions(+), 9 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 96bf5f86bc446..a256422ca7866 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -226,6 +226,7 @@ decimal_bitwise_operands = "warn" default_union_representation = "warn" doc_include_without_cfg = "warn" duration_suboptimal_units = "warn" +elidable_lifetime_names = "warn" empty_enum_variants_with_brackets = "warn" empty_enums = "warn" exit = "warn" diff --git a/datafusion/core/tests/parquet/encryption.rs b/datafusion/core/tests/parquet/encryption.rs index 12bdb600c2ac9..a59871f30e9a8 100644 --- a/datafusion/core/tests/parquet/encryption.rs +++ b/datafusion/core/tests/parquet/encryption.rs @@ -41,10 +41,10 @@ use std::sync::atomic::{AtomicU8, Ordering}; use std::sync::{Arc, Mutex}; use tempfile::TempDir; -async fn read_parquet_test_data<'a, T: Into>( +async fn read_parquet_test_data>( path: T, ctx: &SessionContext, - options: ParquetReadOptions<'a>, + options: ParquetReadOptions<'_>, ) -> Vec { ctx.read_parquet(path.into(), options) .await diff --git a/datafusion/functions-nested/src/array_has.rs b/datafusion/functions-nested/src/array_has.rs index 0f680469f6023..bb3c2dd13fff1 100644 --- a/datafusion/functions-nested/src/array_has.rs +++ b/datafusion/functions-nested/src/array_has.rs @@ -330,8 +330,8 @@ impl<'a> ArrayWrapper<'a> { /// Primitive and string element types take a per-type fast path; nested (and any /// other) element types fall back to the per-row `eq` kernel, which allocates a /// `BooleanArray` per row. -fn array_has_dispatch_for_array<'a>( - haystack: ArrayWrapper<'a>, +fn array_has_dispatch_for_array( + haystack: ArrayWrapper<'_>, needle: &ArrayRef, ) -> Result { let combined_nulls = NullBuffer::union(haystack.nulls(), needle.nulls()); diff --git a/datafusion/optimizer/src/utils.rs b/datafusion/optimizer/src/utils.rs index 4ea1589cfa7df..d4ac31e8a517c 100644 --- a/datafusion/optimizer/src/utils.rs +++ b/datafusion/optimizer/src/utils.rs @@ -141,7 +141,7 @@ impl<'a> ColumnReference<'a> { } /// Returns references to all columns in the schema -pub(crate) fn schema_columns<'a>(schema: &'a DFSchema) -> HashSet> { +pub(crate) fn schema_columns(schema: &DFSchema) -> HashSet> { schema .iter() .flat_map(|(qualifier, field)| { diff --git a/datafusion/physical-plan/src/column_rewriter.rs b/datafusion/physical-plan/src/column_rewriter.rs index 2df95cd61474e..e03f5ab5d3d9d 100644 --- a/datafusion/physical-plan/src/column_rewriter.rs +++ b/datafusion/physical-plan/src/column_rewriter.rs @@ -43,7 +43,7 @@ impl<'a> PhysicalColumnRewriter<'a> { } } -impl<'a> TreeNodeRewriter for PhysicalColumnRewriter<'a> { +impl TreeNodeRewriter for PhysicalColumnRewriter<'_> { type Node = Arc; fn f_down( diff --git a/datafusion/sql/src/relation/mod.rs b/datafusion/sql/src/relation/mod.rs index 08a292475fd72..8549bd833e038 100644 --- a/datafusion/sql/src/relation/mod.rs +++ b/datafusion/sql/src/relation/mod.rs @@ -39,9 +39,7 @@ struct SqlToRelRelationContext<'a, 'b, S: ContextProvider> { } // Implement RelationPlannerContext -impl<'a, 'b, S: ContextProvider> RelationPlannerContext - for SqlToRelRelationContext<'a, 'b, S> -{ +impl RelationPlannerContext for SqlToRelRelationContext<'_, '_, S> { fn context_provider(&self) -> &dyn ContextProvider { self.planner.context_provider } From 460c0bd58a07444cca27307f71ef02ae7d599ab5 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 11:39:04 +0200 Subject: [PATCH 27/42] feat: enable clippy::inconsistent_struct_constructor Initializing fields in declaration order makes a struct literal easy to check against the definition, and makes it obvious when a field is missing. All hits are pure reorderings. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/datasource-parquet/src/metrics.rs | 8 ++++---- datafusion/execution/src/task.rs | 2 +- datafusion/ffi/src/expr/interval.rs | 2 +- datafusion/functions-aggregate-common/src/tdigest.rs | 2 +- .../src/expressions/case/literal_lookup_table/mod.rs | 2 +- .../physical-plan/src/joins/hash_join/shared_bounds.rs | 4 ++-- datafusion/physical-plan/src/joins/utils.rs | 2 +- datafusion/physical-plan/src/repartition/mod.rs | 4 ++-- 9 files changed, 14 insertions(+), 13 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index a256422ca7866..1e105abe9a641 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -240,6 +240,7 @@ fn_params_excessive_bools = "warn" fn_to_numeric_cast_any = "warn" ignore_without_reason = "warn" imprecise_flops = "warn" +inconsistent_struct_constructor = "warn" index_refutable_slice = "warn" inefficient_to_string = "warn" infinite_loop = "warn" diff --git a/datafusion/datasource-parquet/src/metrics.rs b/datafusion/datasource-parquet/src/metrics.rs index cbdcb73196b17..a3573c8624792 100644 --- a/datafusion/datasource-parquet/src/metrics.rs +++ b/datafusion/datasource-parquet/src/metrics.rs @@ -215,22 +215,22 @@ impl ParquetFileMetrics { files_ranges_pruned_statistics, predicate_evaluation_errors, row_groups_pruned_bloom_filter, - row_groups_pruned_statistics, limit_pruned_row_groups, + row_groups_pruned_statistics, + row_groups_pruned_dynamic_filter, bytes_scanned, pushdown_rows_pruned, pushdown_rows_matched, row_pushdown_eval_time, - page_index_rows_pruned, - page_index_pages_pruned, statistics_eval_time, bloom_filter_eval_time, + page_index_rows_pruned, + page_index_pages_pruned, page_index_eval_time, metadata_load_time, scan_efficiency_ratio, predicate_cache_inner_records, predicate_cache_records, - row_groups_pruned_dynamic_filter, } } diff --git a/datafusion/execution/src/task.rs b/datafusion/execution/src/task.rs index 1c1a717d19c79..184bae502e709 100644 --- a/datafusion/execution/src/task.rs +++ b/datafusion/execution/src/task.rs @@ -104,8 +104,8 @@ impl TaskContext { runtime: Arc, ) -> Self { Self { - task_id, session_id, + task_id, session_config, scalar_functions, higher_order_functions, diff --git a/datafusion/ffi/src/expr/interval.rs b/datafusion/ffi/src/expr/interval.rs index 6334f7bb24d90..7aae7a9e3fede 100644 --- a/datafusion/ffi/src/expr/interval.rs +++ b/datafusion/ffi/src/expr/interval.rs @@ -36,7 +36,7 @@ impl TryFrom<&Interval> for FFI_Interval { let upper = value.upper().try_into()?; let lower = value.lower().try_into()?; - Ok(FFI_Interval { upper, lower }) + Ok(FFI_Interval { lower, upper }) } } impl TryFrom for FFI_Interval { diff --git a/datafusion/functions-aggregate-common/src/tdigest.rs b/datafusion/functions-aggregate-common/src/tdigest.rs index 8db7d0bc8a541..860f56f6c00dc 100644 --- a/datafusion/functions-aggregate-common/src/tdigest.rs +++ b/datafusion/functions-aggregate-common/src/tdigest.rs @@ -688,12 +688,12 @@ impl TDigest { } Ok(Self { + centroids, max_size, sum, count, max, min, - centroids, }) } } diff --git a/datafusion/physical-expr/src/expressions/case/literal_lookup_table/mod.rs b/datafusion/physical-expr/src/expressions/case/literal_lookup_table/mod.rs index 0d4291ccc934b..94735c22a55e0 100644 --- a/datafusion/physical-expr/src/expressions/case/literal_lookup_table/mod.rs +++ b/datafusion/physical-expr/src/expressions/case/literal_lookup_table/mod.rs @@ -189,8 +189,8 @@ impl LiteralLookupTable { Some(Self { lookup, - then_and_else_values, else_index, + then_and_else_values, }) } diff --git a/datafusion/physical-plan/src/joins/hash_join/shared_bounds.rs b/datafusion/physical-plan/src/joins/hash_join/shared_bounds.rs index 94ec4565a4cef..77b327accffe3 100644 --- a/datafusion/physical-plan/src/joins/hash_join/shared_bounds.rs +++ b/datafusion/physical-plan/src/joins/hash_join/shared_bounds.rs @@ -501,8 +501,8 @@ impl SharedBuildAccumulator { *completed_partitions += 1; } partitions[partition_id] = PartitionStatus::Reported(PartitionData { - pushdown, bounds, + pushdown, keys_have_null, }); } @@ -520,8 +520,8 @@ impl SharedBuildAccumulator { ) => { if matches!(data, PartitionStatus::Pending) { *data = PartitionStatus::Reported(PartitionData { - pushdown, bounds, + pushdown, keys_have_null, }); } diff --git a/datafusion/physical-plan/src/joins/utils.rs b/datafusion/physical-plan/src/joins/utils.rs index 20467a7ec5e33..85c31473165e5 100644 --- a/datafusion/physical-plan/src/joins/utils.rs +++ b/datafusion/physical-plan/src/joins/utils.rs @@ -1830,6 +1830,7 @@ impl BuildProbeJoinMetrics { .ratio_metrics("avg_fanout", partition); Self { + baseline, build_time, build_input_batches, build_input_rows, @@ -1837,7 +1838,6 @@ impl BuildProbeJoinMetrics { join_time, input_batches, input_rows, - baseline, probe_hit_rate, avg_fanout, } diff --git a/datafusion/physical-plan/src/repartition/mod.rs b/datafusion/physical-plan/src/repartition/mod.rs index 063954a72a094..033498799449a 100644 --- a/datafusion/physical-plan/src/repartition/mod.rs +++ b/datafusion/physical-plan/src/repartition/mod.rs @@ -551,9 +551,9 @@ impl RepartitionExecState { tx, rx, reservation, - spill_readers, - spill_writers, shared_coalescer, + spill_writers, + spill_readers, }, ); } From e19846af878d8230528f012b2d173956674a4f02 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 11:41:19 +0200 Subject: [PATCH 28/42] feat: enable clippy::bool_to_int_with_if `if cond { 1 } else { 0 }` is `T::from(cond)`, which cannot get the two branches the wrong way round. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/common/src/utils/mod.rs | 2 +- datafusion/expr/src/logical_plan/plan.rs | 2 +- .../src/analyzer/resolve_grouping_function.rs | 2 +- datafusion/physical-plan/benches/multi_group_by.rs | 12 ++++++------ datafusion/physical-plan/src/aggregates/mod.rs | 6 +++--- datafusion/physical-plan/src/display.rs | 2 +- 7 files changed, 14 insertions(+), 13 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 1e105abe9a641..80fdf546bda7a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -217,6 +217,7 @@ zstd = { version = "0.13", default-features = false } allow_attributes = "warn" as_ptr_cast_mut = "warn" assigning_clones = "warn" +bool_to_int_with_if = "warn" branches_sharing_code = "warn" checked_conversions = "warn" clear_with_drain = "warn" diff --git a/datafusion/common/src/utils/mod.rs b/datafusion/common/src/utils/mod.rs index 73772b319351c..2c8f3c8e74285 100644 --- a/datafusion/common/src/utils/mod.rs +++ b/datafusion/common/src/utils/mod.rs @@ -950,7 +950,7 @@ pub mod datafusion_strsim { let mut distance_b = i; for (j, b_elem) in b.into_iter().enumerate() { - let cost = if a_elem == b_elem { 0usize } else { 1usize }; + let cost = usize::from(a_elem != b_elem); let distance_a = distance_b + cost; distance_b = cache[j]; result = min(result + 1, min(distance_a, distance_b + 1)); diff --git a/datafusion/expr/src/logical_plan/plan.rs b/datafusion/expr/src/logical_plan/plan.rs index 1a141ea52a13a..ba4fccfb06703 100644 --- a/datafusion/expr/src/logical_plan/plan.rs +++ b/datafusion/expr/src/logical_plan/plan.rs @@ -1914,7 +1914,7 @@ impl LogicalPlan { produce_one_row, schema: _, }) => { - let rows = if *produce_one_row { 1 } else { 0 }; + let rows = i32::from(*produce_one_row); write!(f, "EmptyRelation: rows={rows}") } LogicalPlan::RecursiveQuery(RecursiveQuery { diff --git a/datafusion/optimizer/src/analyzer/resolve_grouping_function.rs b/datafusion/optimizer/src/analyzer/resolve_grouping_function.rs index 95649ab8286b7..8b1077c6b002b 100644 --- a/datafusion/optimizer/src/analyzer/resolve_grouping_function.rs +++ b/datafusion/optimizer/src/analyzer/resolve_grouping_function.rs @@ -85,7 +85,7 @@ fn replace_grouping_exprs( let columns = schema.columns(); let mut new_agg_expr = Vec::new(); let mut projection_exprs = Vec::new(); - let grouping_id_len = if is_grouping_set { 1 } else { 0 }; + let grouping_id_len = usize::from(is_grouping_set); let group_expr_len = columns.len() - aggr_expr.len() - grouping_id_len; projection_exprs.extend( columns diff --git a/datafusion/physical-plan/benches/multi_group_by.rs b/datafusion/physical-plan/benches/multi_group_by.rs index 0c689f9fcb6ce..360d311c44633 100644 --- a/datafusion/physical-plan/benches/multi_group_by.rs +++ b/datafusion/physical-plan/benches/multi_group_by.rs @@ -66,7 +66,7 @@ fn generate_batches( let num_full_batches = num_rows / batch_size; let remainder = num_rows % batch_size; - let num_batches = num_full_batches + if remainder > 0 { 1 } else { 0 }; + let num_batches = num_full_batches + usize::from(remainder > 0); (0..num_batches) .map(|batch_idx| { @@ -386,7 +386,7 @@ fn generate_fsb_batches( let num_full_batches = num_rows / batch_size; let remainder = num_rows % batch_size; - let num_batches = num_full_batches + if remainder > 0 { 1 } else { 0 }; + let num_batches = num_full_batches + usize::from(remainder > 0); (0..num_batches) .map(|batch_idx| { @@ -477,7 +477,7 @@ fn generate_f16_batches( let num_full_batches = num_rows / batch_size; let remainder = num_rows % batch_size; - let num_batches = num_full_batches + if remainder > 0 { 1 } else { 0 }; + let num_batches = num_full_batches + usize::from(remainder > 0); (0..num_batches) .map(|batch_idx| { @@ -559,7 +559,7 @@ fn generate_duration_batches( ) -> Vec> { let num_full_batches = num_rows / batch_size; let remainder = num_rows % batch_size; - let num_batches = num_full_batches + if remainder > 0 { 1 } else { 0 }; + let num_batches = num_full_batches + usize::from(remainder > 0); (0..num_batches) .map(|batch_idx| { @@ -643,7 +643,7 @@ fn generate_interval_batches( ) -> Vec> { let num_full_batches = num_rows / batch_size; let remainder = num_rows % batch_size; - let num_batches = num_full_batches + if remainder > 0 { 1 } else { 0 }; + let num_batches = num_full_batches + usize::from(remainder > 0); (0..num_batches) .map(|batch_idx| { @@ -730,7 +730,7 @@ fn generate_decimal256_batches( ) -> Vec> { let num_full_batches = num_rows / batch_size; let remainder = num_rows % batch_size; - let num_batches = num_full_batches + if remainder > 0 { 1 } else { 0 }; + let num_batches = num_full_batches + usize::from(remainder > 0); (0..num_batches) .map(|batch_idx| { diff --git a/datafusion/physical-plan/src/aggregates/mod.rs b/datafusion/physical-plan/src/aggregates/mod.rs index c82263f98226c..aa7fb5d834021 100644 --- a/datafusion/physical-plan/src/aggregates/mod.rs +++ b/datafusion/physical-plan/src/aggregates/mod.rs @@ -3050,9 +3050,9 @@ pub(crate) fn group_id_array( {max_ordinal} require {total_bits} bits, which exceeds 64" ); } - let semantic_id = group.iter().fold(0u64, |acc, &is_null| { - (acc << 1) | if is_null { 1 } else { 0 } - }); + let semantic_id = group + .iter() + .fold(0u64, |acc, &is_null| (acc << 1) | u64::from(is_null)); let full_id = semantic_id | ((ordinal as u64) << n); if total_bits <= 8 { Ok(Arc::new(UInt8Array::from(vec![full_id as u8; num_rows]))) diff --git a/datafusion/physical-plan/src/display.rs b/datafusion/physical-plan/src/display.rs index d2bdcef2e97a3..24a559b27f8f5 100644 --- a/datafusion/physical-plan/src/display.rs +++ b/datafusion/physical-plan/src/display.rs @@ -1352,7 +1352,7 @@ impl TreeRenderVisitor<'_, '_> { } else { let total_spaces = max_render_width - render_width; let half_spaces = total_spaces / 2; - let extra_left_space = if total_spaces.is_multiple_of(2) { 0 } else { 1 }; + let extra_left_space = usize::from(!total_spaces.is_multiple_of(2)); format!( "{}{}{}", " ".repeat(half_spaces + extra_left_space), From a8e2e35e314f6f2acad1bf80380ad8b3bb258634 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 11:48:03 +0200 Subject: [PATCH 29/42] feat: enable clippy::unnecessary_struct_initialization `T { ..Default::default() }` and `Self { ..self.clone() }` are just `T::default()` and `self.clone()`. Where clippy suggested a bare `Default::default()` the concrete type name is kept, since it is what tells the reader what is being built. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + .../core/src/execution/context/parquet.rs | 35 +++---------------- datafusion/datasource-arrow/src/source.rs | 2 +- datafusion/datasource/src/test_util.rs | 2 +- datafusion/physical-plan/src/filter.rs | 16 +++------ 5 files changed, 12 insertions(+), 44 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 80fdf546bda7a..601be7a536345 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -324,6 +324,7 @@ unnecessary_lazy_evaluations = "warn" unnecessary_safety_comment = "warn" unnecessary_safety_doc = "warn" unnecessary_self_imports = "warn" +unnecessary_struct_initialization = "warn" unused_async = "warn" unused_peekable = "warn" unused_rounding = "warn" diff --git a/datafusion/core/src/execution/context/parquet.rs b/datafusion/core/src/execution/context/parquet.rs index 3c750352f199a..aad8518ba4a45 100644 --- a/datafusion/core/src/execution/context/parquet.rs +++ b/datafusion/core/src/execution/context/parquet.rs @@ -314,12 +314,7 @@ mod tests { // Read the dataframe from 'output1.parquet' with the default file extension. let read_df = ctx - .read_parquet( - &path1, - ParquetReadOptions { - ..Default::default() - }, - ) + .read_parquet(&path1, ParquetReadOptions::default()) .await?; let results = read_df.collect().await?; @@ -342,12 +337,7 @@ mod tests { // Read the dataframe from 'output3.parquet.snappy.parquet' with the wrong file extension. let read_df = ctx - .read_parquet( - &path2, - ParquetReadOptions { - ..Default::default() - }, - ) + .read_parquet(&path2, ParquetReadOptions::default()) .await; let binding = DataFilePaths::to_urls(&path2).unwrap(); let expected_path = binding[0].as_str(); @@ -360,12 +350,7 @@ mod tests { // Read the dataframe from 'output3.parquet.snappy.parquet' with the correct file extension. let read_df = ctx - .read_parquet( - &path3, - ParquetReadOptions { - ..Default::default() - }, - ) + .read_parquet(&path3, ParquetReadOptions::default()) .await?; let results = read_df.collect().await?; @@ -376,12 +361,7 @@ mod tests { // errors on an empty location instead of producing a 0-column table. std::fs::create_dir(&path4)?; let err = ctx - .read_parquet( - &path4, - ParquetReadOptions { - ..Default::default() - }, - ) + .read_parquet(&path4, ParquetReadOptions::default()) .await .expect_err("read_parquet on an empty folder should error"); assert!( @@ -391,12 +371,7 @@ mod tests { // Read the dataframe from double dot folder; let read_df = ctx - .read_parquet( - &path5, - ParquetReadOptions { - ..Default::default() - }, - ) + .read_parquet(&path5, ParquetReadOptions::default()) .await?; let results = read_df.collect().await?; diff --git a/datafusion/datasource-arrow/src/source.rs b/datafusion/datasource-arrow/src/source.rs index dba99a9758886..64014709b3148 100644 --- a/datafusion/datasource-arrow/src/source.rs +++ b/datafusion/datasource-arrow/src/source.rs @@ -315,7 +315,7 @@ impl FileSource for ArrowSource { } fn with_batch_size(&self, _batch_size: usize) -> Arc { - Arc::new(Self { ..self.clone() }) + Arc::new(self.clone()) } fn metrics(&self) -> &ExecutionPlanMetricsSet { diff --git a/datafusion/datasource/src/test_util.rs b/datafusion/datasource/src/test_util.rs index 20dfae5b3ac79..8b787aa104381 100644 --- a/datafusion/datasource/src/test_util.rs +++ b/datafusion/datasource/src/test_util.rs @@ -91,7 +91,7 @@ impl FileSource for MockSource { } fn with_batch_size(&self, _batch_size: usize) -> Arc { - Arc::new(Self { ..self.clone() }) + Arc::new(self.clone()) } fn metrics(&self) -> &ExecutionPlanMetricsSet { diff --git a/datafusion/physical-plan/src/filter.rs b/datafusion/physical-plan/src/filter.rs index 5df5482fb75de..afd5edb70e6c8 100644 --- a/datafusion/physical-plan/src/filter.rs +++ b/datafusion/physical-plan/src/filter.rs @@ -2148,9 +2148,7 @@ mod tests { Statistics { num_rows: Precision::Inexact(1000), total_byte_size: Precision::Inexact(4000), - column_statistics: vec![ColumnStatistics { - ..Default::default() - }], + column_statistics: vec![ColumnStatistics::default()], }, schema, )); @@ -2320,15 +2318,9 @@ mod tests { max_value: Precision::Inexact(ScalarValue::Int32(Some(100))), ..Default::default() }, - ColumnStatistics { - ..Default::default() - }, - ColumnStatistics { - ..Default::default() - }, - ColumnStatistics { - ..Default::default() - }, + ColumnStatistics::default(), + ColumnStatistics::default(), + ColumnStatistics::default(), ], }, schema, From 9dce16b6efd6953a2888fff3ca034b0c80f38e3a Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 11:55:32 +0200 Subject: [PATCH 30/42] feat: enable clippy::manual_assert `if !cond { panic!(msg) }` is `assert!(cond, msg)`, which states the invariant instead of its negation. One of clippy's rewrites produced a double negative (`!...is_none()`); that one is written as `.is_some()` instead. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + benchmarks/src/bin/mem_profile.rs | 18 ++++++++---------- .../core/src/bin/print_functions_docs.rs | 11 +++++------ .../core/src/datasource/physical_plan/csv.rs | 7 ++++--- .../core/src/datasource/physical_plan/json.rs | 7 ++++--- .../tests/fuzz_cases/topk_filter_pushdown.rs | 11 +++++------ datafusion/core/tests/memory_limit/mod.rs | 9 ++++----- datafusion/core/tests/sql/unparser.rs | 15 +++++++-------- .../physical-expr-common/src/binary_map.rs | 13 ++++++------- datafusion/physical-plan/src/buffer.rs | 4 +--- datafusion/pruning/src/pruning_predicate.rs | 9 ++++----- datafusion/sql/tests/cases/diagnostic.rs | 4 +--- datafusion/sql/tests/sql_integration.rs | 7 ++++--- 13 files changed, 54 insertions(+), 62 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 601be7a536345..5734f1ba9aa89 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -267,6 +267,7 @@ linkedlist = "warn" literal_string_with_formatting_args = "warn" lossy_float_literal = "warn" macro_use_imports = "warn" +manual_assert = "warn" manual_ilog2 = "warn" manual_instant_elapsed = "warn" manual_is_power_of_two = "warn" diff --git a/benchmarks/src/bin/mem_profile.rs b/benchmarks/src/bin/mem_profile.rs index 41a0baecbba86..36e22570f5b12 100644 --- a/benchmarks/src/bin/mem_profile.rs +++ b/benchmarks/src/bin/mem_profile.rs @@ -145,11 +145,10 @@ fn run_benchmark_as_child_process( env::var("CARGO_TARGET_DIR").unwrap_or_else(|_| "target".to_string()); let command = format!("{target_dir}/{profile}/dfbench"); // Check whether benchmark binary exists - if !Path::new(&command).exists() { - panic!( - "Benchmark binary not found: `{command}`\nRun this command from the top-level `datafusion/` directory so `target/{profile}/dfbench` can be found.", - ); - } + assert!( + Path::new(&command).exists(), + "Benchmark binary not found: `{command}`\nRun this command from the top-level `datafusion/` directory so `target/{profile}/dfbench` can be found.", + ); args.insert(0, command); let mut results = vec![]; @@ -339,11 +338,10 @@ mod tests { let stdout = String::from_utf8_lossy(&output.stdout); let stderr = String::from_utf8_lossy(&output.stderr); - if !output.status.success() { - panic!( - "mem_profile failed\nstdout:\n{stdout}\nstderr:\n{stderr}---------------------", - ); - } + assert!( + output.status.success(), + "mem_profile failed\nstdout:\n{stdout}\nstderr:\n{stderr}---------------------", + ); assert!( stdout.contains("Peak RSS") diff --git a/datafusion/core/src/bin/print_functions_docs.rs b/datafusion/core/src/bin/print_functions_docs.rs index 86f433ac8e12c..c4b5fc4ef6d16 100644 --- a/datafusion/core/src/bin/print_functions_docs.rs +++ b/datafusion/core/src/bin/print_functions_docs.rs @@ -34,12 +34,11 @@ use std::sync::Arc; fn main() -> Result<()> { let args: Vec = args().collect(); - if args.len() != 2 { - panic!( - "Usage: {} type (one of 'aggregate', 'scalar', 'window')", - args[0] - ); - } + assert!( + args.len() == 2, + "Usage: {} type (one of 'aggregate', 'scalar', 'window')", + args[0] + ); let function_type = args[1].trim().to_lowercase(); let docs = match function_type.as_str() { diff --git a/datafusion/core/src/datasource/physical_plan/csv.rs b/datafusion/core/src/datasource/physical_plan/csv.rs index 56642d583e414..7980df87fa576 100644 --- a/datafusion/core/src/datasource/physical_plan/csv.rs +++ b/datafusion/core/src/datasource/physical_plan/csv.rs @@ -780,9 +780,10 @@ mod tests { } } - if part_0_name.is_empty() { - panic!("Did not find part_0 in csv output files!") - } + assert!( + !part_0_name.is_empty(), + "Did not find part_0 in csv output files!" + ); // register each partition as well as the top level dir let csv_read_option = CsvReadOptions::new().schema(&schema).has_header(false); ctx.register_csv( diff --git a/datafusion/core/src/datasource/physical_plan/json.rs b/datafusion/core/src/datasource/physical_plan/json.rs index b70791c7b2390..6b4361e0c4d07 100644 --- a/datafusion/core/src/datasource/physical_plan/json.rs +++ b/datafusion/core/src/datasource/physical_plan/json.rs @@ -426,9 +426,10 @@ mod tests { } } - if part_0_name.is_empty() { - panic!("Did not find part_0 in json output files!") - } + assert!( + !part_0_name.is_empty(), + "Did not find part_0 in json output files!" + ); // register each partition as well as the top level dir let json_read_option = JsonReadOptions::default(); diff --git a/datafusion/core/tests/fuzz_cases/topk_filter_pushdown.rs b/datafusion/core/tests/fuzz_cases/topk_filter_pushdown.rs index 316cb177fac46..80df91cb1036b 100644 --- a/datafusion/core/tests/fuzz_cases/topk_filter_pushdown.rs +++ b/datafusion/core/tests/fuzz_cases/topk_filter_pushdown.rs @@ -259,12 +259,11 @@ async fn run_query( let result = run_query_with_config(&query, cfg_with_dynamic_filters, dataset.clone()).await; // Check that dynamic filters were actually pushed down - if !has_dynamic_filter_expr_pushdown(&result.explain_plan) { - panic!( - "Dynamic filter was not pushed down in query: {query}\n\n{}", - result.explain_plan - ); - } + assert!( + has_dynamic_filter_expr_pushdown(&result.explain_plan), + "Dynamic filter was not pushed down in query: {query}\n\n{}", + result.explain_plan + ); RunQueryResult { query: query.to_string(), diff --git a/datafusion/core/tests/memory_limit/mod.rs b/datafusion/core/tests/memory_limit/mod.rs index 84d7e9c4508b5..61369bd50e826 100644 --- a/datafusion/core/tests/memory_limit/mod.rs +++ b/datafusion/core/tests/memory_limit/mod.rs @@ -941,11 +941,10 @@ impl TestCase { match df.collect().await { Ok(_batches) => { - if !expected_success { - panic!( - "Unexpected success when running, expected memory limit failure" - ) - } + assert!( + expected_success, + "Unexpected success when running, expected memory limit failure" + ); } Err(e) => { if expected_success { diff --git a/datafusion/core/tests/sql/unparser.rs b/datafusion/core/tests/sql/unparser.rs index 355a58fd6f45b..3982c60dbc7d9 100644 --- a/datafusion/core/tests/sql/unparser.rs +++ b/datafusion/core/tests/sql/unparser.rs @@ -970,14 +970,13 @@ async fn run_roundtrip_tests( println!("\x1b[32m✓\x1b[0m {} query: {}", suite_name, sql.name); } } - if !errors.is_empty() { - panic!( - "{} {} test(s) failed:\n\n{}", - errors.len(), - suite_name, - errors.join("\n\n---\n\n") - ); - } + assert!( + errors.is_empty(), + "{} {} test(s) failed:\n\n{}", + errors.len(), + suite_name, + errors.join("\n\n---\n\n") + ) } #[tokio::test] diff --git a/datafusion/physical-expr-common/src/binary_map.rs b/datafusion/physical-expr-common/src/binary_map.rs index 0fe810f96188b..1ae272380dcba 100644 --- a/datafusion/physical-expr-common/src/binary_map.rs +++ b/datafusion/physical-expr-common/src/binary_map.rs @@ -467,13 +467,12 @@ where observe_payload_fn(payload); } // Check for overflow in offsets (if more data was sent than can be represented) - if O::from_usize(self.buffer.len()).is_none() { - panic!( - "Put {} bytes in buffer, more than can be represented by a {}", - self.buffer.len(), - type_name::() - ); - } + assert!( + O::from_usize(self.buffer.len()).is_some(), + "Put {} bytes in buffer, more than can be represented by a {}", + self.buffer.len(), + type_name::() + ) } /// Converts this set into a `StringArray`, `LargeStringArray`, diff --git a/datafusion/physical-plan/src/buffer.rs b/datafusion/physical-plan/src/buffer.rs index 24cca6b0b17f4..5879b98c348e3 100644 --- a/datafusion/physical-plan/src/buffer.rs +++ b/datafusion/physical-plan/src/buffer.rs @@ -650,9 +650,7 @@ mod tests { // A panic while polling the input must surface as a stream error, not a // silent end-of-stream that drops the rest of the partition's output. let input = futures::stream::iter([1, 2, 3, 4]).map(|v| { - if v == 3 { - panic!("boom on 3"); - } + assert!(v != 3, "boom on 3"); Ok(v) }); let (_, res) = memory_pool_and_reservation(); diff --git a/datafusion/pruning/src/pruning_predicate.rs b/datafusion/pruning/src/pruning_predicate.rs index 692735e4e1e44..840fe9b699688 100644 --- a/datafusion/pruning/src/pruning_predicate.rs +++ b/datafusion/pruning/src/pruning_predicate.rs @@ -2601,11 +2601,10 @@ mod tests { let mut fields = HashSet::new(); for (_col, _ty, field) in p.required_columns().iter() { let was_new = fields.insert(field); - if !was_new { - panic!( - "Duplicate field in required schema: {field:?}. Previous fields:\n{fields:#?}" - ); - } + assert!( + was_new, + "Duplicate field in required schema: {field:?}. Previous fields:\n{fields:#?}" + ) } } diff --git a/datafusion/sql/tests/cases/diagnostic.rs b/datafusion/sql/tests/cases/diagnostic.rs index 1f2cefdec0629..1f4eeca83bfe2 100644 --- a/datafusion/sql/tests/cases/diagnostic.rs +++ b/datafusion/sql/tests/cases/diagnostic.rs @@ -170,9 +170,7 @@ fn get_spans(query: &'static str) -> HashMap { } } - if !stack.is_empty() { - panic!("unbalanced tags"); - } + assert!(stack.is_empty(), "unbalanced tags"); spans } diff --git a/datafusion/sql/tests/sql_integration.rs b/datafusion/sql/tests/sql_integration.rs index 08a95381b32c8..a255b940e62f0 100644 --- a/datafusion/sql/tests/sql_integration.rs +++ b/datafusion/sql/tests/sql_integration.rs @@ -5283,9 +5283,10 @@ fn assert_field_not_found(mut err: DataFusionError, name: &str) { DataFusionError::SchemaError(_, _) => { let msg = format!("{err}"); let expected = format!("Schema error: No field named {name}."); - if !msg.starts_with(&expected) { - panic!("error [{msg}] did not start with [{expected}]"); - } + assert!( + msg.starts_with(&expected), + "error [{msg}] did not start with [{expected}]" + ) } _ => panic!("assert_field_not_found wrong error type"), } From 358f225ee13954f719f493b09abd4555e57a72d3 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 12:02:21 +0200 Subject: [PATCH 31/42] feat: enable clippy::rest_pat_in_fully_bound_structs A `..` in a pattern that already binds every field does nothing today, but silently swallows any field added later. Removing it turns that into a compile error. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + datafusion/core/src/execution/context/mod.rs | 8 ++------ datafusion/core/src/physical_planner.rs | 4 +--- datafusion/expr/src/expr_schema.rs | 1 - datafusion/expr/src/logical_plan/statement.rs | 15 +++------------ .../src/simplify_expressions/unwrap_cast.rs | 4 ---- datafusion/physical-plan/src/topk/mod.rs | 2 +- 7 files changed, 8 insertions(+), 27 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 5734f1ba9aa89..68b7172a8ace6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -298,6 +298,7 @@ pub_without_shorthand = "warn" rc_mutex = "warn" ref_as_ptr = "warn" ref_option_ref = "warn" +rest_pat_in_fully_bound_structs = "warn" # Catches copy-paste bugs in `if`/`else if` chains same_functions_in_if_condition = "warn" same_length_and_capacity = "warn" diff --git a/datafusion/core/src/execution/context/mod.rs b/datafusion/core/src/execution/context/mod.rs index b9691f4f72b01..0c05aac874f59 100644 --- a/datafusion/core/src/execution/context/mod.rs +++ b/datafusion/core/src/execution/context/mod.rs @@ -1107,9 +1107,7 @@ impl SessionContext { } fn set_variable(&self, stmt: SetVariable) -> Result<()> { - let SetVariable { - variable, value, .. - } = stmt; + let SetVariable { variable, value } = stmt; // Check if this is a runtime configuration if variable.starts_with("datafusion.runtime.") { @@ -1551,9 +1549,7 @@ impl SessionContext { } fn execute_prepared(&self, execute: Execute) -> Result { - let Execute { - name, parameters, .. - } = execute; + let Execute { name, parameters } = execute; let prepared = self.state.read().get_prepared(&name).ok_or_else(|| { exec_datafusion_err!("Prepared statement '{}' does not exist", name) })?; diff --git a/datafusion/core/src/physical_planner.rs b/datafusion/core/src/physical_planner.rs index 3c1e7b50780a5..99a0a3f159d75 100644 --- a/datafusion/core/src/physical_planner.rs +++ b/datafusion/core/src/physical_planner.rs @@ -1216,9 +1216,7 @@ impl DefaultPhysicalPlanner { physical_partitioning, )?) } - LogicalPlan::Sort(Sort { - expr, input, fetch, .. - }) => { + LogicalPlan::Sort(Sort { expr, input, fetch }) => { let physical_input = children.one()?; let input_dfschema = input.as_ref().schema(); let sort_exprs = create_physical_sort_exprs( diff --git a/datafusion/expr/src/expr_schema.rs b/datafusion/expr/src/expr_schema.rs index 04715a911a99d..ee70e90153e38 100644 --- a/datafusion/expr/src/expr_schema.rs +++ b/datafusion/expr/src/expr_schema.rs @@ -541,7 +541,6 @@ impl ExprSchemable for Expr { let WindowFunction { fun, params: WindowFunctionParams { args, .. }, - .. } = window_function.as_ref(); let fields = args diff --git a/datafusion/expr/src/logical_plan/statement.rs b/datafusion/expr/src/logical_plan/statement.rs index daf29d7c81d3f..77f1ac3651996 100644 --- a/datafusion/expr/src/logical_plan/statement.rs +++ b/datafusion/expr/src/logical_plan/statement.rs @@ -93,20 +93,13 @@ impl Statement { Statement::TransactionStart(TransactionStart { access_mode, isolation_level, - .. }) => { write!(f, "TransactionStart: {access_mode:?} {isolation_level:?}") } - Statement::TransactionEnd(TransactionEnd { - conclusion, - chain, - .. - }) => { + Statement::TransactionEnd(TransactionEnd { conclusion, chain }) => { write!(f, "TransactionEnd: {conclusion:?} chain:={chain}") } - Statement::SetVariable(SetVariable { - variable, value, .. - }) => { + Statement::SetVariable(SetVariable { variable, value }) => { write!(f, "SetVariable: set {variable:?} to {value:?}") } Statement::ResetVariable(ResetVariable { variable }) => { @@ -125,9 +118,7 @@ impl Statement { .join(", ") ) } - Statement::Execute(Execute { - name, parameters, .. - }) => { + Statement::Execute(Execute { name, parameters }) => { write!( f, "Execute: {} params=[{}]", diff --git a/datafusion/optimizer/src/simplify_expressions/unwrap_cast.rs b/datafusion/optimizer/src/simplify_expressions/unwrap_cast.rs index ef0bfa516fe41..1b07cfa428df5 100644 --- a/datafusion/optimizer/src/simplify_expressions/unwrap_cast.rs +++ b/datafusion/optimizer/src/simplify_expressions/unwrap_cast.rs @@ -118,12 +118,10 @@ pub(super) fn is_cast_expr_and_support_unwrap_cast_in_comparison_for_binary( Expr::TryCast(TryCast { expr: left_expr, field, - .. }) | Expr::Cast(Cast { expr: left_expr, field, - .. }), Expr::Literal(lit_val, _), ) => { @@ -161,12 +159,10 @@ pub(super) fn is_cast_expr_and_support_unwrap_cast_in_comparison_for_inlist( let (Expr::TryCast(TryCast { expr: left_expr, field, - .. }) | Expr::Cast(Cast { expr: left_expr, field, - .. })) = expr else { return false; diff --git a/datafusion/physical-plan/src/topk/mod.rs b/datafusion/physical-plan/src/topk/mod.rs index 1e3efff36b1d8..361cbacebaa3e 100644 --- a/datafusion/physical-plan/src/topk/mod.rs +++ b/datafusion/physical-plan/src/topk/mod.rs @@ -1772,7 +1772,7 @@ impl PartitionedTopKRank { let mut coalescer = BatchCoalescer::new(Arc::clone(&schema), batch_size); for pk in sorted_pks { - let RankPartitionState { mut heap, ties, .. } = + let RankPartitionState { mut heap, ties } = states.remove(&pk).expect("key from states.keys()"); if let Some(batch) = heap.emit()? { (&batch).record_output(&metrics.baseline); From e3a2747f11c80ee1fe761ccf6cab09aea282c89f Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 12:09:21 +0200 Subject: [PATCH 32/42] feat: enable clippy::equatable_if_let `if let Some(true) = x` reads as a binding but is really an equality check; `x == Some(true)` (or `matches!`) says so. Clippy suggested one tuple comparison, `(is_valid, is_included) == (true, Some(true))`; that one is written as a plain `&&` instead. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + .../user_defined_scalar_functions.rs | 2 +- datafusion/datasource-parquet/src/opener/mod.rs | 2 +- datafusion/expr/src/predicate_bounds.rs | 2 +- datafusion/ffi/src/tests/async_provider.rs | 2 +- .../aggregate/groups_accumulator/accumulate.rs | 16 ++++++++-------- datafusion/functions-aggregate/src/variance.rs | 4 ++-- .../physical-expr/src/equivalence/class.rs | 2 +- datafusion/physical-expr/src/simplifier/not.rs | 2 +- datafusion/pruning/src/pruning_predicate.rs | 2 +- datafusion/spark/src/function/url/parse_url.rs | 2 +- 11 files changed, 19 insertions(+), 18 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 68b7172a8ace6..69f7e3e4a0d51 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -230,6 +230,7 @@ duration_suboptimal_units = "warn" elidable_lifetime_names = "warn" empty_enum_variants_with_brackets = "warn" empty_enums = "warn" +equatable_if_let = "warn" exit = "warn" expl_impl_clone_on_copy = "warn" # Catches `From` impls that can panic diff --git a/datafusion/core/tests/user_defined/user_defined_scalar_functions.rs b/datafusion/core/tests/user_defined/user_defined_scalar_functions.rs index 59cb336bb4fdb..9458bbd02cef0 100644 --- a/datafusion/core/tests/user_defined/user_defined_scalar_functions.rs +++ b/datafusion/core/tests/user_defined/user_defined_scalar_functions.rs @@ -1872,7 +1872,7 @@ impl ExtensionType for MyUserExtensionType { &self, data_type: &DataType, ) -> std::result::Result<(), ArrowError> { - if let DataType::Utf8 = data_type { + if matches!(data_type, DataType::Utf8) { Ok(()) } else { Err(ArrowError::InvalidArgumentError( diff --git a/datafusion/datasource-parquet/src/opener/mod.rs b/datafusion/datasource-parquet/src/opener/mod.rs index a57f4695b55e3..d6230015c69b5 100644 --- a/datafusion/datasource-parquet/src/opener/mod.rs +++ b/datafusion/datasource-parquet/src/opener/mod.rs @@ -676,7 +676,7 @@ impl ParquetMorselPlanner { impl MorselPlanner for ParquetMorselPlanner { fn plan(self: Box) -> Result> { - if let ParquetOpenState::Done = self.state { + if matches!(self.state, ParquetOpenState::Done) { return Ok(None); } diff --git a/datafusion/expr/src/predicate_bounds.rs b/datafusion/expr/src/predicate_bounds.rs index 6b672221a7d06..13631b2e7c4b2 100644 --- a/datafusion/expr/src/predicate_bounds.rs +++ b/datafusion/expr/src/predicate_bounds.rs @@ -166,7 +166,7 @@ impl PredicateBoundsEvaluator<'_> { } // If `expr` is not nullable, we can be certain `expr` is not null - if let Ok(false) = expr.nullable(self.input_schema) { + if matches!(expr.nullable(self.input_schema), Ok(false)) { return NullableInterval::FALSE; } diff --git a/datafusion/ffi/src/tests/async_provider.rs b/datafusion/ffi/src/tests/async_provider.rs index 83057d8c45db3..99a61e612204c 100644 --- a/datafusion/ffi/src/tests/async_provider.rs +++ b/datafusion/ffi/src/tests/async_provider.rs @@ -75,7 +75,7 @@ fn async_table_provider_thread( runtime.block_on(async move { let mut num_received = 0; - while let Some(true) = batch_request.recv().await { + while batch_request.recv().await == Some(true) { let record_batch = match num_received { 0 => Some(create_record_batch(1, 5)), 1 => Some(create_record_batch(6, 1)), diff --git a/datafusion/functions-aggregate-common/src/aggregate/groups_accumulator/accumulate.rs b/datafusion/functions-aggregate-common/src/aggregate/groups_accumulator/accumulate.rs index 09e1df4eae70c..a5ed75f346167 100644 --- a/datafusion/functions-aggregate-common/src/aggregate/groups_accumulator/accumulate.rs +++ b/datafusion/functions-aggregate-common/src/aggregate/groups_accumulator/accumulate.rs @@ -264,7 +264,7 @@ impl NullState { .zip(data.iter()) .zip(filter.iter()) .for_each(|((&group_index, new_value), filter_value)| { - if let Some(true) = filter_value { + if filter_value == Some(true) { seen_values.set_bit(group_index, true); value_fn(group_index, new_value); } @@ -278,7 +278,7 @@ impl NullState { .zip(group_indices.iter()) .zip(values.iter()) .for_each(|((filter_value, &group_index), new_value)| { - if let Some(true) = filter_value + if filter_value == Some(true) && let Some(new_value) = new_value { seen_values.set_bit(group_index, true); @@ -442,7 +442,7 @@ pub fn accumulate( .zip(data.iter()) .zip(filter.iter()) .for_each(|((&group_index, &new_value), filter_value)| { - if let Some(true) = filter_value { + if filter_value == Some(true) { value_fn(group_index, new_value); } }) @@ -458,7 +458,7 @@ pub fn accumulate( .zip(group_indices.iter()) .zip(values.iter()) .for_each(|((filter_value, &group_index), new_value)| { - if let Some(true) = filter_value + if filter_value == Some(true) && let Some(new_value) = new_value { value_fn(group_index, new_value) @@ -903,7 +903,7 @@ mod test { .zip(filter.iter()) .for_each(|((&group_index, value), is_included)| { // if value passed filter - if let Some(true) = is_included + if is_included == Some(true) && let Some(value) = value { mock.saw_value(group_index); @@ -966,7 +966,7 @@ mod test { ), (None, Some(filter)) => group_indices.iter().zip(filter.iter()).for_each( |(&group_index, is_included)| { - if let Some(true) = is_included { + if is_included == Some(true) { expected_values.push(group_index); } }, @@ -978,7 +978,7 @@ mod test { .zip(filter.iter()) .for_each(|((&group_index, is_valid), is_included)| { // if value passed filter - if let (true, Some(true)) = (is_valid, is_included) { + if is_valid && is_included == Some(true) { expected_values.push(group_index); } }); @@ -1032,7 +1032,7 @@ mod test { .zip(filter.iter()) .for_each(|((&group_index, value), is_included)| { // if value passed filter - if let Some(true) = is_included + if is_included == Some(true) && let Some(value) = value { mock.saw_value(group_index); diff --git a/datafusion/functions-aggregate/src/variance.rs b/datafusion/functions-aggregate/src/variance.rs index b8e52f849a7cc..a2ec83057874e 100644 --- a/datafusion/functions-aggregate/src/variance.rs +++ b/datafusion/functions-aggregate/src/variance.rs @@ -409,7 +409,7 @@ impl Accumulator for VarianceAccumulator { Ok(ScalarValue::Float64(match self.count { 0 => None, 1 => { - if let StatsType::Population = self.stats_type { + if self.stats_type == StatsType::Population { Some(0.0) } else { None @@ -486,7 +486,7 @@ impl VarianceGroupsAccumulator { let _ = emit_to.take_needed(&mut self.means); let m2s = emit_to.take_needed(&mut self.m2s); - if let StatsType::Sample = self.stats_type { + if self.stats_type == StatsType::Sample { counts.iter_mut().for_each(|count| { *count = count.saturating_sub(1); }); diff --git a/datafusion/physical-expr/src/equivalence/class.rs b/datafusion/physical-expr/src/equivalence/class.rs index 1f9a6a583cc44..c016944f76bec 100644 --- a/datafusion/physical-expr/src/equivalence/class.rs +++ b/datafusion/physical-expr/src/equivalence/class.rs @@ -362,7 +362,7 @@ impl EquivalenceGroup { let (mut idx, mut change) = (0, false); while idx < self.classes.len() { let cls = &mut self.classes[idx]; - if let Some(AcrossPartitions::Heterogeneous) = cls.constant { + if cls.constant == Some(AcrossPartitions::Heterogeneous) { change = true; if cls.len() == 1 { // If this class becomes trivial, remove it entirely: diff --git a/datafusion/physical-expr/src/simplifier/not.rs b/datafusion/physical-expr/src/simplifier/not.rs index 886cadd6a262d..7496eda6c4d30 100644 --- a/datafusion/physical-expr/src/simplifier/not.rs +++ b/datafusion/physical-expr/src/simplifier/not.rs @@ -69,7 +69,7 @@ pub fn simplify_not_expr( if let ScalarValue::Boolean(Some(val)) = literal.value() { return Ok(Transformed::yes(lit(ScalarValue::Boolean(Some(!val))))); } - if let ScalarValue::Boolean(None) = literal.value() { + if matches!(literal.value(), ScalarValue::Boolean(None)) { return Ok(Transformed::yes(lit(ScalarValue::Boolean(None)))); } } diff --git a/datafusion/pruning/src/pruning_predicate.rs b/datafusion/pruning/src/pruning_predicate.rs index 840fe9b699688..5d949d6f1efaa 100644 --- a/datafusion/pruning/src/pruning_predicate.rs +++ b/datafusion/pruning/src/pruning_predicate.rs @@ -755,7 +755,7 @@ impl BoolVecBuilder { // `false` for this conjunct means we know for sure no rows could // pass the predicate and thus we set the corresponding container // location to false. - if let Some(false) = new { + if new == Some(false) { *cur = false; } } diff --git a/datafusion/spark/src/function/url/parse_url.rs b/datafusion/spark/src/function/url/parse_url.rs index 9ceed8b155bbd..1a20cdc10b44a 100644 --- a/datafusion/spark/src/function/url/parse_url.rs +++ b/datafusion/spark/src/function/url/parse_url.rs @@ -81,7 +81,7 @@ impl ParseUrl { /// * `Err(DataFusionError)` - If the URL is malformed and cannot be parsed fn parse(value: &str, part: &str, key: Option<&str>) -> Result> { let url: std::result::Result = Url::parse(value); - if let Err(ParseError::RelativeUrlWithoutBase) = url { + if url == Err(ParseError::RelativeUrlWithoutBase) { return if !value.contains("://") { // Schemeless URLs are treated as relative URIs (like java.net.URI). // Manually parse path, query, and fragment components. From 685ea7037c93e3429936c19b7c5a12be29ba3679 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 12:21:30 +0200 Subject: [PATCH 33/42] feat: enable clippy::explicit_deref_methods `x.deref()` and `x.deref_mut()` are the operator spelled the long way; `&*x` / `&mut *x` is the idiomatic form and does not need `Deref` in scope. Co-Authored-By: Claude Opus 5 (1M context) --- Cargo.toml | 1 + .../provider_filter_pushdown.rs | 3 +-- .../core/tests/execution/logical_plan.rs | 2 +- .../core/tests/tracing/asserting_tracer.rs | 3 +-- datafusion/execution/src/async_stream.rs | 5 ++--- datafusion/ffi/src/udaf/accumulator.rs | 3 +-- datafusion/ffi/src/udaf/groups_accumulator.rs | 3 +-- datafusion/optimizer/src/decorrelate.rs | 20 ++++++++----------- .../src/decorrelate_predicate_subquery.rs | 4 ++-- .../src/repartition/distributor_channels.rs | 7 +++---- 10 files changed, 21 insertions(+), 30 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 69f7e3e4a0d51..011b3aea85b11 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -233,6 +233,7 @@ empty_enums = "warn" equatable_if_let = "warn" exit = "warn" expl_impl_clone_on_copy = "warn" +explicit_deref_methods = "warn" # Catches `From` impls that can panic fallible_impl_from = "warn" filter_map_next = "warn" diff --git a/datafusion/core/tests/custom_sources_cases/provider_filter_pushdown.rs b/datafusion/core/tests/custom_sources_cases/provider_filter_pushdown.rs index a8f7f09ad016b..10c91f0432a69 100644 --- a/datafusion/core/tests/custom_sources_cases/provider_filter_pushdown.rs +++ b/datafusion/core/tests/custom_sources_cases/provider_filter_pushdown.rs @@ -15,7 +15,6 @@ // specific language governing permissions and limitations // under the License. -use std::ops::Deref; use std::sync::Arc; use arrow::array::{Int32Builder, Int64Array}; @@ -204,7 +203,7 @@ impl TableProvider for CustomProvider { Expr::Literal(ScalarValue::Int16(Some(i)), _) => *i as i64, Expr::Literal(ScalarValue::Int32(Some(i)), _) => *i as i64, Expr::Literal(ScalarValue::Int64(Some(i)), _) => *i, - Expr::Cast(Cast { expr, field: _ }) => match expr.deref() { + Expr::Cast(Cast { expr, field: _ }) => match &**expr { Expr::Literal(lit_value, _) => match lit_value { ScalarValue::Int8(Some(v)) => *v as i64, ScalarValue::Int16(Some(v)) => *v as i64, diff --git a/datafusion/core/tests/execution/logical_plan.rs b/datafusion/core/tests/execution/logical_plan.rs index 3eaa3fb2ed5e6..79d8b306106b4 100644 --- a/datafusion/core/tests/execution/logical_plan.rs +++ b/datafusion/core/tests/execution/logical_plan.rs @@ -86,7 +86,7 @@ async fn count_only_nulls() -> Result<()> { let column = only(result.columns()); assert_eq!(field.data_type(), &DataType::Int64); // TODO should be UInt64 - assert_eq!(column.deref(), &Int64Array::from(vec![0])); + assert_eq!(&**column, &Int64Array::from(vec![0])); Ok(()) } diff --git a/datafusion/core/tests/tracing/asserting_tracer.rs b/datafusion/core/tests/tracing/asserting_tracer.rs index 700f9f3308466..a73a7f28c8125 100644 --- a/datafusion/core/tests/tracing/asserting_tracer.rs +++ b/datafusion/core/tests/tracing/asserting_tracer.rs @@ -17,7 +17,6 @@ use std::any::Any; use std::collections::VecDeque; -use std::ops::Deref; use std::sync::{Arc, LazyLock}; use datafusion_common::{HashMap, HashSet}; @@ -28,7 +27,7 @@ use tokio::sync::{Mutex, MutexGuard}; /// Initializes the global join set tracer with the asserting tracer. /// Call this function before spawning any tasks that should be traced. pub fn init_asserting_tracer() { - set_join_set_tracer(ASSERTING_TRACER.deref()) + set_join_set_tracer(&*ASSERTING_TRACER) .expect("Failed to initialize asserting tracer"); } diff --git a/datafusion/execution/src/async_stream.rs b/datafusion/execution/src/async_stream.rs index 7ca6ba4850cab..0462c53a0ffd3 100644 --- a/datafusion/execution/src/async_stream.rs +++ b/datafusion/execution/src/async_stream.rs @@ -20,7 +20,6 @@ use futures::future::FusedFuture; use futures::stream::FusedStream; use parking_lot::Mutex; use pin_project_lite::pin_project; -use std::ops::DerefMut; use std::pin::Pin; use std::sync::Arc; use std::task::{Context, Poll}; @@ -181,7 +180,7 @@ impl Emitter { /// nothing yields control back to the consumer in between. fn set(&mut self, value: T) { let mut guard = self.slot.lock(); - match guard.deref_mut() { + match &mut *guard { Some(_) => panic!("Misuse: await was not called after calling emit"), slot => *slot = Some(value), } @@ -200,7 +199,7 @@ impl TryEmitter { /// Panics if called before the previous emit future has been awaited. pub fn emit(&mut self, value: T) -> impl FusedFuture { let mut guard = self.slot.lock(); - match guard.deref_mut() { + match &mut *guard { Some(_) => panic!("Misuse: await was not called after calling emit"), slot => *slot = Some(Ok::(value)), } diff --git a/datafusion/ffi/src/udaf/accumulator.rs b/datafusion/ffi/src/udaf/accumulator.rs index d08567d369476..4d696cadb70e3 100644 --- a/datafusion/ffi/src/udaf/accumulator.rs +++ b/datafusion/ffi/src/udaf/accumulator.rs @@ -17,7 +17,6 @@ use std::any::Any; use std::ffi::c_void; -use std::ops::Deref; use std::ptr::null_mut; use arrow::array::ArrayRef; @@ -96,7 +95,7 @@ impl FFI_Accumulator { unsafe fn inner(&self) -> &dyn Accumulator { unsafe { let private_data = self.private_data as *const AccumulatorPrivateData; - (*private_data).accumulator.deref() + &*(*private_data).accumulator } } } diff --git a/datafusion/ffi/src/udaf/groups_accumulator.rs b/datafusion/ffi/src/udaf/groups_accumulator.rs index 840787126d90c..ad2714da0cc60 100644 --- a/datafusion/ffi/src/udaf/groups_accumulator.rs +++ b/datafusion/ffi/src/udaf/groups_accumulator.rs @@ -17,7 +17,6 @@ use std::any::Any; use std::ffi::c_void; -use std::ops::Deref; use std::ptr::null_mut; use std::sync::Arc; @@ -103,7 +102,7 @@ impl FFI_GroupsAccumulator { unsafe fn inner(&self) -> &dyn GroupsAccumulator { unsafe { let private_data = self.private_data as *const GroupsAccumulatorPrivateData; - (*private_data).accumulator.deref() + &*(*private_data).accumulator } } } diff --git a/datafusion/optimizer/src/decorrelate.rs b/datafusion/optimizer/src/decorrelate.rs index 9490af0e59749..2be058e4e3d52 100644 --- a/datafusion/optimizer/src/decorrelate.rs +++ b/datafusion/optimizer/src/decorrelate.rs @@ -18,7 +18,6 @@ //! [`PullUpCorrelatedExpr`] converts correlated subqueries to `Joins` use std::collections::BTreeSet; -use std::ops::Deref; use std::sync::Arc; use crate::simplify_expressions::ExprSimplifier; @@ -199,7 +198,7 @@ impl TreeNodeRewriter for PullUpCorrelatedExpr { let mut expr_result_map_for_count_bug = HashMap::new(); let pull_up_expr_opt = if let Some(expr_result_map) = - self.collected_count_expr_map.get(plan_filter.input.deref()) + self.collected_count_expr_map.get(&*plan_filter.input) { if let Some(expr) = conjunction(subquery_filters.clone()) { filter_exprs_evaluation_result_on_empty_batch( @@ -258,7 +257,7 @@ impl TreeNodeRewriter for PullUpCorrelatedExpr { let mut expr_result_map_for_count_bug = HashMap::new(); if let Some(expr_result_map) = - self.collected_count_expr_map.get(projection.input.deref()) + self.collected_count_expr_map.get(&*projection.input) { proj_exprs_evaluation_result_on_empty_batch( &projection.expr, @@ -352,8 +351,7 @@ impl TreeNodeRewriter for PullUpCorrelatedExpr { } self.correlated_subquery_cols_map .insert(plan.clone(), new_correlated_cols); - if let Some(input_map) = - self.collected_count_expr_map.get(alias.input.deref()) + if let Some(input_map) = self.collected_count_expr_map.get(&*alias.input) { self.collected_count_expr_map .insert(plan.clone(), input_map.clone()); @@ -361,10 +359,8 @@ impl TreeNodeRewriter for PullUpCorrelatedExpr { Ok(Transformed::no(plan)) } LogicalPlan::Limit(limit) => { - let input_expr_map = self - .collected_count_expr_map - .get(limit.input.deref()) - .cloned(); + let input_expr_map = + self.collected_count_expr_map.get(&*limit.input).cloned(); // handling the limit clause in the subquery let new_plan = match (self.exists_sub_query, self.join_filters.is_empty()) { @@ -432,16 +428,16 @@ fn can_pullup_over_aggregation(expr: &Expr) -> bool { right, }) = expr { - match (left.deref(), right.deref()) { + match (&**left, &**right) { (Expr::Column(_), right) => !right.any_column_refs(), (left, Expr::Column(_)) => !left.any_column_refs(), (Expr::Cast(Cast { expr, .. }), right) - if matches!(expr.deref(), Expr::Column(_)) => + if matches!(&**expr, Expr::Column(_)) => { !right.any_column_refs() } (left, Expr::Cast(Cast { expr, .. })) - if matches!(expr.deref(), Expr::Column(_)) => + if matches!(&**expr, Expr::Column(_)) => { !left.any_column_refs() } diff --git a/datafusion/optimizer/src/decorrelate_predicate_subquery.rs b/datafusion/optimizer/src/decorrelate_predicate_subquery.rs index 0609109ec6e58..66ea0806bd3b6 100644 --- a/datafusion/optimizer/src/decorrelate_predicate_subquery.rs +++ b/datafusion/optimizer/src/decorrelate_predicate_subquery.rs @@ -394,7 +394,7 @@ fn build_join( right, })), ) => { - let right_col = create_col_from_scalar_expr(right.deref(), alias)?; + let right_col = create_col_from_scalar_expr(&right, alias)?; let in_predicate = Expr::eq(left.deref().clone(), Expr::Column(right_col)); in_predicate.and(join_filter) } @@ -407,7 +407,7 @@ fn build_join( right, })), ) => { - let right_col = create_col_from_scalar_expr(right.deref(), alias)?; + let right_col = create_col_from_scalar_expr(&right, alias)?; Expr::eq(left.deref().clone(), Expr::Column(right_col)) } diff --git a/datafusion/physical-plan/src/repartition/distributor_channels.rs b/datafusion/physical-plan/src/repartition/distributor_channels.rs index 4880561c12134..fb86aafadd7ad 100644 --- a/datafusion/physical-plan/src/repartition/distributor_channels.rs +++ b/datafusion/physical-plan/src/repartition/distributor_channels.rs @@ -40,7 +40,6 @@ use std::{ collections::VecDeque, future::Future, - ops::DerefMut, pin::Pin, sync::{ Arc, @@ -220,7 +219,7 @@ impl Future for SendFuture<'_, T> { // if so, allow sender to create another if this.gate.empty_channels.load(Ordering::SeqCst) == 0 { let mut guard = this.gate.send_wakers.lock(); - if let Some(send_wakers) = guard.deref_mut() { + if let Some(send_wakers) = &mut *guard { send_wakers.push((cx.waker().clone(), this.channel.id)); return Poll::Pending; } @@ -298,7 +297,7 @@ impl Future for RecvFuture<'_, T> { assert!(!this.rdy, "polled ready future"); let mut guard_channel_state = this.channel.state.lock(); - let channel_state = guard_channel_state.deref_mut(); + let channel_state = &mut *guard_channel_state; let data = channel_state.data.as_mut().expect("not dropped yet"); match data.pop_front() { @@ -431,7 +430,7 @@ impl Gate { let to_wake = { let mut guard = self.send_wakers.lock(); - if let Some(send_wakers) = guard.deref_mut() { + if let Some(send_wakers) = &mut *guard { // `drain_filter` is unstable, so implement our own let (wake, keep) = send_wakers.drain(..).partition(|(_waker, id2)| id == *id2); From 65ef16ee1f86f8009502d8febe7c56fe1f136864 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 13:42:27 +0200 Subject: [PATCH 34/42] Revert "feat: enable clippy::unnecessary_safety_comment" This reverts commit c7e25ffcb9bda9c62ba6d078f425659213a68b17. --- Cargo.toml | 1 - datafusion/datasource-json/src/source.rs | 2 +- datafusion/functions-aggregate/src/percentile_cont.rs | 2 +- datafusion/functions/src/string/repeat.rs | 2 +- datafusion/functions/src/unicode/character_length.rs | 2 +- datafusion/optimizer/src/extract_equijoin_predicate.rs | 2 +- datafusion/physical-expr-common/src/binary_map.rs | 2 +- datafusion/physical-plan/src/sorts/cursor.rs | 5 +---- datafusion/spark/src/function/string/length.rs | 2 +- 9 files changed, 8 insertions(+), 12 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 011b3aea85b11..fc2380f0fca27 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -325,7 +325,6 @@ unnecessary_box_returns = "warn" # `{:?}` on a `Path` quotes and escapes it; `{}` on `.display()` does not unnecessary_debug_formatting = "warn" unnecessary_lazy_evaluations = "warn" -unnecessary_safety_comment = "warn" unnecessary_safety_doc = "warn" unnecessary_self_imports = "warn" unnecessary_struct_initialization = "warn" diff --git a/datafusion/datasource-json/src/source.rs b/datafusion/datasource-json/src/source.rs index e8dc41cff3c57..47241c9d99ab5 100644 --- a/datafusion/datasource-json/src/source.rs +++ b/datafusion/datasource-json/src/source.rs @@ -66,7 +66,7 @@ const JSON_CONVERTER_BUFFER_SIZE: usize = 2 * 1024 * 1024; /// A stream wrapper that holds SpawnedTask handles to keep them alive /// until the stream is fully consumed or dropped. /// -/// This ensures cancel-safety. When the stream is dropped, the tasks +/// This ensures cancel-safety: when the stream is dropped, the tasks /// are properly aborted via SpawnedTask's Drop implementation. struct JsonArrayStream { inner: ReceiverStream>, diff --git a/datafusion/functions-aggregate/src/percentile_cont.rs b/datafusion/functions-aggregate/src/percentile_cont.rs index fae6fcf2d2007..3a98900bbb446 100644 --- a/datafusion/functions-aggregate/src/percentile_cont.rs +++ b/datafusion/functions-aggregate/src/percentile_cont.rs @@ -63,7 +63,7 @@ use crate::utils::validate_percentile_expr; /// Precision multiplier for linear interpolation calculations. /// -/// This value of 1,000,000 was chosen to balance precision against overflow: +/// This value of 1,000,000 was chosen to balance precision with overflow safety: /// - Provides 6 decimal places of precision for the fractional component /// - Small enough to avoid overflow when multiplied with typical numeric values /// - Sufficient precision for most statistical applications diff --git a/datafusion/functions/src/string/repeat.rs b/datafusion/functions/src/string/repeat.rs index 877ff98b7a92f..a53f1e2e4fc42 100644 --- a/datafusion/functions/src/string/repeat.rs +++ b/datafusion/functions/src/string/repeat.rs @@ -306,7 +306,7 @@ where // Doubling strategy: copy what we have so far until we reach the target while buffer.len() < src.len() * count { let copy_len = buffer.len().min(src.len() * count - buffer.len()); - // We're copying valid UTF-8 bytes that we already verified + // SAFETY: we're copying valid UTF-8 bytes that we already verified buffer.extend_from_within(..copy_len); } } diff --git a/datafusion/functions/src/unicode/character_length.rs b/datafusion/functions/src/unicode/character_length.rs index 85d9595c4605a..9f0d952a02636 100644 --- a/datafusion/functions/src/unicode/character_length.rs +++ b/datafusion/functions/src/unicode/character_length.rs @@ -158,10 +158,10 @@ where } else { let values: Vec<_> = (0..array.len()) .map(|i| { + // Safety: we are iterating with array.len() so the index is always valid if array.is_null(i) { T::default_value() } else { - // SAFETY: we are iterating with array.len() so the index is always valid let value = unsafe { array.value_unchecked(i) }; if value.is_empty() { T::default_value() diff --git a/datafusion/optimizer/src/extract_equijoin_predicate.rs b/datafusion/optimizer/src/extract_equijoin_predicate.rs index 58f9a4cd42a2d..0a50761e8a9f7 100644 --- a/datafusion/optimizer/src/extract_equijoin_predicate.rs +++ b/datafusion/optimizer/src/extract_equijoin_predicate.rs @@ -95,7 +95,7 @@ impl OptimizerRule for ExtractEquijoinPredicate { && equijoin_predicates.is_empty() && non_equijoin_expr.is_some() { - // Checked in the outer `if` + // SAFETY: checked in the outer `if` let expr = non_equijoin_expr.clone().unwrap(); let (equijoin_predicates, non_equijoin_expr) = split_is_not_distinct_from_and_other_join_predicate( diff --git a/datafusion/physical-expr-common/src/binary_map.rs b/datafusion/physical-expr-common/src/binary_map.rs index 1ae272380dcba..a95d13ee867bd 100644 --- a/datafusion/physical-expr-common/src/binary_map.rs +++ b/datafusion/physical-expr-common/src/binary_map.rs @@ -556,7 +556,7 @@ fn single_null_buffer(num_values: usize, null_index: usize) -> NullBuffer { null_builder.append_n_non_nulls(null_index); null_builder.append_null(); null_builder.append_n_non_nulls(num_values - null_index - 1); - // The inner builder must be constructed + // SAFETY: inner builder must be constructed null_builder.finish().unwrap() } diff --git a/datafusion/physical-plan/src/sorts/cursor.rs b/datafusion/physical-plan/src/sorts/cursor.rs index 22037620e2efa..003de2375ad3f 100644 --- a/datafusion/physical-plan/src/sorts/cursor.rs +++ b/datafusion/physical-plan/src/sorts/cursor.rs @@ -460,19 +460,16 @@ impl CursorValues for StringViewArray { #[inline(always)] fn compare(l: &Self, l_idx: usize, r: &Self, r_idx: usize) -> Ordering { - // Prior assertions guarantee that l_idx and r_idx are valid indices. + // SAFETY: Prior assertions guarantee that l_idx and r_idx are valid indices. // Null-checks are assumed to have been handled in the wrapper (e.g., ArrayValues). // And the bound is checked in is_finished, it is safe to call get_unchecked if l.data_buffers().is_empty() && r.data_buffers().is_empty() { - // SAFETY: see above let l_view = unsafe { l.views().get_unchecked(l_idx) }; - // SAFETY: see above let r_view = unsafe { r.views().get_unchecked(r_idx) }; return StringViewArray::inline_key_fast(*l_view) .cmp(&StringViewArray::inline_key_fast(*r_view)); } - // SAFETY: see above unsafe { GenericByteViewArray::compare_unchecked(l, l_idx, r, r_idx) } } diff --git a/datafusion/spark/src/function/string/length.rs b/datafusion/spark/src/function/string/length.rs index 57f40583d4f26..8c5539a0577d8 100644 --- a/datafusion/spark/src/function/string/length.rs +++ b/datafusion/spark/src/function/string/length.rs @@ -154,10 +154,10 @@ where } else { let values: Vec<_> = (0..array.len()) .map(|i| { + // Safety: we are iterating with array.len() so the index is always valid if array.is_null(i) { i32::default() } else { - // SAFETY: we are iterating with array.len() so the index is always valid let value = unsafe { array.value_unchecked(i) }; if value.is_empty() { i32::default() From 76bde3a1a198a9dbba3c263e6a3ba2bf3be41572 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 13:45:26 +0200 Subject: [PATCH 35/42] refactor: use `==` instead of `matches!` where the values are comparable Addresses review feedback on the `equatable_if_let` commit: clippy suggested `matches!` in a few places where a plain equality check reads better. `predicate_bounds.rs` uses `.ok() == Some(false)` because `DataFusionError` does not implement `PartialEq`, so `== Ok(false)` does not compile. Co-Authored-By: Claude Opus 5 (1M context) --- .../core/tests/user_defined/user_defined_scalar_functions.rs | 2 +- datafusion/expr/src/predicate_bounds.rs | 2 +- datafusion/pruning/src/pruning_predicate.rs | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/datafusion/core/tests/user_defined/user_defined_scalar_functions.rs b/datafusion/core/tests/user_defined/user_defined_scalar_functions.rs index 9458bbd02cef0..da71ffb10eaa7 100644 --- a/datafusion/core/tests/user_defined/user_defined_scalar_functions.rs +++ b/datafusion/core/tests/user_defined/user_defined_scalar_functions.rs @@ -1872,7 +1872,7 @@ impl ExtensionType for MyUserExtensionType { &self, data_type: &DataType, ) -> std::result::Result<(), ArrowError> { - if matches!(data_type, DataType::Utf8) { + if *data_type == DataType::Utf8 { Ok(()) } else { Err(ArrowError::InvalidArgumentError( diff --git a/datafusion/expr/src/predicate_bounds.rs b/datafusion/expr/src/predicate_bounds.rs index 13631b2e7c4b2..aa947416c87b5 100644 --- a/datafusion/expr/src/predicate_bounds.rs +++ b/datafusion/expr/src/predicate_bounds.rs @@ -166,7 +166,7 @@ impl PredicateBoundsEvaluator<'_> { } // If `expr` is not nullable, we can be certain `expr` is not null - if matches!(expr.nullable(self.input_schema), Ok(false)) { + if expr.nullable(self.input_schema).ok() == Some(false) { return NullableInterval::FALSE; } diff --git a/datafusion/pruning/src/pruning_predicate.rs b/datafusion/pruning/src/pruning_predicate.rs index 5d949d6f1efaa..6df5cbc5cc135 100644 --- a/datafusion/pruning/src/pruning_predicate.rs +++ b/datafusion/pruning/src/pruning_predicate.rs @@ -795,12 +795,12 @@ impl BoolVecBuilder { fn is_always_true(expr: &Arc) -> bool { expr.downcast_ref::() - .is_some_and(|l| matches!(l.value(), ScalarValue::Boolean(Some(true)))) + .is_some_and(|l| *l.value() == ScalarValue::Boolean(Some(true))) } fn is_always_false(expr: &Arc) -> bool { expr.downcast_ref::() - .is_some_and(|l| matches!(l.value(), ScalarValue::Boolean(Some(false)))) + .is_some_and(|l| *l.value() == ScalarValue::Boolean(Some(false))) } /// Describes which columns statistics are necessary to evaluate a From 0d0bad881a54f7fa1e5f4d60059f61d5a8bb7d49 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 14:01:21 +0200 Subject: [PATCH 36/42] Revert "feat: enable clippy::float_cmp_const" This reverts commit d82dc41e0b7e3ce31493f4b28db6c8d183b3839a. --- Cargo.toml | 1 - datafusion/expr-common/src/interval_arithmetic.rs | 8 -------- 2 files changed, 9 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index fc2380f0fca27..c7ba4b2fc0c2c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -238,7 +238,6 @@ explicit_deref_methods = "warn" fallible_impl_from = "warn" filter_map_next = "warn" flat_map_option = "warn" -float_cmp_const = "warn" fn_params_excessive_bools = "warn" fn_to_numeric_cast_any = "warn" ignore_without_reason = "warn" diff --git a/datafusion/expr-common/src/interval_arithmetic.rs b/datafusion/expr-common/src/interval_arithmetic.rs index 0f18a591bb369..68541e1e6b32c 100644 --- a/datafusion/expr-common/src/interval_arithmetic.rs +++ b/datafusion/expr-common/src/interval_arithmetic.rs @@ -110,15 +110,7 @@ macro_rules! value_transition { Int16(Some(value)) if value == i16::$bound => Int16(None), Int32(Some(value)) if value == i32::$bound => Int32(None), Int64(Some(value)) if value == i64::$bound => Int64(None), - #[expect( - clippy::float_cmp_const, - reason = "We really do want to detect the exact bound here" - )] Float32(Some(value)) if value == f32::$bound => Float32(None), - #[expect( - clippy::float_cmp_const, - reason = "We really do want to detect the exact bound here" - )] Float64(Some(value)) if value == f64::$bound => Float64(None), DurationSecond(Some(value)) if value == i64::$bound => DurationSecond(None), DurationMillisecond(Some(value)) if value == i64::$bound => { From 92ae8a00147eb868eeefe528732f9aed9d2ebe6b Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 14:01:42 +0200 Subject: [PATCH 37/42] Revert "feat: enable clippy::lossy_float_literal" This reverts commit 36abeb74d28d24a5d40892fa38497763e34a9851. --- Cargo.toml | 1 - datafusion/common/src/scalar/mod.rs | 4 ---- 2 files changed, 5 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index c7ba4b2fc0c2c..d4c2f2e961eda 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -266,7 +266,6 @@ large_types_passed_by_value = "warn" linkedlist = "warn" # Catches `"{foo}"` where the string is never actually formatted literal_string_with_formatting_args = "warn" -lossy_float_literal = "warn" macro_use_imports = "warn" manual_assert = "warn" manual_ilog2 = "warn" diff --git a/datafusion/common/src/scalar/mod.rs b/datafusion/common/src/scalar/mod.rs index 4e7d48dd67437..c156f59147506 100644 --- a/datafusion/common/src/scalar/mod.rs +++ b/datafusion/common/src/scalar/mod.rs @@ -9628,10 +9628,6 @@ mod tests { } #[test] - #[expect( - clippy::lossy_float_literal, - reason = "The literals below spell out exact powers of two, which float `Display` renders differently" - )] fn test_scalar_distance_u64_boundaries() { // 1. Full-domain integer ranges // i64::MIN to i64::MAX -> distance is u64::MAX From 42ef8b18be665c1f70d69f22a1aa6f389ada00c1 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 14:04:37 +0200 Subject: [PATCH 38/42] Revert "feat: enable clippy::fallible_impl_from" This reverts commit 85c06c63346833b9bcef556b8fb2897d0c9480c9. --- Cargo.toml | 2 -- datafusion/common/src/scalar/mod.rs | 4 ---- datafusion/proto-common/src/from_proto/mod.rs | 8 -------- 3 files changed, 14 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index d4c2f2e961eda..484eb6eebc906 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -234,8 +234,6 @@ equatable_if_let = "warn" exit = "warn" expl_impl_clone_on_copy = "warn" explicit_deref_methods = "warn" -# Catches `From` impls that can panic -fallible_impl_from = "warn" filter_map_next = "warn" flat_map_option = "warn" fn_params_excessive_bools = "warn" diff --git a/datafusion/common/src/scalar/mod.rs b/datafusion/common/src/scalar/mod.rs index c156f59147506..bef46702b85ea 100644 --- a/datafusion/common/src/scalar/mod.rs +++ b/datafusion/common/src/scalar/mod.rs @@ -5353,10 +5353,6 @@ impl From> for ScalarValue { } /// Wrapper to create ScalarValue::Struct for convenience -#[expect( - clippy::fallible_impl_from, - reason = "Making this fallible would be a breaking API change" -)] impl From> for ScalarValue { fn from(value: Vec<(&str, ScalarValue)>) -> Self { value diff --git a/datafusion/proto-common/src/from_proto/mod.rs b/datafusion/proto-common/src/from_proto/mod.rs index 67ff95a40d0a7..169ff7f3d9ff2 100644 --- a/datafusion/proto-common/src/from_proto/mod.rs +++ b/datafusion/proto-common/src/from_proto/mod.rs @@ -750,10 +750,6 @@ impl From for Constraints { } } -#[expect( - clippy::fallible_impl_from, - reason = "Making this fallible would be a breaking API change" -)] impl From for Constraint { fn from(value: protobuf::Constraint) -> Self { match value.constraint_mode.unwrap() { @@ -886,10 +882,6 @@ impl From for JoinSide { } } -#[expect( - clippy::fallible_impl_from, - reason = "Making this fallible would be a breaking API change" -)] impl From<&protobuf::Constraint> for Constraint { fn from(value: &protobuf::Constraint) -> Self { match &value.constraint_mode { From f65d360e150c650f2f483285e8d9fa5abd0f9e65 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 14:12:46 +0200 Subject: [PATCH 39/42] refactor: address review feedback on duration units and the ignored test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * `test_parse_duration_with_overflow_check` uses `Duration::from_mins` for the `"…m"` input again, so the constructor mirrors the unit suffix in the string being parsed. That trips `duration_suboptimal_units`, so the test gets an `#[expect]` saying why. * Restore the `TODO`/`Issue` comments above the ignored `sort_with_mem_limit_2_cols_2` test, keeping a short `#[ignore]` reason. Co-Authored-By: Claude Opus 5 (1M context) --- datafusion/core/src/execution/context/mod.rs | 6 +++++- .../memory_limit_validation/sort_mem_validation.rs | 4 +++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/datafusion/core/src/execution/context/mod.rs b/datafusion/core/src/execution/context/mod.rs index 0c05aac874f59..78497604da56c 100644 --- a/datafusion/core/src/execution/context/mod.rs +++ b/datafusion/core/src/execution/context/mod.rs @@ -2981,6 +2981,10 @@ mod tests { } #[test] + #[expect( + clippy::duration_suboptimal_units, + reason = "Each `Duration` deliberately uses the same unit as the suffix in the string it is parsed from" + )] fn test_parse_duration_with_overflow_check() { const LIST_FILES_CACHE_TTL: &str = "datafusion.runtime.list_files_cache_ttl"; @@ -2992,7 +2996,7 @@ mod tests { ), ( "307445734561825860m", - Duration::from_hours(5124095576030431), + Duration::from_mins(307445734561825860), ), ( "307445734561825860m10s", diff --git a/datafusion/core/tests/memory_limit/memory_limit_validation/sort_mem_validation.rs b/datafusion/core/tests/memory_limit/memory_limit_validation/sort_mem_validation.rs index 766312567ce41..689915f4aa9db 100644 --- a/datafusion/core/tests/memory_limit/memory_limit_validation/sort_mem_validation.rs +++ b/datafusion/core/tests/memory_limit/memory_limit_validation/sort_mem_validation.rs @@ -161,7 +161,9 @@ async fn sort_with_mem_limit_2_cols_1() { .await; } -#[ignore = "Query fails, see https://github.com/apache/datafusion/issues/14143"] +// TODO: Query fails, fix it +// Issue: https://github.com/apache/datafusion/issues/14143 +#[ignore = "Query fails, see the issue above"] #[tokio::test] async fn sort_with_mem_limit_2_cols_2() { let memory_usage_in_theory = 80_000_000 * 2; // 2 columns From 1755ad146fcaea2a5f62a91faa5ddea10d9d8886 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 14:15:44 +0200 Subject: [PATCH 40/42] fix: apply iter_on_single_items to a new site from main `ab12f5e4b` ("fix(ffi): preserve TableProvider DML overrides") landed on main after this branch was measured and added a new `[x].into_iter()`, which the `iter_on_single_items` lint enabled here rejects. CI builds the merge commit, so it failed there but not locally. Co-Authored-By: Claude Opus 5 (1M context) --- datafusion/ffi/src/table_provider.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/datafusion/ffi/src/table_provider.rs b/datafusion/ffi/src/table_provider.rs index f1a79added0ab..44e82f1fc9b56 100644 --- a/datafusion/ffi/src/table_provider.rs +++ b/datafusion/ffi/src/table_provider.rs @@ -1061,11 +1061,10 @@ mod tests { let session = FFI_SessionRef::new(&state, None, ffi_provider.logical_codec.clone()); - let assignments = [FFI_TableProviderUpdateAssignment { + let assignments = std::iter::once(FFI_TableProviderUpdateAssignment { column: SString::from("b"), expr_serialized: SVec::new(), - }] - .into_iter() + }) .collect(); let result = unsafe { (ffi_provider.update)(&ffi_provider, session, assignments, SVec::new()).await From be274ef009b54ae8dcaffc301eaabc59d8291794 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 13 Aug 2026 14:19:43 +0200 Subject: [PATCH 41/42] Revert "feat: enable clippy::large_digit_groups" This reverts commit fddce2011ecab52053ade2ea6faf2d36aaaeec74. --- Cargo.toml | 1 - .../src/aggregate/avg_distinct/decimal.rs | 12 ------------ 2 files changed, 13 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 484eb6eebc906..e7d322d9061ae 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -253,7 +253,6 @@ iter_not_returning_iterator = "warn" iter_on_empty_collections = "warn" iter_on_single_items = "warn" iter_without_into_iter = "warn" -large_digit_groups = "warn" # Detects large stack-allocated futures that may cause stack overflow crashes (see threshold in clippy.toml) large_futures = "warn" large_include_file = "warn" diff --git a/datafusion/functions-aggregate-common/src/aggregate/avg_distinct/decimal.rs b/datafusion/functions-aggregate-common/src/aggregate/avg_distinct/decimal.rs index 780835fd10046..0394a8391ad70 100644 --- a/datafusion/functions-aggregate-common/src/aggregate/avg_distinct/decimal.rs +++ b/datafusion/functions-aggregate-common/src/aggregate/avg_distinct/decimal.rs @@ -173,10 +173,6 @@ mod tests { } #[test] - #[expect( - clippy::large_digit_groups, - reason = "The grouping spells out the decimal scale, e.g. 180_00000000 is 180 with scale 8" - )] fn test_decimal64_distinct_avg_accumulator() -> Result<()> { let precision = 10_u8; let scale = 4_i8; @@ -206,10 +202,6 @@ mod tests { } #[test] - #[expect( - clippy::large_digit_groups, - reason = "The grouping spells out the decimal scale, e.g. 180_00000000 is 180 with scale 8" - )] fn test_decimal128_distinct_avg_accumulator() -> Result<()> { let precision = 10_u8; let scale = 4_i8; @@ -239,10 +231,6 @@ mod tests { } #[test] - #[expect( - clippy::large_digit_groups, - reason = "The grouping spells out the decimal scale, e.g. 180_00000000 is 180 with scale 8" - )] fn test_decimal256_distinct_avg_accumulator() -> Result<()> { let precision = 50_u8; let scale = 2_i8; From b24f047169db430ed823ee9b289855f6a5b454a6 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Tue, 18 Aug 2026 05:49:40 +0200 Subject: [PATCH 42/42] fix: apply new lints to code that arrived from main The upstream merge brought in code written before these lints were enabled, so it failed `clippy -D warnings` on the merge commit: - `branches_sharing_code`: hoist `raw_keys` out of the if/else in `dictionary.rs` - `iter_on_single_items`: `[None].iter()` -> `std::iter::once(&None)` - `elidable_lifetime_names`: elide `'a` in `scan_with_args_inner`, `infer_boxed`, and `infer_options_boxed` Co-Authored-By: Claude Opus 5 (1M context) --- datafusion/catalog-listing/src/table.rs | 4 ++-- datafusion/catalog/src/cte_worktable.rs | 4 ++-- datafusion/core/src/datasource/listing/table.rs | 12 ++++++------ datafusion/physical-expr/src/expressions/binary.rs | 2 +- .../group_values/multi_group_by/dictionary.rs | 3 +-- 5 files changed, 12 insertions(+), 13 deletions(-) diff --git a/datafusion/catalog-listing/src/table.rs b/datafusion/catalog-listing/src/table.rs index 1749cdac2f311..7f5d38b9991d5 100644 --- a/datafusion/catalog-listing/src/table.rs +++ b/datafusion/catalog-listing/src/table.rs @@ -583,10 +583,10 @@ impl ListingTable { Box::pin(self.scan_with_args_inner(state, args)) } - async fn scan_with_args_inner<'a>( + async fn scan_with_args_inner( &self, state: &dyn Session, - args: ScanArgs<'a>, + args: ScanArgs<'_>, ) -> datafusion_common::Result { let projection = args.projection().map(|p| p.to_vec()); let filters = args.filters().map(|f| f.to_vec()).unwrap_or_default(); diff --git a/datafusion/catalog/src/cte_worktable.rs b/datafusion/catalog/src/cte_worktable.rs index 5f5094793c01d..9180e01043ed0 100644 --- a/datafusion/catalog/src/cte_worktable.rs +++ b/datafusion/catalog/src/cte_worktable.rs @@ -128,10 +128,10 @@ impl TableProvider for CteWorkTable { } impl CteWorkTable { - fn scan_with_args_inner<'a>( + fn scan_with_args_inner( &self, _state: &dyn Session, - args: &ScanArgs<'a>, + args: &ScanArgs<'_>, ) -> Result { Ok(ScanResult::new(Arc::new(WorkTableExec::new( self.name.clone(), diff --git a/datafusion/core/src/datasource/listing/table.rs b/datafusion/core/src/datasource/listing/table.rs index f5a76c2cf579d..982766dc88519 100644 --- a/datafusion/core/src/datasource/listing/table.rs +++ b/datafusion/core/src/datasource/listing/table.rs @@ -76,18 +76,18 @@ impl ListingTableConfigExt for ListingTableConfig { } /// Body of [`ListingTableConfigExt::infer`]. -fn infer_boxed<'a>( +fn infer_boxed( config: ListingTableConfig, - state: &'a dyn Session, -) -> BoxFuture<'a, datafusion_common::Result> { + state: &dyn Session, +) -> BoxFuture<'_, datafusion_common::Result> { Box::pin(async move { config.infer_options(state).await?.infer_schema(state).await }) } /// Body of [`ListingTableConfigExt::infer_options`]. -fn infer_options_boxed<'a>( +fn infer_options_boxed( config: ListingTableConfig, - state: &'a dyn Session, -) -> BoxFuture<'a, datafusion_common::Result> { + state: &dyn Session, +) -> BoxFuture<'_, datafusion_common::Result> { Box::pin(async move { let store = if let Some(url) = config.table_paths.first() { state.runtime_env().object_store(url)? diff --git a/datafusion/physical-expr/src/expressions/binary.rs b/datafusion/physical-expr/src/expressions/binary.rs index e86748ccc964e..ee61d80565f83 100644 --- a/datafusion/physical-expr/src/expressions/binary.rs +++ b/datafusion/physical-expr/src/expressions/binary.rs @@ -5592,7 +5592,7 @@ mod tests { .unwrap() .into_array(batch.num_rows()) .unwrap(); - let expected: BooleanArray = [None].iter().collect(); + let expected: BooleanArray = std::iter::once(&None).collect(); assert_eq!(result.as_ref(), &expected); } diff --git a/datafusion/physical-plan/src/aggregates/group_values/multi_group_by/dictionary.rs b/datafusion/physical-plan/src/aggregates/group_values/multi_group_by/dictionary.rs index 501b13d0cd183..4dd488380a9d8 100644 --- a/datafusion/physical-plan/src/aggregates/group_values/multi_group_by/dictionary.rs +++ b/datafusion/physical-plan/src/aggregates/group_values/multi_group_by/dictionary.rs @@ -392,8 +392,8 @@ impl GroupColumn ) })?; + let raw_keys = dict_keys.values(); if dict_keys.null_count() == 0 { - let raw_keys = dict_keys.values(); for &row in rows { let val_idx = raw_keys[row].as_usize(); if self.val_to_inner[val_idx] == usize::MAX { @@ -411,7 +411,6 @@ impl GroupColumn self.group_to_inner.push(self.val_to_inner[val_idx]); } } else { - let raw_keys = dict_keys.values(); let null_buf = dict_keys.nulls().unwrap(); for &row in rows { let slot = if null_buf.is_null(row) {