diff --git a/Cargo.toml b/Cargo.toml index 66bd816945908..370e11fc115cf 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -282,25 +282,19 @@ explicit_into_iter_loop = "allow" # 55 hits explicit_iter_loop = "allow" # 189 hits float_cmp = "allow" # 8 hits; exact float comparisons are often intentional here from_iter_instead_of_collect = "allow" # 51 hits -if_not_else = "allow" # 133 hits -ignored_unit_patterns = "allow" # 52 hits implicit_clone = "allow" # 198 hits implicit_hasher = "allow" # 17 hits; some sites feed arrow APIs that require the default hasher inline_always = "allow" # 45 hits items_after_statements = "allow" # 171 hits -manual_string_new = "allow" # 84 hits many_single_char_names = "allow" # 12 hits; short names are idiomatic in the numeric kernels map_unwrap_or = "allow" # 198 hits -match_bool = "allow" # 46 hits match_same_arms = "allow" # 261 hits match_wildcard_for_single_variants = "allow" # 132 hits missing_errors_doc = "allow" # 1807 hits -missing_fields_in_debug = "allow" # 29 hits missing_panics_doc = "allow" # 244 hits must_use_candidate = "allow" # 2726 hits needless_raw_string_hashes = "allow" # 540 hits redundant_closure_for_method_calls = "allow" # 686 hits -redundant_else = "allow" # 48 hits return_self_not_must_use = "allow" # 644 hits semicolon_if_nothing_returned = "allow" # 1353 hits similar_names = "allow" # 228 hits; too many false positives, e.g. `expr`/`exprs` @@ -311,7 +305,6 @@ too_many_lines = "allow" # 484 hits trivially_copy_pass_by_ref = "allow" # 74 hits unnecessary_literal_bound = "allow" # 471 hits unnecessary_wraps = "allow" # 427 hits -unnested_or_patterns = "allow" # 68 hits unreadable_literal = "allow" # 502 hits unused_self = "allow" # 69 hits used_underscore_items = "allow" # 28 hits diff --git a/benchmarks/src/cancellation.rs b/benchmarks/src/cancellation.rs index 5f7fdcc43d99d..47bd76e80fc0d 100644 --- a/benchmarks/src/cancellation.rs +++ b/benchmarks/src/cancellation.rs @@ -127,12 +127,12 @@ fn run_test(wait_time: u64, store: Arc) -> Result { let store = Arc::clone(&store); tokio::select! { biased; - _ = async move { + () = async move { datafusion(store).await.unwrap(); } => { println!("matched case doing work"); }, - _ = captured_token.cancelled() => { + () = captured_token.cancelled() => { println!("Received shutdown request"); return; }, diff --git a/benchmarks/src/sql_benchmark.rs b/benchmarks/src/sql_benchmark.rs index 24db7e0a0fb2e..2382381c7b759 100644 --- a/benchmarks/src/sql_benchmark.rs +++ b/benchmarks/src/sql_benchmark.rs @@ -231,52 +231,49 @@ impl SqlBenchmark { let mut local_result = vec![]; for query in run_queries { - match save_results { - true => { - debug!( - "Running query (saving results) {}-{}: {query}", - self.group, self.subgroup - ); + if save_results { + debug!( + "Running query (saving results) {}-{}: {query}", + self.group, self.subgroup + ); - let df = ctx.sql(query).await?; - if !self.expect.is_empty() { - let physical_plan = df.create_physical_plan().await?; - self.validate_expected_plan(&physical_plan)?; - } + let df = ctx.sql(query).await?; + if !self.expect.is_empty() { + let physical_plan = df.create_physical_plan().await?; + self.validate_expected_plan(&physical_plan)?; + } + + let result_schema = Arc::new(df.schema().as_arrow().clone()); + let mut batches = df.collect().await?; + let trimmed = query.trim_start(); - let result_schema = Arc::new(df.schema().as_arrow().clone()); - let mut batches = df.collect().await?; - let trimmed = query.trim_start(); - - // save the output for select/with queries - if starts_with_ignore_ascii_case(trimmed, "select") - || starts_with_ignore_ascii_case(trimmed, "with") - { - if batches.is_empty() { - batches.push(RecordBatch::new_empty(result_schema)); - } - let row_count_for_query = - batches.iter().map(RecordBatch::num_rows).sum::(); - debug!( - "Persisting {} batches ({} rows)...", - batches.len(), - row_count_for_query - ); - - result_count = row_count_for_query; - local_result = batches; + // save the output for select/with queries + if starts_with_ignore_ascii_case(trimmed, "select") + || starts_with_ignore_ascii_case(trimmed, "with") + { + if batches.is_empty() { + batches.push(RecordBatch::new_empty(result_schema)); } - } - false => { + let row_count_for_query = + batches.iter().map(RecordBatch::num_rows).sum::(); debug!( - "Running query (ignoring results) {}-{}: {query}", - self.group, self.subgroup + "Persisting {} batches ({} rows)...", + batches.len(), + row_count_for_query ); - result_count = self - .execute_sql_without_result_buffering(query, ctx) - .await?; + result_count = row_count_for_query; + local_result = batches; } + } else { + debug!( + "Running query (ignoring results) {}-{}: {query}", + self.group, self.subgroup + ); + + result_count = self + .execute_sql_without_result_buffering(query, ctx) + .await?; } } @@ -377,15 +374,15 @@ impl SqlBenchmark { // Get the first result query (assuming only one for now) let query = &self.result_queries[0]; - let formatted_actual_results = if !query.query.trim().is_empty() { - let results = ctx.sql(&query.query).await?.collect().await?; - format_record_batches(&results) - } else { + let formatted_actual_results = if query.query.trim().is_empty() { let actual_results = self .last_results .as_ref() .expect("last_results should be present after successful run"); format_record_batches(actual_results) + } else { + let results = ctx.sql(&query.query).await?.collect().await?; + format_record_batches(&results) }?; Self::compare_results(query, &formatted_actual_results, &query.expected_result) @@ -509,7 +506,7 @@ impl SqlBenchmark { while let Some(result) = reader_result { match result { - Ok(_) => { + Ok(()) => { if !is_blank_or_comment_line(&line) { // boxing required because of recursion Box::pin(self.process_line(ctx, &mut reader, &mut line)).await?; @@ -824,7 +821,7 @@ impl BenchmarkDirective { loop { match reader_result { - Some(Ok(_)) => { + Some(Ok(())) => { if is_comment_line(line) { // comment, ignore } else if is_blank_line(line) { @@ -956,7 +953,7 @@ impl BenchmarkDirective { loop { match reader_result { - Some(Ok(_)) => { + Some(Ok(())) => { if line.trim() == "----" { found_break = true; break; @@ -1045,7 +1042,7 @@ impl BenchmarkDirective { loop { match reader_result { - Some(Ok(_)) => { + Some(Ok(())) => { if line.trim() == "----" { found_break = true; break; @@ -1108,7 +1105,7 @@ impl BenchmarkDirective { loop { match reader_result { - Some(Ok(_)) => { + Some(Ok(())) => { if is_comment_line(line) { // Clear the line buffer for the next iteration. line.clear(); @@ -1449,7 +1446,7 @@ fn read_query_from_reader( loop { match reader_result { - Some(Ok(_)) => { + Some(Ok(())) => { if is_comment_line(&line) { // comment, ignore } else if is_blank_line(&line) { diff --git a/datafusion-cli/src/exec.rs b/datafusion-cli/src/exec.rs index 288ce4b7351b6..c0c086e874760 100644 --- a/datafusion-cli/src/exec.rs +++ b/datafusion-cli/src/exec.rs @@ -69,7 +69,7 @@ pub async fn exec_from_lines( reader: &mut BufReader, print_options: &PrintOptions, ) -> Result<()> { - let mut query = "".to_owned(); + let mut query = String::new(); for line in reader.lines() { match line { @@ -79,10 +79,10 @@ pub async fn exec_from_lines( query.push_str(line); if line.ends_with(';') { match exec_and_print(ctx, print_options, query).await { - Ok(_) => {} + Ok(()) => {} Err(err) => eprintln!("{err}"), } - query = "".to_string(); + query = String::new(); } else { query.push('\n'); } @@ -175,7 +175,7 @@ pub async fn exec_from_repl( rl.add_history_entry(line.trim_end())?; tokio::select! { res = exec_and_print(ctx, print_options, line) => match res { - Ok(_) => {} + Ok(()) => {} Err(err) => eprintln!("{err}"), }, _ = signal::ctrl_c() => { diff --git a/datafusion-cli/src/functions.rs b/datafusion-cli/src/functions.rs index 0d7d8f33738fa..76c56d9029d49 100644 --- a/datafusion-cli/src/functions.rs +++ b/datafusion-cli/src/functions.rs @@ -665,8 +665,7 @@ impl TableFunctionImpl for StatisticsCacheFunc { { for (path, entry) in file_statistics_cache.list_entries() { path_arr.push(path.path.to_string()); - table_arr - .push(path.table.map_or_else(|| "".to_string(), |t| t.to_string())); + table_arr.push(path.table.map_or_else(String::new, |t| t.to_string())); file_modified_arr .push(Some(entry.value.meta.last_modified.timestamp_millis())); file_size_bytes_arr.push(entry.value.meta.size); diff --git a/datafusion-cli/src/helper.rs b/datafusion-cli/src/helper.rs index e1e26701a2846..e2b3b76a87607 100644 --- a/datafusion-cli/src/helper.rs +++ b/datafusion-cli/src/helper.rs @@ -49,10 +49,10 @@ pub struct CliHelper { impl CliHelper { pub fn new(dialect: &Dialect, color: bool) -> Self { - let highlighter: Box = if !color { - Box::new(NoSyntaxHighlighter {}) - } else { + let highlighter: Box = if color { Box::new(SyntaxHighlighter::new(dialect)) + } else { + Box::new(NoSyntaxHighlighter {}) }; Self { completer: FilenameCompleter::new(), diff --git a/datafusion-cli/src/main.rs b/datafusion-cli/src/main.rs index f82206a5bd184..e15b153b9cb27 100644 --- a/datafusion-cli/src/main.rs +++ b/datafusion-cli/src/main.rs @@ -407,10 +407,10 @@ fn parse_batch_size(size: &str) -> Result { } fn parse_command(command: &str) -> Result { - if !command.is_empty() { - Ok(command.to_string()) - } else { + if command.is_empty() { Err("-c flag expects only non empty commands".to_string()) + } else { + Ok(command.to_string()) } } diff --git a/datafusion-cli/src/object_storage/instrumented.rs b/datafusion-cli/src/object_storage/instrumented.rs index a0321cacb374b..062529c98d2be 100644 --- a/datafusion-cli/src/object_storage/instrumented.rs +++ b/datafusion-cli/src/object_storage/instrumented.rs @@ -499,7 +499,7 @@ impl fmt::Debug for RequestDetails { .field("size", &self.size) .field("range", &self.range) .field("extra_display", &self.extra_display) - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion-cli/src/print_format.rs b/datafusion-cli/src/print_format.rs index 0443a7a289602..d946bf8fa86a7 100644 --- a/datafusion-cli/src/print_format.rs +++ b/datafusion-cli/src/print_format.rs @@ -128,10 +128,9 @@ fn format_batches_with_maxrows( filtered_batches.push(sliced_batch); over_limit = true; break; - } else { - filtered_batches.push(batch.clone()); - row_count += batch.num_rows(); } + filtered_batches.push(batch.clone()); + row_count += batch.num_rows(); } let formatted = diff --git a/datafusion-examples/examples/custom_data_source/default_column_values.rs b/datafusion-examples/examples/custom_data_source/default_column_values.rs index d2024621aad76..b4c66bc770576 100644 --- a/datafusion-examples/examples/custom_data_source/default_column_values.rs +++ b/datafusion-examples/examples/custom_data_source/default_column_values.rs @@ -317,11 +317,11 @@ impl PhysicalExprAdapter for DefaultValuePhysicalExprAdapter { } // Replace columns with their default literals if any - let rewritten = if !replacements.is_empty() { + let rewritten = if replacements.is_empty() { + expr + } else { let refs: HashMap<_, _> = replacements.iter().map(|(k, v)| (*k, v)).collect(); replace_columns_with_literals(expr, &refs)? - } else { - expr }; // Apply the default adapter as a fallback for other schema adaptations diff --git a/datafusion-examples/examples/data_io/parquet_encrypted_with_kms.rs b/datafusion-examples/examples/data_io/parquet_encrypted_with_kms.rs index 8e92f465eafe9..6193c656149f6 100644 --- a/datafusion-examples/examples/data_io/parquet_encrypted_with_kms.rs +++ b/datafusion-examples/examples/data_io/parquet_encrypted_with_kms.rs @@ -200,7 +200,7 @@ async fn read_encrypted_with_sql(ctx: &SessionContext, table_path: &str) -> Resu extensions_options! { struct EncryptionConfig { /// Comma-separated list of columns to encrypt - pub encrypted_columns: String, default = "".to_owned() + pub encrypted_columns: String, default = String::new() } } diff --git a/datafusion-examples/examples/udf/simple_udtf.rs b/datafusion-examples/examples/udf/simple_udtf.rs index 3b55a0456a0aa..0374e913db35c 100644 --- a/datafusion-examples/examples/udf/simple_udtf.rs +++ b/datafusion-examples/examples/udf/simple_udtf.rs @@ -110,10 +110,9 @@ impl TableProvider for LocalCsvTable { let batch_lines = max_return_lines - lines; batches.push(batch.slice(0, batch_lines)); break; - } else { - batches.push(batch.clone()); - lines += batch_lines; } + batches.push(batch.clone()); + lines += batch_lines; } batches } else { diff --git a/datafusion/catalog-listing/src/helpers.rs b/datafusion/catalog-listing/src/helpers.rs index dc090378a8513..21c6667eba9b8 100644 --- a/datafusion/catalog-listing/src/helpers.rs +++ b/datafusion/catalog-listing/src/helpers.rs @@ -213,12 +213,14 @@ pub async fn list_partitions( depth: depth + 1, files: None, }; - match depth < max_depth { - true => match futures.len() < CONCURRENCY_LIMIT { - true => futures.push(child.list(store)), - false => pending.push(child.list(store)), - }, - false => out.push(child), + if depth < max_depth { + if futures.len() < CONCURRENCY_LIMIT { + futures.push(child.list(store)) + } else { + pending.push(child.list(store)) + } + } else { + out.push(child) } } } @@ -390,10 +392,10 @@ pub async fn pruned_partition_list<'a>( file_extension: &'a str, partition_cols: &'a [(String, DataType)], ) -> Result>> { - let prefix = if !partition_cols.is_empty() { - evaluate_partition_prefix(partition_cols, filters) - } else { + let prefix = if partition_cols.is_empty() { None + } else { + evaluate_partition_prefix(partition_cols, filters) }; let objects = table_path diff --git a/datafusion/catalog-listing/src/table.rs b/datafusion/catalog-listing/src/table.rs index eb5480dde6944..852834c899775 100644 --- a/datafusion/catalog-listing/src/table.rs +++ b/datafusion/catalog-listing/src/table.rs @@ -422,12 +422,11 @@ fn derive_common_ordering_from_files(file_groups: &[FileGroup]) -> Option 0, so ordering must be valid"); - CurrentOrderingState::SomeOrdering(ordering) } + let ordering = + LexOrdering::new(current.as_ref()[..prefix_len].to_vec()) + .expect("prefix_len > 0, so ordering must be valid"); + CurrentOrderingState::SomeOrdering(ordering) } // If one file has ordering and another doesn't, no common ordering // Return None and log a trace message explaining why diff --git a/datafusion/catalog/src/streaming.rs b/datafusion/catalog/src/streaming.rs index c2b5d691866b5..097bca8c02cf5 100644 --- a/datafusion/catalog/src/streaming.rs +++ b/datafusion/catalog/src/streaming.rs @@ -158,7 +158,9 @@ impl StreamingTable { _filters: &[Expr], limit: Option, ) -> Result> { - let physical_sort = if !self.sort_order.is_empty() { + let physical_sort = if self.sort_order.is_empty() { + vec![] + } else { let df_schema = DFSchema::try_from(Arc::clone(&self.schema))?; let eqp = state.execution_props(); @@ -182,8 +184,6 @@ impl StreamingTable { } else { original_sort_exprs } - } else { - vec![] }; let exec = StreamingTableExec::try_new( diff --git a/datafusion/common/src/column.rs b/datafusion/common/src/column.rs index b36b8f89779e3..5bd48f37fca79 100644 --- a/datafusion/common/src/column.rs +++ b/datafusion/common/src/column.rs @@ -37,6 +37,10 @@ pub struct Column { pub spans: Spans, } +#[expect( + clippy::missing_fields_in_debug, + reason = "this Debug output appears in user-facing error messages; `spans` is diagnostic bookkeeping and a `..` would only add noise" +)] impl fmt::Debug for Column { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("Column") diff --git a/datafusion/common/src/config.rs b/datafusion/common/src/config.rs index 4258ce6f003a9..2848fa3f429cf 100644 --- a/datafusion/common/src/config.rs +++ b/datafusion/common/src/config.rs @@ -1979,7 +1979,7 @@ config_namespace! { /// instead of being converted into a [`std::fmt::Error`] pub safe: bool, default = true /// Format string for nulls - pub null: String, default = "".into() + pub null: String, default = String::new() /// Date format for date arrays pub date_format: Option, default = Some("%Y-%m-%d".to_string()) /// Format for DateTime arrays @@ -3378,7 +3378,7 @@ impl Default for ConfigFileEncryptionProperties { config_namespace_with_hashmap! { pub struct ColumnEncryptionProperties { /// Per column encryption key - pub column_key_as_hex: String, default = "".to_string() + pub column_key_as_hex: String, default = String::new() /// Per column encryption key metadata pub column_metadata_as_hex: Option, default = None } @@ -3557,7 +3557,7 @@ pub struct ConfigFileDecryptionProperties { config_namespace_with_hashmap! { pub struct ColumnDecryptionProperties { /// Per column encryption key - pub column_key_as_hex: String, default = "".to_string() + pub column_key_as_hex: String, default = String::new() } } @@ -4448,12 +4448,12 @@ mod tests { #[cfg(feature = "parquet_encryption")] impl parquet::encryption::decrypt::KeyRetriever for ParquetEncryptionKeyRetriever { fn retrieve_key(&self, key_metadata: &[u8]) -> parquet::errors::Result> { - if !key_metadata.is_empty() { - Ok(b"1234567890123450".to_vec()) - } else { + if key_metadata.is_empty() { Err(parquet::errors::ParquetError::General( "Key metadata not provided".to_string(), )) + } else { + Ok(b"1234567890123450".to_vec()) } } } @@ -4527,7 +4527,7 @@ mod tests { let parsed_metadata = table_config.parquet.key_value_metadata.clone(); assert_eq!(parsed_metadata.get("should not exist1"), None); - assert_eq!(parsed_metadata.get("key1"), Some(&Some("".into()))); + assert_eq!(parsed_metadata.get("key1"), Some(&Some(String::new()))); assert_eq!(parsed_metadata.get("key2"), Some(&Some("value2".into()))); assert_eq!( parsed_metadata.get("key3"), diff --git a/datafusion/common/src/datatype.rs b/datafusion/common/src/datatype.rs index 19847f8583505..2e86e24b763e6 100644 --- a/datafusion/common/src/datatype.rs +++ b/datafusion/common/src/datatype.rs @@ -181,10 +181,10 @@ impl FieldExt for Field { fn renamed(self, new_name: &str) -> Self { // check if this is a new name before allocating a new Field / copying // the existing one - if self.name() != new_name { - self.with_name(new_name) - } else { + if self.name() == new_name { self + } else { + self.with_name(new_name) } } @@ -214,10 +214,10 @@ impl FieldExt for Field { } fn into_list_item(self) -> Self { - if self.name() != Field::LIST_FIELD_DEFAULT_NAME { - self.with_name(Field::LIST_FIELD_DEFAULT_NAME) - } else { + if self.name() == Field::LIST_FIELD_DEFAULT_NAME { self + } else { + self.with_name(Field::LIST_FIELD_DEFAULT_NAME) } } } diff --git a/datafusion/common/src/dfschema.rs b/datafusion/common/src/dfschema.rs index a0e2f0590a628..b48791ebe6c47 100644 --- a/datafusion/common/src/dfschema.rs +++ b/datafusion/common/src/dfschema.rs @@ -395,7 +395,7 @@ impl DFSchema { // field to lookup is qualified but current field is unqualified. (Some(_), None) => false, // field to lookup is unqualified, no need to compare qualifier - (None, Some(_)) | (None, None) => f.name() == name, + (None, Some(_) | None) => f.name() == name, }) .map(|(idx, _)| idx); matches.next() @@ -631,14 +631,7 @@ impl DFSchema { /// encoded UTF8 array to be equivalent to a plain UTF8 array. pub fn has_equivalent_names_and_types(&self, other: &Self) -> Result<()> { // case 1 : schema length mismatch - if self.fields().len() != other.fields().len() { - _plan_err!( - "Schema mismatch: the schema length are not same \ - Expected schema length: {}, got: {}", - self.fields().len(), - other.fields().len() - ) - } else { + if self.fields().len() == other.fields().len() { // case 2 : schema length match, but fields mismatch // check if the fields name are the same and have the same data types self.fields() @@ -663,6 +656,13 @@ impl DFSchema { Ok(()) } }) + } else { + _plan_err!( + "Schema mismatch: the schema length are not same \ + Expected schema length: {}, got: {}", + self.fields().len(), + other.fields().len() + ) } } @@ -1319,14 +1319,7 @@ impl SchemaExt for Schema { // It is only used by insert into cases. fn logically_equivalent_names_and_types(&self, other: &Self) -> Result<()> { // case 1 : schema length mismatch - if self.fields().len() != other.fields().len() { - _plan_err!( - "Inserting query must have the same schema length as the table. \ - Expected table schema length: {}, got: {}", - self.fields().len(), - other.fields().len() - ) - } else { + if self.fields().len() == other.fields().len() { // case 2 : schema length match, but fields mismatch // check if the fields name are the same and have the same data types self.fields() @@ -1345,6 +1338,13 @@ impl SchemaExt for Schema { Ok(()) } }) + } else { + _plan_err!( + "Inserting query must have the same schema length as the table. \ + Expected table schema length: {}, got: {}", + self.fields().len(), + other.fields().len() + ) } } } diff --git a/datafusion/common/src/error.rs b/datafusion/common/src/error.rs index d1fcb50f73492..247e592d8e1a0 100644 --- a/datafusion/common/src/error.rs +++ b/datafusion/common/src/error.rs @@ -296,7 +296,9 @@ impl Display for SchemaError { )?; } - if !valid_fields.is_empty() { + if valid_fields.is_empty() { + Ok(()) + } else { write!( f, "\nValid fields are {}.", @@ -306,8 +308,6 @@ impl Display for SchemaError { .collect::>() .join(", ") ) - } else { - Ok(()) } } Self::DuplicateQualifiedField { qualifier, name } => { @@ -555,11 +555,11 @@ impl DataFusionError { return format!("{}{}", Self::BACK_TRACE_SEP, back_trace); } - "".to_owned() + String::new() } #[cfg(not(feature = "backtrace"))] - "".to_owned() + String::new() } /// Return a [`DataFusionErrorBuilder`] to build a [`DataFusionError`] @@ -606,7 +606,7 @@ impl DataFusionError { pub fn message(&self) -> Cow<'_, str> { match *self { DataFusionError::ArrowError(ref desc, ref backtrace) => { - let backtrace = backtrace.clone().unwrap_or_else(|| "".to_owned()); + let backtrace = backtrace.clone().unwrap_or_else(String::new); Cow::Owned(format!("{desc}{backtrace}")) } #[cfg(feature = "parquet")] @@ -614,8 +614,7 @@ impl DataFusionError { DataFusionError::IoError(ref desc) => Cow::Owned(desc.to_string()), #[cfg(feature = "sql")] DataFusionError::SQL(ref desc, ref backtrace) => { - let backtrace: String = - backtrace.clone().unwrap_or_else(|| "".to_owned()); + let backtrace: String = backtrace.clone().unwrap_or_else(String::new); Cow::Owned(format!("{desc:?}{backtrace}")) } DataFusionError::Configuration(ref desc) => Cow::Owned(desc.to_string()), @@ -628,7 +627,7 @@ impl DataFusionError { DataFusionError::Plan(ref desc) => Cow::Owned(desc.to_string()), DataFusionError::SchemaError(ref desc, ref backtrace) => { let backtrace: &str = - &backtrace.as_ref().clone().unwrap_or_else(|| "".to_owned()); + &backtrace.as_ref().clone().unwrap_or_else(String::new); Cow::Owned(format!("{desc}{backtrace}")) } DataFusionError::Execution(ref desc) => Cow::Owned(desc.to_string()), diff --git a/datafusion/common/src/nested_struct.rs b/datafusion/common/src/nested_struct.rs index 3604126075bde..7d47cb8e9d228 100644 --- a/datafusion/common/src/nested_struct.rs +++ b/datafusion/common/src/nested_struct.rs @@ -647,14 +647,14 @@ fn cast_dictionary_column( let result = result?; // If key types differ, delegate key casting to Arrow. - if source_key_type != target_key_type { + if source_key_type == target_key_type { + Ok(result) + } else { let target_dict_type = DataType::Dictionary( Box::new(target_key_type.clone()), Box::new(target_value_type.clone()), ); Ok(cast_with_options(&result, &target_dict_type, cast_options)?) - } else { - Ok(result) } } @@ -3335,7 +3335,9 @@ pub fn adapt_batch_to_schema( let cast_options = CastOptions::default(); for (target_field, col) in target_schema.fields().iter().zip(batch.columns()) { - if target_field.data_type() != col.data_type() { + if target_field.data_type() == col.data_type() { + columns.push(Arc::clone(col)); + } else { // If data types differ, verify that target_field's data type contains // the column's data type (e.g. stricter nested struct / list field nullability). if !target_field.data_type().contains(col.data_type()) { @@ -3349,8 +3351,6 @@ pub fn adapt_batch_to_schema( needs_column_adaptation = true; let adapted_col = cast_column(col, target_field.data_type(), &cast_options)?; columns.push(adapted_col); - } else { - columns.push(Arc::clone(col)); } } diff --git a/datafusion/common/src/scalar/mod.rs b/datafusion/common/src/scalar/mod.rs index bad526a3a2227..af1339b88bac0 100644 --- a/datafusion/common/src/scalar/mod.rs +++ b/datafusion/common/src/scalar/mod.rs @@ -723,11 +723,11 @@ impl PartialOrd for ScalarValue { (LargeListView(arr1), LargeListView(arr2)) => { partial_cmp_list(arr1.as_ref(), arr2.as_ref()) } - (List(_), _) - | (LargeList(_), _) - | (FixedSizeList(_), _) - | (ListView(_), _) - | (LargeListView(_), _) => None, + ( + List(_) | LargeList(_) | FixedSizeList(_) | ListView(_) + | LargeListView(_), + _, + ) => None, (Struct(struct_arr1), Struct(struct_arr2)) => { partial_cmp_struct(struct_arr1.as_ref(), struct_arr2.as_ref()) } @@ -1703,9 +1703,9 @@ impl ScalarValue { | DataType::Date64 => ScalarValue::new_zero(datatype), // String types - DataType::Utf8 => Ok(ScalarValue::Utf8(Some("".to_string()))), - DataType::LargeUtf8 => Ok(ScalarValue::LargeUtf8(Some("".to_string()))), - DataType::Utf8View => Ok(ScalarValue::Utf8View(Some("".to_string()))), + DataType::Utf8 => Ok(ScalarValue::Utf8(Some(String::new()))), + DataType::LargeUtf8 => Ok(ScalarValue::LargeUtf8(Some(String::new()))), + DataType::Utf8View => Ok(ScalarValue::Utf8View(Some(String::new()))), // Binary types DataType::Binary => Ok(ScalarValue::Binary(Some(vec![]))), @@ -3191,10 +3191,8 @@ impl ScalarValue { // not supported if the TimeUnit is not valid (Time32 can // only be used with Second and Millisecond, Time64 only // with Microsecond and Nanosecond) - DataType::Time32(TimeUnit::Microsecond) - | DataType::Time32(TimeUnit::Nanosecond) - | DataType::Time64(TimeUnit::Second) - | DataType::Time64(TimeUnit::Millisecond) => { + DataType::Time32(TimeUnit::Microsecond | TimeUnit::Nanosecond) + | DataType::Time64(TimeUnit::Second | TimeUnit::Millisecond) => { return _not_impl_err!( "Unsupported creation of {:?} array from ScalarValue {:?}", data_type, @@ -4258,9 +4256,10 @@ impl ScalarValue { }; ScalarValue::FixedSizeBinary( size, - match array.is_null(index) { - true => None, - false => Some(array.value(index).into()), + if array.is_null(index) { + None + } else { + Some(array.value(index).into()) }, ) } @@ -5650,7 +5649,7 @@ impl fmt::Display for ScalarValue { match epoch.checked_add_signed(Duration::try_days(v as i64).unwrap()) { Some(date) => date.to_string(), - None => "".to_string(), + None => String::new(), } }) )?, @@ -5661,7 +5660,7 @@ impl fmt::Display for ScalarValue { match epoch.checked_add_signed(Duration::try_milliseconds(v).unwrap()) { Some(date) => date.to_string(), - None => "".to_string(), + None => String::new(), } }) )?, @@ -10640,11 +10639,11 @@ mod tests { // Test string types assert_eq!( ScalarValue::new_default(&DataType::Utf8).unwrap(), - ScalarValue::Utf8(Some("".to_string())) + ScalarValue::Utf8(Some(String::new())) ); assert_eq!( ScalarValue::new_default(&DataType::LargeUtf8).unwrap(), - ScalarValue::LargeUtf8(Some("".to_string())) + ScalarValue::LargeUtf8(Some(String::new())) ); // Test binary types diff --git a/datafusion/common/src/stats.rs b/datafusion/common/src/stats.rs index 1a226c369884f..4a02e7ff7dca7 100644 --- a/datafusion/common/src/stats.rs +++ b/datafusion/common/src/stats.rs @@ -102,9 +102,8 @@ impl Precision { (Precision::Exact(a), Precision::Exact(b)) => { Precision::Exact(if a >= b { a.clone() } else { b.clone() }) } - (Precision::Inexact(a), Precision::Exact(b)) - | (Precision::Exact(a), Precision::Inexact(b)) - | (Precision::Inexact(a), Precision::Inexact(b)) => { + (Precision::Inexact(a), Precision::Exact(b) | Precision::Inexact(b)) + | (Precision::Exact(a), Precision::Inexact(b)) => { Precision::Inexact(if a >= b { a.clone() } else { b.clone() }) } (_, _) => Precision::Absent, @@ -119,9 +118,8 @@ impl Precision { (Precision::Exact(a), Precision::Exact(b)) => { Precision::Exact(if a >= b { b.clone() } else { a.clone() }) } - (Precision::Inexact(a), Precision::Exact(b)) - | (Precision::Exact(a), Precision::Inexact(b)) - | (Precision::Inexact(a), Precision::Inexact(b)) => { + (Precision::Inexact(a), Precision::Exact(b) | Precision::Inexact(b)) + | (Precision::Exact(a), Precision::Inexact(b)) => { Precision::Inexact(if a >= b { b.clone() } else { a.clone() }) } (_, _) => Precision::Absent, @@ -147,9 +145,8 @@ impl Precision { || Precision::Inexact(a.saturating_add(*b)), Precision::Exact, ), - (Precision::Inexact(a), Precision::Exact(b)) - | (Precision::Exact(a), Precision::Inexact(b)) - | (Precision::Inexact(a), Precision::Inexact(b)) => { + (Precision::Inexact(a), Precision::Exact(b) | Precision::Inexact(b)) + | (Precision::Exact(a), Precision::Inexact(b)) => { Precision::Inexact(a.saturating_add(*b)) } (_, _) => Precision::Absent, @@ -165,9 +162,8 @@ impl Precision { || Precision::Inexact(a.saturating_sub(*b)), Precision::Exact, ), - (Precision::Inexact(a), Precision::Exact(b)) - | (Precision::Exact(a), Precision::Inexact(b)) - | (Precision::Inexact(a), Precision::Inexact(b)) => { + (Precision::Inexact(a), Precision::Exact(b) | Precision::Inexact(b)) + | (Precision::Exact(a), Precision::Inexact(b)) => { Precision::Inexact(a.saturating_sub(*b)) } (_, _) => Precision::Absent, @@ -183,9 +179,8 @@ impl Precision { || Precision::Inexact(a.saturating_mul(*b)), Precision::Exact, ), - (Precision::Inexact(a), Precision::Exact(b)) - | (Precision::Exact(a), Precision::Inexact(b)) - | (Precision::Inexact(a), Precision::Inexact(b)) => { + (Precision::Inexact(a), Precision::Exact(b) | Precision::Inexact(b)) + | (Precision::Exact(a), Precision::Inexact(b)) => { Precision::Inexact(a.saturating_mul(*b)) } (_, _) => Precision::Absent, @@ -240,9 +235,8 @@ impl Precision { .add_checked(b) .map(Precision::Exact) .unwrap_or(Precision::Absent), - (Precision::Inexact(a), Precision::Exact(b)) - | (Precision::Exact(a), Precision::Inexact(b)) - | (Precision::Inexact(a), Precision::Inexact(b)) => a + (Precision::Inexact(a), Precision::Exact(b) | Precision::Inexact(b)) + | (Precision::Exact(a), Precision::Inexact(b)) => a .add_checked(b) .map(Precision::Inexact) .unwrap_or(Precision::Absent), @@ -283,9 +277,8 @@ impl Precision { (Precision::Exact(a), Precision::Exact(b)) => { a.sub(b).map(Precision::Exact).unwrap_or(Precision::Absent) } - (Precision::Inexact(a), Precision::Exact(b)) - | (Precision::Exact(a), Precision::Inexact(b)) - | (Precision::Inexact(a), Precision::Inexact(b)) => a + (Precision::Inexact(a), Precision::Exact(b) | Precision::Inexact(b)) + | (Precision::Exact(a), Precision::Inexact(b)) => a .sub(b) .map(Precision::Inexact) .unwrap_or(Precision::Absent), @@ -302,9 +295,8 @@ impl Precision { .mul_checked(b) .map(Precision::Exact) .unwrap_or(Precision::Absent), - (Precision::Inexact(a), Precision::Exact(b)) - | (Precision::Exact(a), Precision::Inexact(b)) - | (Precision::Inexact(a), Precision::Inexact(b)) => a + (Precision::Inexact(a), Precision::Exact(b) | Precision::Inexact(b)) + | (Precision::Exact(a), Precision::Inexact(b)) => a .mul_checked(b) .map(Precision::Inexact) .unwrap_or(Precision::Absent), @@ -899,9 +891,11 @@ where Precision::Exact(right.clone()) } } - (Precision::Exact(left), Precision::Inexact(right)) - | (Precision::Inexact(left), Precision::Exact(right)) - | (Precision::Inexact(left), Precision::Inexact(right)) => { + ( + Precision::Exact(left) | Precision::Inexact(left), + Precision::Inexact(right), + ) + | (Precision::Inexact(left), Precision::Exact(right)) => { if left <= *right { Precision::Inexact(left) } else { @@ -927,9 +921,11 @@ where Precision::Exact(right.clone()) } } - (Precision::Exact(left), Precision::Inexact(right)) - | (Precision::Inexact(left), Precision::Exact(right)) - | (Precision::Inexact(left), Precision::Inexact(right)) => { + ( + Precision::Exact(left) | Precision::Inexact(left), + Precision::Inexact(right), + ) + | (Precision::Inexact(left), Precision::Exact(right)) => { if left >= *right { Precision::Inexact(left) } else { @@ -1019,35 +1015,35 @@ impl Display for Statistics { .enumerate() .map(|(i, cs)| { let s = format!("(Col[{i}]:"); - let s = if cs.min_value != Precision::Absent { - format!("{} Min={}", s, cs.min_value) - } else { + let s = if cs.min_value == Precision::Absent { s - }; - let s = if cs.max_value != Precision::Absent { - format!("{} Max={}", s, cs.max_value) } else { - s + format!("{} Min={}", s, cs.min_value) }; - let s = if cs.sum_value != Precision::Absent { - format!("{} Sum={}", s, cs.sum_value) - } else { + let s = if cs.max_value == Precision::Absent { s - }; - let s = if cs.null_count != Precision::Absent { - format!("{} Null={}", s, cs.null_count) } else { - s + format!("{} Max={}", s, cs.max_value) }; - let s = if cs.distinct_count != Precision::Absent { - format!("{} Distinct={}", s, cs.distinct_count) + let s = if cs.sum_value == Precision::Absent { + s } else { + format!("{} Sum={}", s, cs.sum_value) + }; + let s = if cs.null_count == Precision::Absent { s + } else { + format!("{} Null={}", s, cs.null_count) }; - let s = if cs.byte_size != Precision::Absent { - format!("{} ScanBytes={}", s, cs.byte_size) + let s = if cs.distinct_count == Precision::Absent { + s } else { + format!("{} Distinct={}", s, cs.distinct_count) + }; + let s = if cs.byte_size == Precision::Absent { s + } else { + format!("{} ScanBytes={}", s, cs.byte_size) }; s + ")" diff --git a/datafusion/common/src/test_util.rs b/datafusion/common/src/test_util.rs index 348fe2ef547f0..122f063788717 100644 --- a/datafusion/common/src/test_util.rs +++ b/datafusion/common/src/test_util.rs @@ -285,16 +285,16 @@ pub fn get_data_dir( let trimmed = dir.trim().to_string(); if !trimmed.is_empty() { let pb = PathBuf::from(trimmed); - if pb.is_dir() { - return Ok(pb); + return if pb.is_dir() { + Ok(pb) } else { - return Err(format!( + Err(format!( "the data dir `{}` defined by env {} not found", pb.display(), udf_env ) - .into()); - } + .into()) + }; } } diff --git a/datafusion/common/src/tree_node.rs b/datafusion/common/src/tree_node.rs index d5261a8925519..ca04a7496f33a 100644 --- a/datafusion/common/src/tree_node.rs +++ b/datafusion/common/src/tree_node.rs @@ -1290,7 +1290,9 @@ impl TreeNode for Arc { f: F, ) -> Result> { let children = self.arc_children(); - if !children.is_empty() { + if children.is_empty() { + Ok(Transformed::no(self)) + } else { let new_children = children .into_iter() .cloned() @@ -1305,8 +1307,6 @@ impl TreeNode for Arc { } else { Ok(Transformed::new(self, false, new_children.tnr)) } - } else { - Ok(Transformed::no(self)) } } } @@ -1338,13 +1338,13 @@ impl TreeNode for T { f: F, ) -> Result> { let (new_self, children) = self.take_children(); - if !children.is_empty() { + if children.is_empty() { + Ok(Transformed::no(new_self)) + } else { let new_children = children.into_iter().map_until_stop_and_collect(f)?; // Propagate up `new_children.transformed` and `new_children.tnr` along with // the node containing transformed children. new_children.map_data(|new_children| new_self.with_new_children(new_children)) - } else { - Ok(Transformed::no(new_self)) } } } diff --git a/datafusion/core/benches/preserve_file_partitioning.rs b/datafusion/core/benches/preserve_file_partitioning.rs index c459853d5e05c..0ec4fe508af34 100644 --- a/datafusion/core/benches/preserve_file_partitioning.rs +++ b/datafusion/core/benches/preserve_file_partitioning.rs @@ -88,9 +88,9 @@ impl BenchConfig { fn from_env() -> Self { match std::env::var("BENCH_SIZE").as_deref() { - Ok("small") | Ok("SMALL") => Self::small(), - Ok("medium") | Ok("MEDIUM") => Self::medium(), - Ok("large") | Ok("LARGE") => Self::large(), + Ok("small" | "SMALL") => Self::small(), + Ok("medium" | "MEDIUM") => Self::medium(), + Ok("large" | "LARGE") => Self::large(), _ => { println!("Using SMALL dataset (set BENCH_SIZE=small|medium|large)"); Self::small() diff --git a/datafusion/core/src/bin/print_functions_docs.rs b/datafusion/core/src/bin/print_functions_docs.rs index 10a259dd8b745..edc922260f3d0 100644 --- a/datafusion/core/src/bin/print_functions_docs.rs +++ b/datafusion/core/src/bin/print_functions_docs.rs @@ -93,7 +93,7 @@ fn print_docs( providers: Vec>, doc_sections: Vec, ) -> Result { - let mut docs = "".to_string(); + let mut docs = String::new(); // Ensure that all providers have documentation let mut providers_with_no_docs = HashSet::new(); @@ -228,9 +228,11 @@ fn print_docs( } } - // If there are any functions that do not have documentation, print them out - // eventually make this an error: https://github.com/apache/datafusion/issues/12872 - if !providers_with_no_docs.is_empty() { + if providers_with_no_docs.is_empty() { + Ok(docs) + } else { + // Some functions do not have documentation, print them out. + // Eventually make this an error: https://github.com/apache/datafusion/issues/12872 eprintln!("INFO: The following functions do not have documentation:"); for f in &providers_with_no_docs { eprintln!(" - {f}"); @@ -238,8 +240,6 @@ fn print_docs( not_impl_err!( "Some functions do not have documentation. Please implement `documentation` for: {providers_with_no_docs:?}" ) - } else { - Ok(docs) } } diff --git a/datafusion/core/src/datasource/file_format/avro.rs b/datafusion/core/src/datasource/file_format/avro.rs index a8b48cc736c92..14c79f06ff631 100644 --- a/datafusion/core/src/datasource/file_format/avro.rs +++ b/datafusion/core/src/datasource/file_format/avro.rs @@ -60,7 +60,7 @@ mod tests { assert_eq!(11, batch.num_columns()); assert_eq!(2, batch.num_rows()); }) - .fold(0, |acc, _| async move { acc + 1i32 }) + .fold(0, |acc, ()| async move { acc + 1i32 }) .await; assert_eq!(tt_batches, 4 /* 8/2 */); diff --git a/datafusion/core/src/datasource/file_format/csv.rs b/datafusion/core/src/datasource/file_format/csv.rs index 2fb64fd6486e6..c3c983097bdc9 100644 --- a/datafusion/core/src/datasource/file_format/csv.rs +++ b/datafusion/core/src/datasource/file_format/csv.rs @@ -210,7 +210,7 @@ mod tests { assert_eq!(12, batch.num_columns()); assert_eq!(2, batch.num_rows()); }) - .fold(0, |acc, _| async move { acc + 1i32 }) + .fold(0, |acc, ()| async move { acc + 1i32 }) .await; assert_eq!(tt_batches, 50 /* 100/2 */); diff --git a/datafusion/core/src/datasource/file_format/json.rs b/datafusion/core/src/datasource/file_format/json.rs index 1f6f27242e723..02039a880c136 100644 --- a/datafusion/core/src/datasource/file_format/json.rs +++ b/datafusion/core/src/datasource/file_format/json.rs @@ -112,7 +112,7 @@ mod tests { assert_eq!(4, batch.num_columns()); assert_eq!(2, batch.num_rows()); }) - .fold(0, |acc, _| async move { acc + 1i32 }) + .fold(0, |acc, ()| async move { acc + 1i32 }) .await; assert_eq!(tt_batches, 6 /* 12/2 */); diff --git a/datafusion/core/src/datasource/file_format/parquet.rs b/datafusion/core/src/datasource/file_format/parquet.rs index bfcfb74848861..06cf0a8318c35 100644 --- a/datafusion/core/src/datasource/file_format/parquet.rs +++ b/datafusion/core/src/datasource/file_format/parquet.rs @@ -709,7 +709,7 @@ mod tests { assert_eq!(11, batch.num_columns()); assert_eq!(2, batch.num_rows()); }) - .fold(0, |acc, _| async move { acc + 1i32 }) + .fold(0, |acc, ()| async move { acc + 1i32 }) .await; assert_eq!(tt_batches, 4 /* 8/2 */); diff --git a/datafusion/core/src/datasource/listing_table_factory.rs b/datafusion/core/src/datasource/listing_table_factory.rs index 1e597e38fb5b1..d3d0c6bdd6a40 100644 --- a/datafusion/core/src/datasource/listing_table_factory.rs +++ b/datafusion/core/src/datasource/listing_table_factory.rs @@ -144,13 +144,14 @@ impl ListingTableFactory { // extension filter is left empty and the explicit paths/globs are used // as provided. let file_extension = if table_paths.len() == 1 { - match first_path.is_collection() { + if first_path.is_collection() { // Setting the extension to be empty instead of allowing the default extension seems // odd, but was done to ensure existing behavior isn't modified. It seems like this // could be refactored to either use the default extension or set the fully expected // extension when compression is included (e.g. ".csv.gz") - true => String::new(), - false => get_extension(&cmd.locations[0]), + String::new() + } else { + get_extension(&cmd.locations[0]) } } else { String::new() @@ -332,7 +333,7 @@ fn get_extension(path: &str) -> String { let res = Path::new(path).extension().and_then(|ext| ext.to_str()); match res { Some(ext) => format!(".{ext}"), - None => "".to_string(), + None => String::new(), } } diff --git a/datafusion/core/src/datasource/physical_plan/csv.rs b/datafusion/core/src/datasource/physical_plan/csv.rs index 7980df87fa576..361e36b214341 100644 --- a/datafusion/core/src/datasource/physical_plan/csv.rs +++ b/datafusion/core/src/datasource/physical_plan/csv.rs @@ -764,7 +764,7 @@ mod tests { // get name of first part let paths = fs::read_dir(&out_dir).unwrap(); - let mut part_0_name: String = "".to_owned(); + let mut part_0_name: String = String::new(); for path in paths { let path = path.unwrap(); let name = path diff --git a/datafusion/core/src/datasource/physical_plan/json.rs b/datafusion/core/src/datasource/physical_plan/json.rs index 6b4361e0c4d07..0309a5ae4bcfb 100644 --- a/datafusion/core/src/datasource/physical_plan/json.rs +++ b/datafusion/core/src/datasource/physical_plan/json.rs @@ -410,7 +410,7 @@ mod tests { // get name of first part let paths = fs::read_dir(&out_dir).unwrap(); - let mut part_0_name: String = "".to_owned(); + let mut part_0_name: String = String::new(); for path in paths { let name = path .unwrap() diff --git a/datafusion/core/src/datasource/provider.rs b/datafusion/core/src/datasource/provider.rs index e574042813a7b..6ed7b4cc8c06a 100644 --- a/datafusion/core/src/datasource/provider.rs +++ b/datafusion/core/src/datasource/provider.rs @@ -87,9 +87,10 @@ impl DefaultTableFactory { } } - match unbounded { - true => self.stream.create(state, cmd).await, - false => self.listing.create(state, cmd).await, + if unbounded { + self.stream.create(state, cmd).await + } else { + self.listing.create(state, cmd).await } } } diff --git a/datafusion/core/src/execution/context/mod.rs b/datafusion/core/src/execution/context/mod.rs index ff1ad25811440..8dee606b037e6 100644 --- a/datafusion/core/src/execution/context/mod.rs +++ b/datafusion/core/src/execution/context/mod.rs @@ -1012,7 +1012,7 @@ impl SessionContext { match (if_not_exists, schema) { (true, Some(_)) => self.return_empty_dataframe(), - (true, None) | (false, None) => { + (_, None) => { let schema = Arc::new(MemorySchemaProvider::new()); catalog.register_schema(schema_name, schema)?; self.return_empty_dataframe() @@ -1031,7 +1031,7 @@ impl SessionContext { match (if_not_exists, catalog) { (true, Some(_)) => self.return_empty_dataframe(), - (true, None) | (false, None) => { + (_, None) => { let new_catalog = Arc::new(MemoryCatalogProvider::new()); self.state .write() diff --git a/datafusion/core/src/execution/session_state.rs b/datafusion/core/src/execution/session_state.rs index aa8ba4c3b733b..9cf3e4b27da5f 100644 --- a/datafusion/core/src/execution/session_state.rs +++ b/datafusion/core/src/execution/session_state.rs @@ -2006,7 +2006,7 @@ impl Debug for SessionStateBuilder { .field("higher_order_functions", &self.higher_order_functions) .field("aggregate_functions", &self.aggregate_functions) .field("window_functions", &self.window_functions) - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion/core/src/physical_planner.rs b/datafusion/core/src/physical_planner.rs index 3bcbc4d43ca53..7e7b34043fdeb 100644 --- a/datafusion/core/src/physical_planner.rs +++ b/datafusion/core/src/physical_planner.rs @@ -1106,10 +1106,10 @@ impl DefaultPhysicalPlanner { } } } - let input_exec = if !async_exprs.is_empty() { - Arc::new(AsyncFuncExec::try_new(async_exprs, input_exec)?) - } else { + let input_exec = if async_exprs.is_empty() { input_exec + } else { + Arc::new(AsyncFuncExec::try_new(async_exprs, input_exec)?) }; let initial_aggr = Arc::new(AggregateExec::try_new( @@ -2437,9 +2437,11 @@ fn is_identity_assignment(expr: &Expr, column_name: &str) -> bool { /// OVER (ORDER BY a RANGES BETWEEN INTERVAL '3 DAY' PRECEDING AND '5 DAY' PRECEDING) are rejected pub fn is_window_frame_bound_valid(window_frame: &WindowFrame) -> bool { match (&window_frame.start_bound, &window_frame.end_bound) { - (WindowFrameBound::Following(_), WindowFrameBound::Preceding(_)) - | (WindowFrameBound::Following(_), WindowFrameBound::CurrentRow) - | (WindowFrameBound::CurrentRow, WindowFrameBound::Preceding(_)) => false, + ( + WindowFrameBound::Following(_) | WindowFrameBound::CurrentRow, + WindowFrameBound::Preceding(_), + ) + | (WindowFrameBound::Following(_), WindowFrameBound::CurrentRow) => false, (WindowFrameBound::Preceding(lhs), WindowFrameBound::Preceding(rhs)) => { !rhs.is_null() && (lhs.is_null() || (lhs >= rhs)) } diff --git a/datafusion/core/src/test/variable.rs b/datafusion/core/src/test/variable.rs index 38207b42cb7b8..1b797dcb58f65 100644 --- a/datafusion/core/src/test/variable.rs +++ b/datafusion/core/src/test/variable.rs @@ -59,19 +59,19 @@ impl UserDefinedVar { impl VarProvider for UserDefinedVar { /// Get user defined variable value fn get_value(&self, var_names: Vec) -> Result { - if var_names[0] != "@integer" { + if var_names[0] == "@integer" { + Ok(ScalarValue::Int32(Some(41))) + } else { let s = format!("{}-{}", "user-defined-var", var_names.concat()); Ok(ScalarValue::from(s)) - } else { - Ok(ScalarValue::Int32(Some(41))) } } fn get_type(&self, var_names: &[String]) -> Option { - if var_names[0] != "@integer" { - Some(DataType::Utf8) - } else { + if var_names[0] == "@integer" { Some(DataType::Int32) + } else { + Some(DataType::Utf8) } } } diff --git a/datafusion/core/tests/custom_sources_cases/dml_planning.rs b/datafusion/core/tests/custom_sources_cases/dml_planning.rs index cb5b134fab04a..670da4c312099 100644 --- a/datafusion/core/tests/custom_sources_cases/dml_planning.rs +++ b/datafusion/core/tests/custom_sources_cases/dml_planning.rs @@ -84,7 +84,7 @@ impl std::fmt::Debug for CaptureDeleteProvider { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("CaptureDeleteProvider") .field("schema", &self.schema) - .finish() + .finish_non_exhaustive() } } @@ -180,7 +180,7 @@ impl std::fmt::Debug for CaptureUpdateProvider { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("CaptureUpdateProvider") .field("schema", &self.schema) - .finish() + .finish_non_exhaustive() } } @@ -254,7 +254,7 @@ impl std::fmt::Debug for CaptureTruncateProvider { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("CaptureTruncateProvider") .field("schema", &self.schema) - .finish() + .finish_non_exhaustive() } } 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 4ec9747058140..3a5e791bdadfe 100644 --- a/datafusion/core/tests/custom_sources_cases/provider_filter_pushdown.rs +++ b/datafusion/core/tests/custom_sources_cases/provider_filter_pushdown.rs @@ -225,9 +225,10 @@ impl TableProvider for CustomProvider { }; Ok(Arc::new(CustomPlan::new( - match projection.is_empty() { - true => Arc::new(Schema::empty()), - false => self.zero_batch.schema(), + if projection.is_empty() { + Arc::new(Schema::empty()) + } else { + self.zero_batch.schema() }, match int_value { 0 => vec![self.zero_batch.clone()], @@ -237,9 +238,10 @@ impl TableProvider for CustomProvider { ))) } _ => Ok(Arc::new(CustomPlan::new( - match projection.is_empty() { - true => Arc::new(Schema::empty()), - false => self.zero_batch.schema(), + if projection.is_empty() { + Arc::new(Schema::empty()) + } else { + self.zero_batch.schema() }, vec![], ))), diff --git a/datafusion/core/tests/execution/coop.rs b/datafusion/core/tests/execution/coop.rs index e02364a0530cc..60e1504bf8bf3 100644 --- a/datafusion/core/tests/execution/coop.rs +++ b/datafusion/core/tests/execution/coop.rs @@ -795,12 +795,12 @@ async fn stream_yields( result = join_handle => { match result { Ok(Poll::Pending) => Yielded::ReadyOrPending, - Ok(Poll::Ready(Ok(_))) => Yielded::ReadyOrPending, + Ok(Poll::Ready(Ok(()))) => Yielded::ReadyOrPending, Ok(Poll::Ready(Err(e))) => Yielded::Err(e), Err(_) => Yielded::Err(exec_datafusion_err!("join error")), } }, - _ = tokio::time::sleep(Duration::from_secs(10)) => { + () = tokio::time::sleep(Duration::from_secs(10)) => { Yielded::Timeout } }; diff --git a/datafusion/core/tests/fuzz_cases/aggregation_fuzzer/query_builder.rs b/datafusion/core/tests/fuzz_cases/aggregation_fuzzer/query_builder.rs index d32078ec6331f..6b1654ad70080 100644 --- a/datafusion/core/tests/fuzz_cases/aggregation_fuzzer/query_builder.rs +++ b/datafusion/core/tests/fuzz_cases/aggregation_fuzzer/query_builder.rs @@ -306,7 +306,7 @@ impl QueryBuilder { self.null_opt(), ) } else { - ("".to_string(), "".to_string()) + (String::new(), String::new()) }; let function = format!( diff --git a/datafusion/core/tests/fuzz_cases/topk_filter_pushdown.rs b/datafusion/core/tests/fuzz_cases/topk_filter_pushdown.rs index 80df91cb1036b..0bc0226ee1cea 100644 --- a/datafusion/core/tests/fuzz_cases/topk_filter_pushdown.rs +++ b/datafusion/core/tests/fuzz_cases/topk_filter_pushdown.rs @@ -391,9 +391,9 @@ async fn test_fuzz_topk_filter_pushdown() { println!("\n\n"); } - if !failures.is_empty() { - panic!("Some test cases failed"); - } else { + if failures.is_empty() { println!("All test cases passed"); + } else { + panic!("Some test cases failed"); } } diff --git a/datafusion/core/tests/fuzz_cases/window_fuzz.rs b/datafusion/core/tests/fuzz_cases/window_fuzz.rs index f69b5e9a41b02..b7a423e59f7a9 100644 --- a/datafusion/core/tests/fuzz_cases/window_fuzz.rs +++ b/datafusion/core/tests/fuzz_cases/window_fuzz.rs @@ -768,7 +768,7 @@ pub(crate) fn make_staggered_batches( let mut rng = StdRng::seed_from_u64(random_seed); let mut input123: Vec<(i32, i32, i32)> = vec![(0, 0, 0); len]; let mut input4: Vec = vec![0; len]; - let mut input5: Vec = vec!["".to_string(); len]; + let mut input5: Vec = vec![String::new(); len]; for v in &mut input123 { *v = ( rng.random_range(0..n_distinct) as i32, diff --git a/datafusion/core/tests/physical_optimizer/join_selection.rs b/datafusion/core/tests/physical_optimizer/join_selection.rs index d4561decc6331..5ec4f149d6209 100644 --- a/datafusion/core/tests/physical_optimizer/join_selection.rs +++ b/datafusion/core/tests/physical_optimizer/join_selection.rs @@ -1246,12 +1246,7 @@ fn check_join_partition_mode( .optimize(join, &ConfigOptions::new()) .unwrap(); - if !is_swapped { - let swapped_join = optimized_join - .downcast_ref::() - .expect("The type of the plan should not be changed"); - assert_eq!(*swapped_join.partition_mode(), expected_mode); - } else { + if is_swapped { let swapping_projection = optimized_join .downcast_ref::() .expect("A proj is required to swap columns back to their original order"); @@ -1261,6 +1256,11 @@ fn check_join_partition_mode( .expect("The type of the plan should not be changed"); assert_eq!(*swapped_join.partition_mode(), expected_mode); + } else { + let swapped_join = optimized_join + .downcast_ref::() + .expect("The type of the plan should not be changed"); + assert_eq!(*swapped_join.partition_mode(), expected_mode); } } diff --git a/datafusion/core/tests/physical_optimizer/sanity_checker.rs b/datafusion/core/tests/physical_optimizer/sanity_checker.rs index 184125dcbe180..ab985c3fdd32b 100644 --- a/datafusion/core/tests/physical_optimizer/sanity_checker.rs +++ b/datafusion/core/tests/physical_optimizer/sanity_checker.rs @@ -51,16 +51,13 @@ async fn register_current_csv( let schema = datafusion::test_util::aggr_test_schema(); let path = format!("{testdata}/csv/aggregate_test_100.csv"); - match infinite { - true => { - let source = FileStreamProvider::new_file(schema, path.into()); - let config = StreamConfig::new(Arc::new(source)); - ctx.register_table(table_name, Arc::new(StreamTable::new(Arc::new(config))))?; - } - false => { - ctx.register_csv(table_name, &path, CsvReadOptions::new().schema(&schema)) - .await?; - } + if infinite { + let source = FileStreamProvider::new_file(schema, path.into()); + let config = StreamConfig::new(Arc::new(source)); + ctx.register_table(table_name, Arc::new(StreamTable::new(Arc::new(config))))?; + } else { + ctx.register_csv(table_name, &path, CsvReadOptions::new().schema(&schema)) + .await?; } Ok(()) diff --git a/datafusion/core/tests/physical_optimizer/test_utils.rs b/datafusion/core/tests/physical_optimizer/test_utils.rs index 6b10efffa90a3..37d182d58828c 100644 --- a/datafusion/core/tests/physical_optimizer/test_utils.rs +++ b/datafusion/core/tests/physical_optimizer/test_utils.rs @@ -919,7 +919,9 @@ pub struct TestScan { impl TestScan { /// Create a new TestScan with the given schema and output ordering pub fn new(schema: SchemaRef, output_ordering: Vec) -> Self { - let eq_properties = if !output_ordering.is_empty() { + let eq_properties = if output_ordering.is_empty() { + EquivalenceProperties::new(Arc::clone(&schema)) + } else { // Convert Vec to the format expected by new_with_orderings // We need to extract the inner Vec from each LexOrdering let orderings: Vec> = output_ordering @@ -931,8 +933,6 @@ impl TestScan { .collect(); EquivalenceProperties::new_with_orderings(Arc::clone(&schema), orderings) - } else { - EquivalenceProperties::new(Arc::clone(&schema)) }; let plan_properties = PlanProperties::new( diff --git a/datafusion/core/tests/sql/unparser.rs b/datafusion/core/tests/sql/unparser.rs index 3982c60dbc7d9..45e78f47dc688 100644 --- a/datafusion/core/tests/sql/unparser.rs +++ b/datafusion/core/tests/sql/unparser.rs @@ -920,13 +920,13 @@ async fn collect_results(ctx: &SessionContext, original: &str) -> TestCaseResult }; } - if expected != actual { + if expected == actual { + TestCaseResult::Success + } else { TestCaseResult::ResultsMismatch { original: original.to_string(), unparsed, } - } else { - TestCaseResult::Success } } diff --git a/datafusion/core/tests/user_defined/user_defined_aggregates.rs b/datafusion/core/tests/user_defined/user_defined_aggregates.rs index d035fa25e1d41..e77d4c183c1df 100644 --- a/datafusion/core/tests/user_defined/user_defined_aggregates.rs +++ b/datafusion/core/tests/user_defined/user_defined_aggregates.rs @@ -992,9 +992,10 @@ impl Accumulator for MetadataBasedAccumulator { } fn evaluate(&mut self) -> Result { - let v = match self.double_output { - true => self.curr_sum * 2, - false => self.curr_sum, + let v = if self.double_output { + self.curr_sum * 2 + } else { + self.curr_sum }; Ok(ScalarValue::from(v)) 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 4f8078542e4ac..640667d3b10ba 100644 --- a/datafusion/core/tests/user_defined/user_defined_scalar_functions.rs +++ b/datafusion/core/tests/user_defined/user_defined_scalar_functions.rs @@ -197,7 +197,7 @@ impl std::fmt::Debug for Simple0ArgsScalarUDF { .field("name", &self.name) .field("signature", &self.signature) .field("fun", &"") - .finish() + .finish_non_exhaustive() } } @@ -1819,9 +1819,10 @@ 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(x: i8, as_bool: bool) -> String { - match as_bool { - true => format!("{}", x != 0), - false => format!("{x}"), + if as_bool { + format!("{}", x != 0) + } else { + format!("{x}") } } diff --git a/datafusion/core/tests/user_defined/user_defined_table_functions.rs b/datafusion/core/tests/user_defined/user_defined_table_functions.rs index 24205cf8c4010..97c74a621acf9 100644 --- a/datafusion/core/tests/user_defined/user_defined_table_functions.rs +++ b/datafusion/core/tests/user_defined/user_defined_table_functions.rs @@ -133,7 +133,9 @@ impl TableProvider for SimpleCsvTable { _filters: &[Expr], _limit: Option, ) -> Result> { - let batches = if !self.exprs.is_empty() { + let batches = if self.exprs.is_empty() { + self.batches.clone() + } else { let max_return_lines = self.interpreter_expr(state).await?; // get max return rows from self.batches let mut batches = vec![]; @@ -144,14 +146,11 @@ impl TableProvider for SimpleCsvTable { let batch_lines = max_return_lines as usize - lines; batches.push(batch.slice(0, batch_lines)); break; - } else { - batches.push(batch.clone()); - lines += batch_lines; } + batches.push(batch.clone()); + lines += batch_lines; } batches - } else { - self.batches.clone() }; Ok(MemorySourceConfig::try_new_exec( &[batches], diff --git a/datafusion/datasource-csv/src/file_format.rs b/datafusion/datasource-csv/src/file_format.rs index f14924563cdb6..2cf5eba92bf94 100644 --- a/datafusion/datasource-csv/src/file_format.rs +++ b/datafusion/datasource-csv/src/file_format.rs @@ -351,7 +351,7 @@ impl Debug for CsvSerializer { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("CsvSerializer") .field("header", &self.header) - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion/datasource-parquet/src/metadata.rs b/datafusion/datasource-parquet/src/metadata.rs index 213cac24a85be..13266cd24d40c 100644 --- a/datafusion/datasource-parquet/src/metadata.rs +++ b/datafusion/datasource-parquet/src/metadata.rs @@ -699,7 +699,7 @@ impl StatisticsAccumulators<'_> { (Some(max_value), Some(true)) => { max_value.evaluate().ok().map(Precision::Exact) } - (Some(max_value), Some(false)) | (Some(max_value), None) => { + (Some(max_value), Some(false) | None) => { max_value.evaluate().ok().map(Precision::Inexact) } (None, _) => None, @@ -711,7 +711,7 @@ impl StatisticsAccumulators<'_> { (Some(min_value), Some(true)) => { min_value.evaluate().ok().map(Precision::Exact) } - (Some(min_value), Some(false)) | (Some(min_value), None) => { + (Some(min_value), Some(false) | None) => { min_value.evaluate().ok().map(Precision::Inexact) } (None, _) => None, diff --git a/datafusion/datasource-parquet/src/opener/mod.rs b/datafusion/datasource-parquet/src/opener/mod.rs index 25c3bc9a77851..8aaffe06962a9 100644 --- a/datafusion/datasource-parquet/src/opener/mod.rs +++ b/datafusion/datasource-parquet/src/opener/mod.rs @@ -310,7 +310,7 @@ impl fmt::Debug for ParquetMorselizer { .field("preserve_order", &self.preserve_order) .field("enable_page_index", &self.enable_page_index) .field("enable_bloom_filter", &self.enable_bloom_filter) - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion/datasource-parquet/src/row_group_filter.rs b/datafusion/datasource-parquet/src/row_group_filter.rs index 2db4cb718d364..2a55f2287c8ce 100644 --- a/datafusion/datasource-parquet/src/row_group_filter.rs +++ b/datafusion/datasource-parquet/src/row_group_filter.rs @@ -350,12 +350,12 @@ impl RowGroupAccessPlanFilter { Ok(values) => { let mut fully_contained_candidates_original_idx: Vec = Vec::new(); for (idx, &value) in row_group_indexes.iter().zip(values.iter()) { - if !value { - self.access_plan.skip(*idx); - metrics.row_groups_pruned_statistics.add_pruned(1); - } else { + if value { metrics.row_groups_pruned_statistics.add_matched(1); fully_contained_candidates_original_idx.push(*idx); + } else { + self.access_plan.skip(*idx); + metrics.row_groups_pruned_statistics.add_pruned(1); } } diff --git a/datafusion/datasource-parquet/src/sink.rs b/datafusion/datasource-parquet/src/sink.rs index 1b79ae665bb14..dd2503a804210 100644 --- a/datafusion/datasource-parquet/src/sink.rs +++ b/datafusion/datasource-parquet/src/sink.rs @@ -694,47 +694,46 @@ fn spawn_parquet_parallel_serialization_task( .await?; current_rg_rows += rb.num_rows(); break; - } else { - let rows_left = max_row_group_rows - current_rg_rows; - let a = rb.slice(0, rows_left); - send_arrays_to_col_writers( - &col_array_channels, - &a, - Arc::clone(&ctx.schema), - ) - .await?; + } + let rows_left = max_row_group_rows - current_rg_rows; + let a = rb.slice(0, rows_left); + send_arrays_to_col_writers( + &col_array_channels, + &a, + Arc::clone(&ctx.schema), + ) + .await?; + + // Signal the parallel column writers that the RowGroup is done, join and finalize RowGroup + // on a separate task, so that we can immediately start on the next RG before waiting + // for the current one to finish. + drop(col_array_channels); + let finalize_rg_task = spawn_rg_join_and_finalize_task( + column_writer_handles, + max_row_group_rows, + &ctx.pool, + encoding_time.clone(), + ); - // Signal the parallel column writers that the RowGroup is done, join and finalize RowGroup - // on a separate task, so that we can immediately start on the next RG before waiting - // for the current one to finish. - drop(col_array_channels); - let finalize_rg_task = spawn_rg_join_and_finalize_task( - column_writer_handles, - max_row_group_rows, - &ctx.pool, - encoding_time.clone(), - ); + // Do not surface error from closed channel (means something + // else hit an error, and the plan is shutting down). + if serialize_tx.send(finalize_rg_task).await.is_err() { + return Ok(()); + } - // Do not surface error from closed channel (means something - // else hit an error, and the plan is shutting down). - if serialize_tx.send(finalize_rg_task).await.is_err() { - return Ok(()); - } + current_rg_rows = 0; + rb = rb.slice(rows_left, rb.num_rows() - rows_left); - current_rg_rows = 0; - rb = rb.slice(rows_left, rb.num_rows() - rows_left); - - row_group_index += 1; - let col_writers = row_group_writer_factory - .create_column_writers(row_group_index)?; - (column_writer_handles, col_array_channels) = - spawn_column_parallel_row_group_writer( - col_writers, - max_buffer_rb, - &ctx.pool, - &encoding_time, - )?; - } + row_group_index += 1; + let col_writers = + row_group_writer_factory.create_column_writers(row_group_index)?; + (column_writer_handles, col_array_channels) = + spawn_column_parallel_row_group_writer( + col_writers, + max_buffer_rb, + &ctx.pool, + &encoding_time, + )?; } } diff --git a/datafusion/datasource-parquet/src/source.rs b/datafusion/datasource-parquet/src/source.rs index 087da503654d7..298de8ddf3bb7 100644 --- a/datafusion/datasource-parquet/src/source.rs +++ b/datafusion/datasource-parquet/src/source.rs @@ -1147,15 +1147,15 @@ impl ParquetSource { // The predicate was serialized against the scan's output schema, so it // must be decoded against the projected schema when a projection is // present. - let predicate_schema = if !base_conf.projection.is_empty() { + let predicate_schema = if base_conf.projection.is_empty() { + schema + } else { let projected_fields: Vec<_> = base_conf .projection .iter() .map(|&i| schema.field(i as usize).clone()) .collect(); Arc::new(Schema::new(projected_fields)) - } else { - schema }; let predicate = scan @@ -1170,9 +1170,10 @@ impl ParquetSource { } let table_schema = FileScanConfig::parse_table_schema_from_proto(base_conf)?; - let object_store_url = match base_conf.object_store_url.is_empty() { - false => ObjectStoreUrl::parse(&base_conf.object_store_url)?, - true => ObjectStoreUrl::local_filesystem(), + let object_store_url = if base_conf.object_store_url.is_empty() { + ObjectStoreUrl::local_filesystem() + } else { + ObjectStoreUrl::parse(&base_conf.object_store_url)? }; let store = ctx .task_ctx() diff --git a/datafusion/datasource/src/decoder.rs b/datafusion/datasource/src/decoder.rs index a21aaedc52c3a..f7a7168d1bf8d 100644 --- a/datafusion/datasource/src/decoder.rs +++ b/datafusion/datasource/src/decoder.rs @@ -91,7 +91,7 @@ impl fmt::Debug for DecoderDeserializer { f.debug_struct("Deserializer") .field("buffered_queue", &self.buffered_queue) .field("finalized", &self.finalized) - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion/datasource/src/file_compression_type.rs b/datafusion/datasource/src/file_compression_type.rs index 89efb580652b1..cad89c880ba58 100644 --- a/datafusion/datasource/src/file_compression_type.rs +++ b/datafusion/datasource/src/file_compression_type.rs @@ -65,7 +65,7 @@ impl GetExt for FileCompressionType { BZIP2 => ".bz2".to_owned(), XZ => ".xz".to_owned(), ZSTD => ".zst".to_owned(), - UNCOMPRESSED => "".to_owned(), + UNCOMPRESSED => String::new(), } } } diff --git a/datafusion/datasource/src/file_scan_config/proto.rs b/datafusion/datasource/src/file_scan_config/proto.rs index 4071ea471b33f..303fa469c20d0 100644 --- a/datafusion/datasource/src/file_scan_config/proto.rs +++ b/datafusion/datasource/src/file_scan_config/proto.rs @@ -189,9 +189,10 @@ impl FileScanConfig { .map(TryInto::try_into) .collect::>>()?; - let object_store_url = match conf.object_store_url.is_empty() { - false => ObjectStoreUrl::parse(&conf.object_store_url)?, - true => ObjectStoreUrl::local_filesystem(), + let object_store_url = if conf.object_store_url.is_empty() { + ObjectStoreUrl::local_filesystem() + } else { + ObjectStoreUrl::parse(&conf.object_store_url)? }; let mut output_ordering = vec![]; diff --git a/datafusion/datasource/src/file_scan_config/sort_pushdown.rs b/datafusion/datasource/src/file_scan_config/sort_pushdown.rs index 3f5beed20fa8d..2d55352ff05d2 100644 --- a/datafusion/datasource/src/file_scan_config/sort_pushdown.rs +++ b/datafusion/datasource/src/file_scan_config/sort_pushdown.rs @@ -111,31 +111,29 @@ impl FileScanConfig { new_config.file_source = new_file_source; // Sort files within groups by statistics when not reversing - let all_non_overlapping = if !reverse_file_groups { - if let Some(sort_order) = LexOrdering::new(order.iter().cloned()) { - let projected_schema = new_config.projected_schema()?; - let projection_indices = new_config - .file_source - .projection() - .as_ref() - .and_then(|p| ordered_column_indices_from_projection(p)); - let result = sort_files_within_groups_by_statistics( - &new_config.file_groups, - &sort_order, - &projected_schema, - projection_indices.as_deref(), - ); - new_config.file_groups = result.file_groups; - result.all_non_overlapping - } else { - false - } - } else { + let all_non_overlapping = if reverse_file_groups { // When reversing, files are already reversed above. We skip // statistics-based sorting here because it would undo the reversal. // Note: reverse path is always Inexact, so all_non_overlapping // is not used (is_exact is false). false + } else if let Some(sort_order) = LexOrdering::new(order.iter().cloned()) { + let projected_schema = new_config.projected_schema()?; + let projection_indices = new_config + .file_source + .projection() + .as_ref() + .and_then(|p| ordered_column_indices_from_projection(p)); + let result = sort_files_within_groups_by_statistics( + &new_config.file_groups, + &sort_order, + &projected_schema, + projection_indices.as_deref(), + ); + new_config.file_groups = result.file_groups; + result.all_non_overlapping + } else { + false }; // Decide whether to keep `output_ordering` (i.e. let the outer diff --git a/datafusion/datasource/src/memory.rs b/datafusion/datasource/src/memory.rs index 7c10dba981c82..61aff0dacb277 100644 --- a/datafusion/datasource/src/memory.rs +++ b/datafusion/datasource/src/memory.rs @@ -550,9 +550,7 @@ impl MemorySourceConfig { target_partitions: usize, output_ordering: LexOrdering, ) -> Result>>> { - if !self.eq_properties().ordering_satisfy(output_ordering)? { - Ok(None) - } else { + if self.eq_properties().ordering_satisfy(output_ordering)? { let total_num_batches = self.partitions.iter().map(|b| b.len()).sum::(); if total_num_batches < target_partitions { @@ -602,9 +600,8 @@ impl MemorySourceConfig { } // Successful repartition. Break inner loop, and return to outer `cnt_to_repartition` loop. break; - } else { - cannot_split_further.push(new_partitions.remove(0)); } + cannot_split_further.push(new_partitions.remove(0)); } } let mut partitions = max_heap @@ -619,6 +616,8 @@ impl MemorySourceConfig { let partitions = partitions.into_iter().map(|rep| rep.batches).collect_vec(); Ok(Some(partitions)) + } else { + Ok(None) } } @@ -885,7 +884,7 @@ impl Debug for MemSink { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("MemSink") .field("num_partitions", &self.batches.len()) - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion/datasource/src/url.rs b/datafusion/datasource/src/url.rs index cfb6608ca0a78..14a46c3e61b6b 100644 --- a/datafusion/datasource/src/url.rs +++ b/datafusion/datasource/src/url.rs @@ -436,16 +436,18 @@ async fn list_with_cache<'b>( #[cfg(not(target_arch = "wasm32"))] fn url_from_filesystem_path(s: &str) -> Option { let path = std::path::Path::new(s); - let is_dir = match path.exists() { - true => path.is_dir(), + let is_dir = if path.exists() { + path.is_dir() + } else { // Fallback to inferring from trailing separator - false => std::path::is_separator(s.chars().last()?), + std::path::is_separator(s.chars().last()?) }; let from_absolute_path = |p| { - let first = match is_dir { - true => Url::from_directory_path(p).ok(), - false => Url::from_file_path(p).ok(), + let first = if is_dir { + Url::from_directory_path(p).ok() + } else { + Url::from_file_path(p).ok() }?; // By default from_*_path preserve relative path segments diff --git a/datafusion/datasource/src/write/demux.rs b/datafusion/datasource/src/write/demux.rs index 1b3098d309789..d27da0235d65e 100644 --- a/datafusion/datasource/src/write/demux.rs +++ b/datafusion/datasource/src/write/demux.rs @@ -257,13 +257,13 @@ fn generate_file_path( file_extension: &str, single_file_output: bool, ) -> Path { - if !single_file_output { + if single_file_output { + base_output_path.prefix().to_owned() + } else { base_output_path .prefix() .clone() .join(format!("{write_id}_{part_idx}.{file_extension}")) - } else { - base_output_path.prefix().to_owned() } } @@ -566,10 +566,10 @@ fn remove_partition_by_columns( .iter() .zip(parted_batch.schema().fields()) .filter_map(|(a, f)| { - if !partition_names.contains(&f.name()) { - Some((Arc::clone(a), (**f).clone())) - } else { + if partition_names.contains(&f.name()) { None + } else { + Some((Arc::clone(a), (**f).clone())) } }) .unzip(); diff --git a/datafusion/datasource/src/write/orchestration.rs b/datafusion/datasource/src/write/orchestration.rs index cd821b3b87897..12cc033511672 100644 --- a/datafusion/datasource/src/write/orchestration.rs +++ b/datafusion/datasource/src/write/orchestration.rs @@ -115,7 +115,7 @@ pub(crate) async fn serialize_rb_stream_to_object_store( match task.join().await { Ok(Ok((cnt, bytes))) => { match writer.write_all(&bytes).await { - Ok(_) => (), + Ok(()) => (), Err(e) => { return SerializedRecordBatchResult::failure( None, @@ -142,7 +142,7 @@ pub(crate) async fn serialize_rb_stream_to_object_store( } match serialize_task.join().await { - Ok(Ok(_)) => (), + Ok(Ok(())) => (), Ok(Err(e)) => return SerializedRecordBatchResult::failure(Some(writer), e), Err(_) => { return SerializedRecordBatchResult::failure( @@ -216,20 +216,18 @@ pub(crate) async fn stateless_serialize_and_write_files( } if any_errors { - match any_abort_errors { - true => { + if any_abort_errors { + return internal_err!( + "Error encountered during writing to ObjectStore and failed to abort all writers. Partial result may have been written." + ); + } + match triggering_error { + Some(e) => return Err(e), + None => { return internal_err!( - "Error encountered during writing to ObjectStore and failed to abort all writers. Partial result may have been written." + "Unknown Error encountered during writing to ObjectStore. All writers successfully aborted." ); } - false => match triggering_error { - Some(e) => return Err(e), - None => { - return internal_err!( - "Unknown Error encountered during writing to ObjectStore. All writers successfully aborted." - ); - } - }, } } diff --git a/datafusion/doc/src/lib.rs b/datafusion/doc/src/lib.rs index 914df27cb85d7..beb381f41937b 100644 --- a/datafusion/doc/src/lib.rs +++ b/datafusion/doc/src/lib.rs @@ -92,10 +92,10 @@ impl Documentation { result.push_str( format!( "\n doc_section({}label = \"{}\"{}),", - if !self.doc_section.include { - "include = \"false\", " - } else { + if self.doc_section.include { "" + } else { + "include = \"false\", " }, self.doc_section.label, self.doc_section diff --git a/datafusion/execution/src/async_stream.rs b/datafusion/execution/src/async_stream.rs index 0462c53a0ffd3..9c8569a46448f 100644 --- a/datafusion/execution/src/async_stream.rs +++ b/datafusion/execution/src/async_stream.rs @@ -222,13 +222,13 @@ impl Future for Emit { type Output = (); fn poll(mut self: Pin<&mut Self>, _cx: &mut Context<'_>) -> Poll<()> { - if !self.done { + if self.done { + Poll::Ready(()) + } else { self.done = true; // Poll::Pending causes the generator to yield, returning control back to the // calling Stream Poll::Pending - } else { - Poll::Ready(()) } } } @@ -402,7 +402,7 @@ mod test { let s = async_stream(|mut emitter| async move { select! { - _ = do_stuff_async() => emitter.emit(()).await, + () = do_stuff_async() => emitter.emit(()).await, else => emitter.emit(()).await, } }); @@ -422,8 +422,8 @@ mod test { let s = async_stream(|mut emitter| async move { select! { - _ = do_stuff_async() => emitter.emit("hey").await, - _ = more_async_work() => emitter.emit("hey").await, + () = do_stuff_async() => emitter.emit("hey").await, + () = more_async_work() => emitter.emit("hey").await, else => emitter.emit("hey").await, } }); @@ -464,7 +464,7 @@ mod test { pin_mut!(s); for i in 0..3 { - assert_matches!(tx.send(i).await, Ok(_)); + assert_matches!(tx.send(i).await, Ok(())); assert_eq!(Some(i), s.next().await); } @@ -573,7 +573,7 @@ mod test { let _ = async_stream(|mut emitter| async move { select! { - _ = do_stuff_async() => { + () = do_stuff_async() => { let another_s = async_try_stream(|mut inner_emitter| async move { inner_emitter.emit(()).await; Ok(()) diff --git a/datafusion/execution/src/disk_manager.rs b/datafusion/execution/src/disk_manager.rs index 313379f01291f..ce9ee180e9240 100644 --- a/datafusion/execution/src/disk_manager.rs +++ b/datafusion/execution/src/disk_manager.rs @@ -51,7 +51,7 @@ impl Debug for DiskManagerBuilder { f.debug_struct("DiskManagerBuilder") .field("mode", &self.mode) .field("max_temp_directory_size", &self.max_temp_directory_size) - .finish() + .finish_non_exhaustive() } } impl Default for DiskManagerBuilder { @@ -218,7 +218,7 @@ impl Debug for DiskManager { .field("used_disk_space", &self.used_disk_space) .field("active_files_count", &self.active_files_count) .field("factory", &self.factory.is_some()) - .finish() + .finish_non_exhaustive() } } /// Information about the current disk usage for spilling diff --git a/datafusion/execution/src/memory_pool/peak_recording.rs b/datafusion/execution/src/memory_pool/peak_recording.rs index b407cc0eaf36b..652ec3979d764 100644 --- a/datafusion/execution/src/memory_pool/peak_recording.rs +++ b/datafusion/execution/src/memory_pool/peak_recording.rs @@ -166,7 +166,7 @@ impl Debug for PeakRecordingPool { .field("inner", &self.inner) .field("peak", &self.peak_reserved()) .field("max", &self.max_reserved()) - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion/execution/src/memory_pool/pool.rs b/datafusion/execution/src/memory_pool/pool.rs index d854cbd627cec..2d57c1576dba0 100644 --- a/datafusion/execution/src/memory_pool/pool.rs +++ b/datafusion/execution/src/memory_pool/pool.rs @@ -219,58 +219,57 @@ impl MemoryPool for FairSpillPool { fn grow(&self, reservation: &MemoryReservation, additional: usize) { let mut state = self.state.lock(); - match reservation.registration.consumer.can_spill { - true => state.spillable += additional, - false => state.unspillable += additional, + if reservation.registration.consumer.can_spill { + state.spillable += additional + } else { + state.unspillable += additional } } fn shrink(&self, reservation: &MemoryReservation, shrink: usize) { let mut state = self.state.lock(); - match reservation.registration.consumer.can_spill { - true => state.spillable -= shrink, - false => state.unspillable -= shrink, + if reservation.registration.consumer.can_spill { + state.spillable -= shrink + } else { + state.unspillable -= shrink } } fn try_grow(&self, reservation: &MemoryReservation, additional: usize) -> Result<()> { let mut state = self.state.lock(); - match reservation.registration.consumer.can_spill { - true => { - // The total amount of memory available to spilling consumers - let spill_available = self.pool_size.saturating_sub(state.unspillable); - - // No spiller may use more than their fraction of the memory available - let available = spill_available - .checked_div(state.num_spill) - .unwrap_or(spill_available); - - if reservation.size() + additional > available { - return Err(insufficient_capacity_err( - reservation, - additional, - available, - self, - )); - } - state.spillable += additional; + if reservation.registration.consumer.can_spill { + // The total amount of memory available to spilling consumers + let spill_available = self.pool_size.saturating_sub(state.unspillable); + + // No spiller may use more than their fraction of the memory available + let available = spill_available + .checked_div(state.num_spill) + .unwrap_or(spill_available); + + if reservation.size() + additional > available { + return Err(insufficient_capacity_err( + reservation, + additional, + available, + self, + )); } - false => { - let available = self - .pool_size - .saturating_sub(state.unspillable + state.spillable); - - if available < additional { - return Err(insufficient_capacity_err( - reservation, - additional, - available, - self, - )); - } - state.unspillable += additional; + state.spillable += additional; + } else { + let available = self + .pool_size + .saturating_sub(state.unspillable + state.spillable); + + if available < additional { + return Err(insufficient_capacity_err( + reservation, + additional, + available, + self, + )); } + state.unspillable += additional; } Ok(()) } diff --git a/datafusion/expr-common/src/casts.rs b/datafusion/expr-common/src/casts.rs index 3518c02772672..2677d6bd7353f 100644 --- a/datafusion/expr-common/src/casts.rs +++ b/datafusion/expr-common/src/casts.rs @@ -1478,9 +1478,9 @@ mod tests { // Test empty string expect_cast( - ScalarValue::Utf8(Some("".to_string())), + ScalarValue::Utf8(Some(String::new())), DataType::Utf8View, - ExpectedCast::Value(ScalarValue::Utf8View(Some("".to_string()))), + ExpectedCast::Value(ScalarValue::Utf8View(Some(String::new()))), ); // Test large string diff --git a/datafusion/expr-common/src/interval_arithmetic.rs b/datafusion/expr-common/src/interval_arithmetic.rs index 9f1291353dc29..e1168cf8352ed 100644 --- a/datafusion/expr-common/src/interval_arithmetic.rs +++ b/datafusion/expr-common/src/interval_arithmetic.rs @@ -1410,18 +1410,18 @@ pub fn satisfy_greater( ); if !left.upper.is_null() && left.upper <= right.lower { - if !strict && left.upper == right.lower { + return if !strict && left.upper == right.lower { // Singleton intervals: - return Ok(Some(( + Ok(Some(( Interval::new(left.upper.clone(), left.upper.clone()), Interval::new(left.upper.clone(), left.upper.clone()), - ))); + ))) } else { // Left-hand side: <--======----0------------> // Right-hand side: <------------0--======----> // No intersection, infeasible to propagate: - return Ok(None); - } + Ok(None) + }; } // Only the lower bound of left-hand side and the upper bound of the right-hand diff --git a/datafusion/expr-common/src/type_coercion/binary.rs b/datafusion/expr-common/src/type_coercion/binary.rs index 381897ae86fdc..f5d6dea7c84af 100644 --- a/datafusion/expr-common/src/type_coercion/binary.rs +++ b/datafusion/expr-common/src/type_coercion/binary.rs @@ -526,18 +526,11 @@ fn bitwise_coercion(left_type: &DataType, right_type: &DataType) -> Option Some(UInt64), (Int64, _) | (_, Int64) - | (UInt32, Int8) - | (Int8, UInt32) - | (UInt32, Int16) - | (Int16, UInt32) - | (UInt32, Int32) - | (Int32, UInt32) => Some(Int64), - (Int32, _) - | (_, Int32) - | (UInt16, Int16) - | (Int16, UInt16) - | (UInt16, Int8) - | (Int8, UInt16) => Some(Int32), + | (UInt32, Int8 | Int16 | Int32) + | (Int8 | Int16 | Int32, UInt32) => Some(Int64), + (Int32, _) | (_, Int32) | (UInt16, Int16 | Int8) | (Int16 | Int8, UInt16) => { + Some(Int32) + } (UInt32, _) | (_, UInt32) => Some(UInt32), (Int16, _) | (_, Int16) | (Int8, UInt8) | (UInt8, Int8) => Some(Int16), (UInt16, _) | (_, UInt16) => Some(UInt16), @@ -1023,20 +1016,18 @@ fn string_temporal_coercion( fn match_rule(l: &DataType, r: &DataType) -> Option { match (l, r) { // Coerce Utf8View/Utf8/LargeUtf8 to Date32/Date64/Time32/Time64/Timestamp - (Utf8, temporal) | (LargeUtf8, temporal) | (Utf8View, temporal) => { - match temporal { - Date32 | Date64 => Some(temporal.clone()), - Time32(_) | Time64(_) => { - if is_time_with_valid_unit(temporal) { - Some(temporal.to_owned()) - } else { - None - } + (Utf8 | LargeUtf8 | Utf8View, temporal) => match temporal { + Date32 | Date64 => Some(temporal.clone()), + Time32(_) | Time64(_) => { + if is_time_with_valid_unit(temporal) { + Some(temporal.to_owned()) + } else { + None } - Timestamp(_, tz) => Some(Timestamp(Nanosecond, tz.clone())), - _ => None, } - } + Timestamp(_, tz) => Some(Timestamp(Nanosecond, tz.clone())), + _ => None, + }, _ => None, } } @@ -1134,20 +1125,14 @@ fn get_wider_decimal_type_cross_variant( { Some(Decimal64(required_precision, s)) } - (Decimal32(_, _), Decimal128(_, _)) - | (Decimal128(_, _), Decimal32(_, _)) - | (Decimal64(_, _), Decimal128(_, _)) - | (Decimal128(_, _), Decimal64(_, _)) + (Decimal32(_, _) | Decimal64(_, _), Decimal128(_, _)) + | (Decimal128(_, _), Decimal32(_, _) | Decimal64(_, _)) if required_precision <= DECIMAL128_MAX_PRECISION => { Some(Decimal128(required_precision, s)) } - (Decimal32(_, _), Decimal256(_, _)) - | (Decimal256(_, _), Decimal32(_, _)) - | (Decimal64(_, _), Decimal256(_, _)) - | (Decimal256(_, _), Decimal64(_, _)) - | (Decimal128(_, _), Decimal256(_, _)) - | (Decimal256(_, _), Decimal128(_, _)) + (Decimal32(_, _) | Decimal64(_, _) | Decimal128(_, _), Decimal256(_, _)) + | (Decimal256(_, _), Decimal32(_, _) | Decimal64(_, _) | Decimal128(_, _)) if required_precision <= DECIMAL256_MAX_PRECISION => { Some(Decimal256(required_precision, s)) diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs index 3b708b4edeec9..2ab8a5b9f8b24 100644 --- a/datafusion/expr/src/expr.rs +++ b/datafusion/expr/src/expr.rs @@ -1929,14 +1929,15 @@ impl Expr { // f_up: unalias on up so we can remove nested aliases like // `(x as foo) as bar` if let Expr::Alias(alias) = expr { - match alias + if alias .metadata .as_ref() .map(|h| h.is_empty()) .unwrap_or(true) { - true => Ok(Transformed::yes(*alias.expr)), - false => Ok(Transformed::no(Expr::Alias(alias))), + Ok(Transformed::yes(*alias.expr)) + } else { + Ok(Transformed::no(Expr::Alias(alias))) } } else { Ok(Transformed::no(expr)) @@ -3569,9 +3570,10 @@ impl Display for Expr { } Expr::ScalarVariable(_, var_names) => write!(f, "{}", var_names.join(".")), Expr::Literal(v, metadata) => { - match metadata.as_ref().map(|m| m.is_empty()).unwrap_or(true) { - false => write!(f, "{v:?} {:?}", metadata.as_ref().unwrap()), - true => write!(f, "{v:?}"), + if metadata.as_ref().map(|m| m.is_empty()).unwrap_or(true) { + write!(f, "{v:?}") + } else { + write!(f, "{v:?} {:?}", metadata.as_ref().unwrap()) } } Expr::Case(case) => { @@ -3806,15 +3808,13 @@ fn fmt_function( args: &[Expr], display: bool, ) -> fmt::Result { - let args: Vec = match display { - true => args.iter().map(|arg| format!("{arg}")).collect(), - false => args.iter().map(|arg| format!("{arg:?}")).collect(), + let args: Vec = if display { + args.iter().map(|arg| format!("{arg}")).collect() + } else { + args.iter().map(|arg| format!("{arg:?}")).collect() }; - let distinct_str = match distinct { - true => "DISTINCT ", - false => "", - }; + let distinct_str = if distinct { "DISTINCT " } else { "" }; write!(f, "{}({}{})", fun, distinct_str, args.join(", ")) } diff --git a/datafusion/expr/src/expr_fn.rs b/datafusion/expr/src/expr_fn.rs index b1a5a12d155ce..30d5bf70c87a1 100644 --- a/datafusion/expr/src/expr_fn.rs +++ b/datafusion/expr/src/expr_fn.rs @@ -542,7 +542,7 @@ impl Debug for SimpleAggregateUDF { .field("signature", &self.signature) .field("return_type", &self.return_type) .field("fun", &"") - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion/expr/src/expr_rewriter/mod.rs b/datafusion/expr/src/expr_rewriter/mod.rs index 4e9839e2f7479..a7edec0e38d24 100644 --- a/datafusion/expr/src/expr_rewriter/mod.rs +++ b/datafusion/expr/src/expr_rewriter/mod.rs @@ -256,7 +256,9 @@ fn coerce_exprs_for_schema( .enumerate() .map(|(idx, expr)| { let new_type = dst_schema.field(idx).data_type(); - if new_type != &expr.get_type(src_schema)? { + if new_type == &expr.get_type(src_schema)? { + Ok(expr) + } else { match expr { Expr::Alias(Alias { expr, name, .. }) => { Ok(expr.cast_to(new_type, src_schema)?.alias(name)) @@ -276,8 +278,6 @@ fn coerce_exprs_for_schema( } } } - } else { - Ok(expr) } }) .collect::>() diff --git a/datafusion/expr/src/expr_schema.rs b/datafusion/expr/src/expr_schema.rs index 36b76f076d26a..688a23d90310f 100644 --- a/datafusion/expr/src/expr_schema.rs +++ b/datafusion/expr/src/expr_schema.rs @@ -288,7 +288,7 @@ impl ExprSchemable for Expr { .transpose()?; Ok(match has_nullable { // If a nullable subexpression is found, the result may also be nullable. - Some(_) => true, + Some(()) => true, // If the list is too long, we assume it is nullable. None if list.len() + 1 > MAX_INSPECT_LIMIT => true, // All the subexpressions are non-nullable, so the result must be non-nullable. @@ -341,14 +341,14 @@ impl ExprSchemable for Expr { Ok(b) => b, }; - if !can_be_true { + if can_be_true { + // The branch might be taken + Some(Ok(())) + } else { // 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(())) } }); @@ -356,7 +356,7 @@ impl ExprSchemable for Expr { // There is at least one reachable nullable 'then' expression, so the case // expression itself is nullable. // Use `Result::map` to propagate the error from `nullable_then` if there is one. - nullable_then.map(|_| true) + nullable_then.map(|()| true) } else if let Some(e) = &case.else_expr { // There are no reachable nullable 'then' expressions, so all we still need to // check is the 'else' expression's nullability. @@ -1218,7 +1218,7 @@ mod tests { let placeholder_meta = FieldMetadata::from(placeholder_meta); let expr = Expr::Placeholder(Placeholder::new_with_field( - "".to_string(), + String::new(), Some( Field::new("", DataType::Utf8, true) .with_metadata(placeholder_meta.to_hashmap()) @@ -1243,7 +1243,7 @@ mod tests { // Non-nullable placeholder field should remain non-nullable let expr = Expr::Placeholder(Placeholder::new_with_field( - "".to_string(), + String::new(), Some(Field::new("", DataType::Utf8, false).into()), )); let expr_field = expr.to_field(&schema).unwrap().1; diff --git a/datafusion/expr/src/higher_order_function.rs b/datafusion/expr/src/higher_order_function.rs index 9744e5520584a..9fcc61500543c 100644 --- a/datafusion/expr/src/higher_order_function.rs +++ b/datafusion/expr/src/higher_order_function.rs @@ -1219,7 +1219,9 @@ fn resolve_higher_order_function( // a map of lambda variable name => a never empty stack of fields [ [..shadowed], in_scope ] vars: &mut HashMap>, ) -> Result> { - let args = if !vars.is_empty() { + let args = if vars.is_empty() { + Transformed::no(args) + } else { /* if this is a nested lambda, we must resolve non-lambda args before invoking lambda_parameters because it will invoke ExprSchemable::to_field for every non-lambda parameter, and if one them contains a lambda variable, it will fail @@ -1234,8 +1236,6 @@ fn resolve_higher_order_function( Expr::Lambda(_) => Ok(Transformed::no(arg)), _ => resolve_lambda_variables(arg, schema, vars), })? - } else { - Transformed::no(args) }; let transformed = args.transformed; @@ -1580,7 +1580,7 @@ mod tests { None, ]) } - (1, Some(accumulator)) | (0, Some(accumulator)) => { + (0 | 1, Some(accumulator)) => { // now we can use the merge output as it's accumulator and // as the finish parameter LambdaParametersProgress::Complete(vec![ diff --git a/datafusion/expr/src/logical_plan/display.rs b/datafusion/expr/src/logical_plan/display.rs index 4afbb5670294a..e6443a927b0ee 100644 --- a/datafusion/expr/src/logical_plan/display.rs +++ b/datafusion/expr/src/logical_plan/display.rs @@ -485,7 +485,7 @@ impl<'a, 'b> PgJsonVisitor<'a, 'b> { let filter_expr = filter .as_ref() .map(|expr| format!(" Filter: {expr}")) - .unwrap_or_else(|| "".to_string()); + .unwrap_or_else(String::new); json!({ "Node Type": format!("{} Join", join_type), "Join Constraint": format!("{:?}", join_constraint), diff --git a/datafusion/expr/src/logical_plan/plan.rs b/datafusion/expr/src/logical_plan/plan.rs index 2c909b6534405..0f4870af9896b 100644 --- a/datafusion/expr/src/logical_plan/plan.rs +++ b/datafusion/expr/src/logical_plan/plan.rs @@ -1676,12 +1676,7 @@ impl LogicalPlan { let name_preserver = NamePreserver::new(&plan); plan.map_expressions(|e| { let (e, has_placeholder) = e.infer_placeholder_types(&schema)?; - if !has_placeholder { - // Performance optimization: - // avoid NamePreserver copy and second pass over expression - // if no placeholders. - Ok(Transformed::no(e)) - } else { + if has_placeholder { let original_name = name_preserver.save(&e); let transformed_expr = e.transform_up(|e| { if let Expr::Placeholder(Placeholder { id, .. }) = e { @@ -1695,6 +1690,11 @@ impl LogicalPlan { })?; // Preserve name to avoid breaking column references to this expression Ok(transformed_expr.update_data(|expr| original_name.restore(expr))) + } else { + // Performance optimization: + // avoid NamePreserver copy and second pass over expression + // if no placeholders. + Ok(Transformed::no(e)) } })? .map_data(|plan| plan.update_schema_data_type()) @@ -2036,7 +2036,7 @@ impl LogicalPlan { .collect(); format!(" projection=[{}]", names.join(", ")) } - _ => "".to_string(), + _ => String::new(), }; write!(f, "TableScan: {table_name}{projected_fields}")?; @@ -2176,7 +2176,7 @@ impl LogicalPlan { let filter_expr = filter .as_ref() .map(|expr| format!(" Filter: {expr}")) - .unwrap_or_else(|| "".to_string()); + .unwrap_or_else(String::new); let null_aware_expr = if *null_aware { " null_aware" } else { "" }; let join_type = if filter.is_none() @@ -2286,7 +2286,7 @@ impl LogicalPlan { if let Some(sort_expr) = sort_expr { expr_vec_fmt!(sort_expr) } else { - "".to_string() + String::new() }, ), }, @@ -4218,7 +4218,9 @@ fn calc_func_dependencies_for_aggregate( // - If so, the functional dependencies will be empty because we cannot guarantee // that GROUP BY expression results will be unique. // - Otherwise, it may be possible to propagate functional dependencies. - if !contains_grouping_set(group_expr) { + if contains_grouping_set(group_expr) { + Ok(FunctionalDependencies::empty()) + } else { let group_by_expr_names = group_expr .iter() .map(|item| item.schema_name().to_string()) @@ -4231,8 +4233,6 @@ fn calc_func_dependencies_for_aggregate( aggr_schema, ); Ok(aggregate_func_dependencies) - } else { - Ok(FunctionalDependencies::empty()) } } @@ -5993,7 +5993,7 @@ mod tests { .unwrap(); let prepared_builder = LogicalPlanBuilder::new(plan) .prepare( - "".to_string(), + String::new(), vec![Field::new("", DataType::Int32, true).into()], ) .unwrap(); diff --git a/datafusion/expr/src/predicate_bounds.rs b/datafusion/expr/src/predicate_bounds.rs index aa947416c87b5..25725edb26c65 100644 --- a/datafusion/expr/src/predicate_bounds.rs +++ b/datafusion/expr/src/predicate_bounds.rs @@ -83,23 +83,20 @@ impl PredicateBoundsEvaluator<'_> { } } Expr::IsNull(e) => { - // If `e` is not nullable, then `e IS NULL` is provably false - if !e.nullable(self.input_schema)? { - NullableInterval::FALSE - } else { + if e.nullable(self.input_schema)? { match e.get_type(self.input_schema)? { // If `e` is a boolean expression, check if `e` is provably 'unknown'. DataType::Boolean => self.evaluate_bounds(e)?.is_unknown()?, // If `e` is not a boolean expression, check if `e` is provably null _ => self.is_null(e), } + } else { + // `e` is not nullable, so `e IS NULL` is provably false + NullableInterval::FALSE } } Expr::IsNotNull(e) => { - // If `e` is not nullable, then `e IS NOT NULL` is provably true - if !e.nullable(self.input_schema)? { - NullableInterval::TRUE - } else { + if e.nullable(self.input_schema)? { match e.get_type(self.input_schema)? { // If `e` is a boolean expression, try to evaluate it and test for not unknown DataType::Boolean => { @@ -108,6 +105,9 @@ impl PredicateBoundsEvaluator<'_> { // If `e` is not a boolean expression, check if `e` is provably null _ => self.is_null(e).not()?, } + } else { + // `e` is not nullable, so `e IS NOT NULL` is provably true + NullableInterval::TRUE } } Expr::IsTrue(e) => self.evaluate_bounds(e)?.is_true()?, @@ -158,11 +158,11 @@ impl PredicateBoundsEvaluator<'_> { fn is_null(&self, expr: &Expr) -> NullableInterval { // Fast path for literals if let Expr::Literal(scalar, _) = expr { - if scalar.is_null() { - return NullableInterval::TRUE; + return if scalar.is_null() { + NullableInterval::TRUE } else { - return NullableInterval::FALSE; - } + NullableInterval::FALSE + }; } // If `expr` is not nullable, we can be certain `expr` is not null @@ -220,13 +220,13 @@ impl PredicateBoundsEvaluator<'_> { is_null = NullableInterval::TRUE_OR_FALSE; } - if !child_is_null.contains_value(ScalarValue::Boolean(Some(false)))? { + if child_is_null.contains_value(ScalarValue::Boolean(Some(false)))? { + Ok(TreeNodeRecursion::Continue) + } else { // If the child is never not null, then the result can also never be not null // and we can stop traversing the children is_null = NullableInterval::TRUE; Ok(TreeNodeRecursion::Stop) - } else { - Ok(TreeNodeRecursion::Continue) } }); diff --git a/datafusion/expr/src/registry.rs b/datafusion/expr/src/registry.rs index 4b9744d9573b6..2b5a0c6451210 100644 --- a/datafusion/expr/src/registry.rs +++ b/datafusion/expr/src/registry.rs @@ -422,7 +422,7 @@ impl Debug for ExtensionTypeRegistration { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { f.debug_struct("DefaultExtensionTypeRegistration") .field("type_name", &self.name) - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion/expr/src/test/function_stub.rs b/datafusion/expr/src/test/function_stub.rs index a1f29b649b2f8..6c99d06c9c3cf 100644 --- a/datafusion/expr/src/test/function_stub.rs +++ b/datafusion/expr/src/test/function_stub.rs @@ -220,7 +220,7 @@ impl std::fmt::Debug for Count { f.debug_struct("Count") .field("name", &self.name()) .field("signature", &self.signature) - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion/expr/src/tree_node.rs b/datafusion/expr/src/tree_node.rs index 941fd22ea179f..c9e83e0a46f50 100644 --- a/datafusion/expr/src/tree_node.rs +++ b/datafusion/expr/src/tree_node.rs @@ -64,8 +64,7 @@ impl TreeNode for Expr { | Expr::TryCast(TryCast { expr, .. }) | Expr::InSubquery(InSubquery { expr, .. }) | Expr::SetComparison(SetComparison { expr, .. }) => expr.apply_elements(f), - Expr::GroupingSet(GroupingSet::Rollup(exprs)) - | Expr::GroupingSet(GroupingSet::Cube(exprs)) => exprs.apply_elements(f), + Expr::GroupingSet(GroupingSet::Rollup(exprs) | GroupingSet::Cube(exprs)) => exprs.apply_elements(f), Expr::ScalarFunction(ScalarFunction { args, .. }) => { args.apply_elements(f) } diff --git a/datafusion/expr/src/type_coercion/functions.rs b/datafusion/expr/src/type_coercion/functions.rs index 781559ddd0c5c..213a5c6eeb267 100644 --- a/datafusion/expr/src/type_coercion/functions.rs +++ b/datafusion/expr/src/type_coercion/functions.rs @@ -115,17 +115,17 @@ pub fn fields_with_udf( let type_signature = &signature.type_signature; if current_fields.is_empty() && type_signature != &TypeSignature::UserDefined { - if type_signature.supports_zero_argument() { - return Ok(vec![]); + return if type_signature.supports_zero_argument() { + Ok(vec![]) } else if type_signature.used_to_support_zero_arguments() { // Special error to help during upgrade: https://github.com/apache/datafusion/issues/13763 - return plan_err!( + plan_err!( "'{}' does not support zero arguments. Use TypeSignature::Nullary for zero arguments", func.name() - ); + ) } else { - return plan_err!("'{}' does not support zero arguments", func.name()); - } + plan_err!("'{}' does not support zero arguments", func.name()) + }; } let current_types = current_fields .iter() @@ -246,15 +246,15 @@ pub fn value_fields_with_higher_order_udf( current_fields.iter().zip(expected.iter()).enumerate() { match (actual, expected) { - (ValueOrLambda::Value(_), ValueOrLambda::Value(_)) => {} - (ValueOrLambda::Lambda(_), ValueOrLambda::Lambda(_)) => {} - (ValueOrLambda::Value(_), ValueOrLambda::Lambda(_)) => { + (ValueOrLambda::Value(_), ValueOrLambda::Value(())) => {} + (ValueOrLambda::Lambda(_), ValueOrLambda::Lambda(())) => {} + (ValueOrLambda::Value(_), ValueOrLambda::Lambda(())) => { let name = func.name(); return plan_err!( "The function '{name}' expected a lambda at position {i} but received a value" ); } - (ValueOrLambda::Lambda(_), ValueOrLambda::Value(_)) => { + (ValueOrLambda::Lambda(_), ValueOrLambda::Value(())) => { let name = func.name(); return plan_err!( "The function '{name}' expected a value at position {i} but received a lambda" @@ -438,20 +438,20 @@ pub fn data_types( let type_signature = &signature.type_signature; if current_types.is_empty() && type_signature != &TypeSignature::UserDefined { - if type_signature.supports_zero_argument() { - return Ok(vec![]); + return if type_signature.supports_zero_argument() { + Ok(vec![]) } else if type_signature.used_to_support_zero_arguments() { // Special error to help during upgrade: https://github.com/apache/datafusion/issues/13763 - return plan_err!( + plan_err!( "function '{}' has signature {type_signature} which does not support zero arguments. Use TypeSignature::Nullary for zero arguments", function_name.as_ref() - ); + ) } else { - return plan_err!( + plan_err!( "Function '{}' has signature {type_signature} which does not support zero arguments", function_name.as_ref() - ); - } + ) + }; } let valid_types = @@ -566,9 +566,8 @@ fn get_valid_types_with_udf( func.name(), errors.join(",") ); - } else { - res } + res } _ => get_valid_types(func.name(), signature, current_types)?, }; diff --git a/datafusion/expr/src/udaf.rs b/datafusion/expr/src/udaf.rs index 8f7e9cc6cfc2b..6f286da334e49 100644 --- a/datafusion/expr/src/udaf.rs +++ b/datafusion/expr/src/udaf.rs @@ -1046,9 +1046,10 @@ impl<'a> UdafSchemaNameBuilder<'a> { } if !order_by.is_empty() { - let clause = match supports_within_group_clause { - true => "WITHIN GROUP", - false => "ORDER BY", + let clause = if supports_within_group_clause { + "WITHIN GROUP" + } else { + "ORDER BY" }; schema_name.write_fmt(format_args!( diff --git a/datafusion/expr/src/utils.rs b/datafusion/expr/src/utils.rs index 6c1dbacf4bb0e..0afba8bf0e835 100644 --- a/datafusion/expr/src/utils.rs +++ b/datafusion/expr/src/utils.rs @@ -394,10 +394,10 @@ fn get_exprs_except_skipped( .columns() .iter() .filter_map(|c| { - if !columns_to_skip.contains(c) { - Some(Expr::Column(c.clone())) - } else { + if columns_to_skip.contains(c) { None + } else { + Some(Expr::Column(c.clone())) } }) .collect::>() diff --git a/datafusion/ffi/src/session/mod.rs b/datafusion/ffi/src/session/mod.rs index 14ab5a04984a9..d6a1845cda14b 100644 --- a/datafusion/ffi/src/session/mod.rs +++ b/datafusion/ffi/src/session/mod.rs @@ -679,10 +679,10 @@ fn table_options_from_rhashmap(options: SVec<(SString, SString)>) -> TableOption .iter() .filter_map(|(k, v)| { let (prefix, _) = k.split_once('.')?; - if !["json", "parquet", "csv"].contains(&prefix) { - Some((k.to_owned(), v.to_owned())) - } else { + if ["json", "parquet", "csv"].contains(&prefix) { None + } else { + Some((k.to_owned(), v.to_owned())) } }) .collect(); diff --git a/datafusion/ffi/src/table_provider.rs b/datafusion/ffi/src/table_provider.rs index 5103297fcba57..463bfebb0d70e 100644 --- a/datafusion/ffi/src/table_provider.rs +++ b/datafusion/ffi/src/table_provider.rs @@ -592,9 +592,10 @@ impl FFI_TableProvider { schema: schema_fn_wrapper, scan: scan_fn_wrapper, table_type: table_type_fn_wrapper, - supports_filters_pushdown: match can_support_pushdown_filters { - true => Some(supports_filters_pushdown_fn_wrapper), - false => None, + supports_filters_pushdown: if can_support_pushdown_filters { + Some(supports_filters_pushdown_fn_wrapper) + } else { + None }, insert_into: insert_into_fn_wrapper, statistics: statistics_fn_wrapper, diff --git a/datafusion/ffi/src/tests/mod.rs b/datafusion/ffi/src/tests/mod.rs index fbc3e83ba49fc..059335b874862 100644 --- a/datafusion/ffi/src/tests/mod.rs +++ b/datafusion/ffi/src/tests/mod.rs @@ -162,9 +162,10 @@ extern "C" fn construct_table_provider( synchronous: bool, codec: FFI_LogicalExtensionCodec, ) -> FFI_TableProvider { - match synchronous { - true => create_sync_table_provider(codec), - false => create_async_table_provider(codec), + if synchronous { + create_sync_table_provider(codec) + } else { + create_async_table_provider(codec) } } diff --git a/datafusion/ffi/src/udwf/mod.rs b/datafusion/ffi/src/udwf/mod.rs index 9ba874dc6b686..b233b517b1154 100644 --- a/datafusion/ffi/src/udwf/mod.rs +++ b/datafusion/ffi/src/udwf/mod.rs @@ -350,11 +350,12 @@ impl WindowUDFImpl for ForeignWindowUDF { ))?; let schema: SchemaRef = schema.into(); - match schema.fields().is_empty() { - true => ffi_err!( + if schema.fields().is_empty() { + ffi_err!( "Unable to retrieve field in WindowUDF via FFI - schema has no fields" - ), - false => Ok(schema.field(0).to_owned().into()), + ) + } else { + Ok(schema.field(0).to_owned().into()) } } } diff --git a/datafusion/functions-aggregate-common/src/min_max.rs b/datafusion/functions-aggregate-common/src/min_max.rs index f0a4a52a4a060..d718415d3a596 100644 --- a/datafusion/functions-aggregate-common/src/min_max.rs +++ b/datafusion/functions-aggregate-common/src/min_max.rs @@ -399,12 +399,18 @@ fn min_max_scalar_same_variant( ordering, )) } - (ScalarValue::IntervalYearMonth(_), ScalarValue::IntervalMonthDayNano(_)) - | (ScalarValue::IntervalYearMonth(_), ScalarValue::IntervalDayTime(_)) - | (ScalarValue::IntervalMonthDayNano(_), ScalarValue::IntervalDayTime(_)) - | (ScalarValue::IntervalMonthDayNano(_), ScalarValue::IntervalYearMonth(_)) - | (ScalarValue::IntervalDayTime(_), ScalarValue::IntervalYearMonth(_)) - | (ScalarValue::IntervalDayTime(_), ScalarValue::IntervalMonthDayNano(_)) => { + ( + ScalarValue::IntervalYearMonth(_) | ScalarValue::IntervalDayTime(_), + ScalarValue::IntervalMonthDayNano(_), + ) + | ( + ScalarValue::IntervalYearMonth(_) | ScalarValue::IntervalMonthDayNano(_), + ScalarValue::IntervalDayTime(_), + ) + | ( + ScalarValue::IntervalMonthDayNano(_) | ScalarValue::IntervalDayTime(_), + ScalarValue::IntervalYearMonth(_), + ) => { return min_max_interval_scalar(lhs, rhs, ordering); } (ScalarValue::DurationSecond(lhs), ScalarValue::DurationSecond(rhs)) => { diff --git a/datafusion/functions-aggregate/src/approx_distinct.rs b/datafusion/functions-aggregate/src/approx_distinct.rs index 1746edd8239f2..f61794adf3fc0 100644 --- a/datafusion/functions-aggregate/src/approx_distinct.rs +++ b/datafusion/functions-aggregate/src/approx_distinct.rs @@ -888,17 +888,20 @@ fn is_hll_groups_type(data_type: &DataType) -> bool { | DataType::Int64 | DataType::Date32 | DataType::Date64 - | DataType::Time32(TimeUnit::Second) - | DataType::Time32(TimeUnit::Millisecond) - | DataType::Time64(TimeUnit::Microsecond) - | DataType::Time64(TimeUnit::Nanosecond) - | DataType::Timestamp(TimeUnit::Second, _) - | DataType::Timestamp(TimeUnit::Millisecond, _) - | DataType::Timestamp(TimeUnit::Microsecond, _) - | DataType::Timestamp(TimeUnit::Nanosecond, _) - | DataType::Interval(IntervalUnit::YearMonth) - | DataType::Interval(IntervalUnit::DayTime) - | DataType::Interval(IntervalUnit::MonthDayNano) + | DataType::Time32(TimeUnit::Second | TimeUnit::Millisecond) + | DataType::Time64(TimeUnit::Microsecond | TimeUnit::Nanosecond) + | DataType::Timestamp( + TimeUnit::Second + | TimeUnit::Millisecond + | TimeUnit::Microsecond + | TimeUnit::Nanosecond, + _ + ) + | DataType::Interval( + IntervalUnit::YearMonth + | IntervalUnit::DayTime + | IntervalUnit::MonthDayNano + ) | DataType::Decimal32(_, _) | DataType::Decimal64(_, _) | DataType::Decimal128(_, _) diff --git a/datafusion/functions-aggregate/src/correlation.rs b/datafusion/functions-aggregate/src/correlation.rs index b9bc57dfa989c..14568ef272152 100644 --- a/datafusion/functions-aggregate/src/correlation.rs +++ b/datafusion/functions-aggregate/src/correlation.rs @@ -212,11 +212,11 @@ impl Accumulator for CorrelationAccumulator { && let ScalarValue::Float64(Some(s1)) = stddev1 && let ScalarValue::Float64(Some(s2)) = stddev2 { - if s1 == 0_f64 || s2 == 0_f64 { - return Ok(ScalarValue::Float64(None)); + return if s1 == 0_f64 || s2 == 0_f64 { + Ok(ScalarValue::Float64(None)) } else { - return Ok(ScalarValue::Float64(Some(c / s1 / s2))); - } + Ok(ScalarValue::Float64(Some(c / s1 / s2))) + }; } Ok(ScalarValue::Float64(None)) diff --git a/datafusion/functions-aggregate/src/first_last.rs b/datafusion/functions-aggregate/src/first_last.rs index d36ea5d63074b..94673ea4551d9 100644 --- a/datafusion/functions-aggregate/src/first_last.rs +++ b/datafusion/functions-aggregate/src/first_last.rs @@ -599,10 +599,7 @@ impl FirstLastGroupsAccumulator { let is_valid = self.extreme_of_each_group_buf.1.get_bit(group_idx); - if !is_valid { - self.extreme_of_each_group_buf.1.set_bit(group_idx, true); - self.extreme_of_each_group_buf.0[group_idx] = idx_in_val; - } else { + if is_valid { let ordering = comparator .compare(self.extreme_of_each_group_buf.0[group_idx], idx_in_val); @@ -611,6 +608,9 @@ impl FirstLastGroupsAccumulator { { self.extreme_of_each_group_buf.0[group_idx] = idx_in_val; } + } else { + self.extreme_of_each_group_buf.1.set_bit(group_idx, true); + self.extreme_of_each_group_buf.0[group_idx] = idx_in_val; } } @@ -912,10 +912,9 @@ impl FirstValueAccumulator { } } return Ok(None); - } else { - // If not ignoring nulls, return the first value if it exists. - return Ok((!value.is_empty()).then_some(0)); } + // If not ignoring nulls, return the first value if it exists. + return Ok((!value.is_empty()).then_some(0)); } let sort_columns = ordering_values @@ -1301,9 +1300,8 @@ impl LastValueAccumulator { } } return Ok(None); - } else { - return Ok((!value.is_empty()).then_some(value.len() - 1)); } + return Ok((!value.is_empty()).then_some(value.len() - 1)); } let sort_columns = ordering_values diff --git a/datafusion/functions-aggregate/src/nth_value.rs b/datafusion/functions-aggregate/src/nth_value.rs index 5e7f9c6c3186b..8175272212ddd 100644 --- a/datafusion/functions-aggregate/src/nth_value.rs +++ b/datafusion/functions-aggregate/src/nth_value.rs @@ -51,14 +51,14 @@ pub fn nth_value( order_by: Vec, ) -> datafusion_expr::Expr { let args = vec![expr, lit(n)]; - if !order_by.is_empty() { + if order_by.is_empty() { + nth_value_udaf().call(args) + } else { nth_value_udaf() .call(args) .order_by(order_by) .build() .unwrap() - } else { - nth_value_udaf().call(args) } } diff --git a/datafusion/functions-aggregate/src/variance.rs b/datafusion/functions-aggregate/src/variance.rs index 1ca258298dc6b..232f15fa774d7 100644 --- a/datafusion/functions-aggregate/src/variance.rs +++ b/datafusion/functions-aggregate/src/variance.rs @@ -98,27 +98,26 @@ impl AggregateUDFImpl for VarianceSample { fn state_fields(&self, args: StateFieldsArgs) -> Result> { let name = args.name; - match args.is_distinct { - false => Ok(vec![ + if args.is_distinct { + let field = Field::new_list_field(DataType::Float64, true); + let state_name = "distinct_var"; + Ok(vec![ + Field::new( + format_state_name(name, state_name), + DataType::List(Arc::new(field)), + true, + ) + .into(), + ]) + } else { + Ok(vec![ Field::new(format_state_name(name, "count"), DataType::UInt64, true), Field::new(format_state_name(name, "mean"), DataType::Float64, true), Field::new(format_state_name(name, "m2"), DataType::Float64, true), ] .into_iter() .map(Arc::new) - .collect()), - true => { - let field = Field::new_list_field(DataType::Float64, true); - let state_name = "distinct_var"; - Ok(vec![ - Field::new( - format_state_name(name, state_name), - DataType::List(Arc::new(field)), - true, - ) - .into(), - ]) - } + .collect()) } } @@ -193,30 +192,27 @@ impl AggregateUDFImpl for VariancePopulation { } fn state_fields(&self, args: StateFieldsArgs) -> Result> { - match args.is_distinct { - false => { - let name = args.name; - Ok(vec![ - Field::new(format_state_name(name, "count"), DataType::UInt64, true), - Field::new(format_state_name(name, "mean"), DataType::Float64, true), - Field::new(format_state_name(name, "m2"), DataType::Float64, true), - ] - .into_iter() - .map(Arc::new) - .collect()) - } - true => { - let field = Field::new_list_field(DataType::Float64, true); - let state_name = "distinct_var"; - Ok(vec![ - Field::new( - format_state_name(args.name, state_name), - DataType::List(Arc::new(field)), - true, - ) - .into(), - ]) - } + if args.is_distinct { + let field = Field::new_list_field(DataType::Float64, true); + let state_name = "distinct_var"; + Ok(vec![ + Field::new( + format_state_name(args.name, state_name), + DataType::List(Arc::new(field)), + true, + ) + .into(), + ]) + } else { + let name = args.name; + Ok(vec![ + Field::new(format_state_name(name, "count"), DataType::UInt64, true), + Field::new(format_state_name(name, "mean"), DataType::Float64, true), + Field::new(format_state_name(name, "m2"), DataType::Float64, true), + ] + .into_iter() + .map(Arc::new) + .collect()) } } @@ -653,10 +649,10 @@ impl Accumulator for DistinctVarianceAccumulator { let count = match self.stat_type { StatsType::Sample => { - if !values.is_empty() { - values.len() - 1 - } else { + if values.is_empty() { 0 + } else { + values.len() - 1 } } StatsType::Population => values.len(), diff --git a/datafusion/functions-nested/src/array_has.rs b/datafusion/functions-nested/src/array_has.rs index 24a9913d2c121..b01abe1c0ce12 100644 --- a/datafusion/functions-nested/src/array_has.rs +++ b/datafusion/functions-nested/src/array_has.rs @@ -142,9 +142,9 @@ impl ScalarUDFImpl for ArrayHas { None, ))); } + // FixedSizeList gets coerced to List Expr::Literal( - // FixedSizeList gets coerced to List - scalar @ ScalarValue::List(_) | scalar @ ScalarValue::LargeList(_), + scalar @ (ScalarValue::List(_) | ScalarValue::LargeList(_)), _, ) => { if let Ok(scalar_values) = @@ -355,9 +355,7 @@ fn array_has_dispatch_for_array( // Fast path for primitive/string elements whose (coerced) type matches the // needle; a type mismatch or a nested type falls through to the per-row kernel. - let fast_path = if visible_values.data_type() != needle.data_type() { - None - } else { + let fast_path = if visible_values.data_type() == needle.data_type() { downcast_primitive_array! { visible_values => { // The element-null path makes several passes over the values, so @@ -397,6 +395,8 @@ fn array_has_dispatch_for_array( )), _ => None, } + } else { + None }; if let Some(values) = fast_path { diff --git a/datafusion/functions-nested/src/except.rs b/datafusion/functions-nested/src/except.rs index 737a3122bbbae..f5b746d93bd15 100644 --- a/datafusion/functions-nested/src/except.rs +++ b/datafusion/functions-nested/src/except.rs @@ -138,10 +138,10 @@ fn array_except_inner(args: &[ArrayRef]) -> Result { &DataType::new_list(DataType::Null, true), len, )), - (DataType::Null, dt @ DataType::List(_)) - | (DataType::Null, dt @ DataType::LargeList(_)) - | (dt @ DataType::List(_), DataType::Null) - | (dt @ DataType::LargeList(_), DataType::Null) => Ok(new_null_array(dt, len)), + (DataType::Null, dt @ (DataType::List(_) | DataType::LargeList(_))) + | (dt @ (DataType::List(_) | DataType::LargeList(_)), DataType::Null) => { + Ok(new_null_array(dt, len)) + } (DataType::List(field), DataType::List(_)) => { check_datatypes("array_except", &[array1, array2])?; let list1 = array1.as_list::(); diff --git a/datafusion/functions-nested/src/planner.rs b/datafusion/functions-nested/src/planner.rs index e96fdb7d4baca..8ca7bde758f60 100644 --- a/datafusion/functions-nested/src/planner.rs +++ b/datafusion/functions-nested/src/planner.rs @@ -85,13 +85,13 @@ impl ExprPlanner for NestedFunctionPlanner { let right_list_ndims = list_ndims(&right_type); // if both are list if left_list_ndims > 0 && right_list_ndims > 0 { - if op == BinaryOperator::AtArrow { + return if op == BinaryOperator::AtArrow { // array1 @> array2 -> array_has_all(array1, array2) - return Ok(PlannerResult::Planned(array_has_all(left, right))); + Ok(PlannerResult::Planned(array_has_all(left, right))) } else { // array1 <@ array2 -> array_has_all(array2, array1) - return Ok(PlannerResult::Planned(array_has_all(right, left))); - } + Ok(PlannerResult::Planned(array_has_all(right, left))) + }; } } diff --git a/datafusion/functions-nested/src/range.rs b/datafusion/functions-nested/src/range.rs index f9384586d685f..b903e83020cb7 100644 --- a/datafusion/functions-nested/src/range.rs +++ b/datafusion/functions-nested/src/range.rs @@ -379,15 +379,15 @@ impl Range { return exec_err!("Cannot generate date range less than 1 day."); } - let stop = if !self.include_upper_bound { + let stop = if self.include_upper_bound { + stop + } else { Date32Type::subtract_month_day_nano_opt(stop, step).ok_or_else(|| { exec_datafusion_err!( "Cannot generate date range where stop {} - {step:?}) overflows", date32_to_string(stop) ) })? - } else { - stop }; let neg = months < 0 || days < 0; diff --git a/datafusion/functions-nested/src/set_ops.rs b/datafusion/functions-nested/src/set_ops.rs index 2214d3d35bb7b..f8991246eac6a 100644 --- a/datafusion/functions-nested/src/set_ops.rs +++ b/datafusion/functions-nested/src/set_ops.rs @@ -519,10 +519,9 @@ fn general_set_op( let len = array1.len(); match (array1.data_type(), array2.data_type()) { (Null, Null) => Ok(new_null_array(&DataType::new_list(Null, true), len)), - (Null, dt @ List(_)) - | (Null, dt @ LargeList(_)) - | (dt @ List(_), Null) - | (dt @ LargeList(_), Null) => Ok(new_null_array(dt, len)), + (Null, dt @ (List(_) | LargeList(_))) | (dt @ (List(_) | LargeList(_)), Null) => { + Ok(new_null_array(dt, len)) + } (List(field), List(_)) => { let array1 = as_list_array(&array1)?; let array2 = as_list_array(&array2)?; diff --git a/datafusion/functions-table/src/generate_series.rs b/datafusion/functions-table/src/generate_series.rs index cf5e026581584..5806e07643728 100644 --- a/datafusion/functions-table/src/generate_series.rs +++ b/datafusion/functions-table/src/generate_series.rs @@ -758,8 +758,7 @@ impl GenerateSeriesFuncImpl { // Parse start date let start_date = match &exprs[0] { Expr::Literal(ScalarValue::Date32(Some(date)), _) => *date, - Expr::Literal(ScalarValue::Date32(None), _) - | Expr::Literal(ScalarValue::Null, _) => { + Expr::Literal(ScalarValue::Date32(None) | ScalarValue::Null, _) => { return Ok(Arc::new(GenerateSeriesTable { schema, args: GenSeriesArgs::ContainsNull { name: self.name }, @@ -776,8 +775,7 @@ impl GenerateSeriesFuncImpl { // Parse end date let end_date = match &exprs[1] { Expr::Literal(ScalarValue::Date32(Some(date)), _) => *date, - Expr::Literal(ScalarValue::Date32(None), _) - | Expr::Literal(ScalarValue::Null, _) => { + Expr::Literal(ScalarValue::Date32(None) | ScalarValue::Null, _) => { return Ok(Arc::new(GenerateSeriesTable { schema, args: GenSeriesArgs::ContainsNull { name: self.name }, @@ -796,8 +794,10 @@ impl GenerateSeriesFuncImpl { Expr::Literal(ScalarValue::IntervalMonthDayNano(Some(interval)), _) => { *interval } - Expr::Literal(ScalarValue::IntervalMonthDayNano(None), _) - | Expr::Literal(ScalarValue::Null, _) => { + Expr::Literal( + ScalarValue::IntervalMonthDayNano(None) | ScalarValue::Null, + _, + ) => { return Ok(Arc::new(GenerateSeriesTable { schema, args: GenSeriesArgs::ContainsNull { name: self.name }, diff --git a/datafusion/functions-window/src/lead_lag.rs b/datafusion/functions-window/src/lead_lag.rs index fea4a1a4aadda..d96685d476acf 100644 --- a/datafusion/functions-window/src/lead_lag.rs +++ b/datafusion/functions-window/src/lead_lag.rs @@ -709,10 +709,10 @@ impl PartitionEvaluator for WindowShiftEvaluator { // - ignore nulls mode and current value is null and is within window bounds // .unwrap() is safe here as there is a none check in front #[expect(clippy::unnecessary_unwrap)] - if !(idx.is_none() || (self.ignore_nulls && array.is_null(idx.unwrap()))) { - ScalarValue::try_from_array(array, idx.unwrap()) - } else { + if idx.is_none() || (self.ignore_nulls && array.is_null(idx.unwrap())) { Ok(self.default_value.clone()) + } else { + ScalarValue::try_from_array(array, idx.unwrap()) } } @@ -723,15 +723,15 @@ impl PartitionEvaluator for WindowShiftEvaluator { ) -> Result { // LEAD, LAG window functions take single column, values will have size 1 let value = &values[0]; - if !self.ignore_nulls { - shift_with_default_value(value, self.shift_offset, &self.default_value) - } else { + if self.ignore_nulls { evaluate_all_with_ignore_null( value, self.shift_offset, &self.default_value, self.is_lag(), ) + } else { + shift_with_default_value(value, self.shift_offset, &self.default_value) } } diff --git a/datafusion/functions-window/src/nth_value.rs b/datafusion/functions-window/src/nth_value.rs index b3678e80f2273..75da3ab443f9e 100644 --- a/datafusion/functions-window/src/nth_value.rs +++ b/datafusion/functions-window/src/nth_value.rs @@ -407,9 +407,8 @@ impl PartitionEvaluator for NthValueEvaluator { state.window_frame_range.end - 1; } return Ok(()); - } else { - // Fall through to the main case because there are no nulls } + // Fall through to the main case because there are no nulls } // Do not memoize for other kinds when nulls are ignored NthValueKind::Last | NthValueKind::Nth => return Ok(()), diff --git a/datafusion/functions/benches/datetime_expressions/to_char.rs b/datafusion/functions/benches/datetime_expressions/to_char.rs index 2be28806c9e8c..a48eef570dca7 100644 --- a/datafusion/functions/benches/datetime_expressions/to_char.rs +++ b/datafusion/functions/benches/datetime_expressions/to_char.rs @@ -109,9 +109,10 @@ fn pick_date_time_pattern(rng: &mut StdRng) -> String { } fn pick_date_and_date_time_mixed_pattern(rng: &mut StdRng) -> String { - match rng.random_bool(0.5) { - true => pick_date_pattern(rng), - false => pick_date_time_pattern(rng), + if rng.random_bool(0.5) { + pick_date_pattern(rng) + } else { + pick_date_time_pattern(rng) } } diff --git a/datafusion/functions/src/core/greatest_least_utils.rs b/datafusion/functions/src/core/greatest_least_utils.rs index 2714a01832175..f21b5ba1e15dc 100644 --- a/datafusion/functions/src/core/greatest_least_utils.rs +++ b/datafusion/functions/src/core/greatest_least_utils.rs @@ -77,7 +77,11 @@ pub(super) fn execute_conditional( let mut result: ArrayRef; // Optimization: merge all scalars into one to avoid recomputing (constant folding) - if !scalars.is_empty() { + if scalars.is_empty() { + // If we only have arrays, start with the first array + // (We must have at least one array) + result = Arc::clone(first_array.unwrap()); + } else { let mut scalars_iter = scalars.iter().map(|x| match x { ColumnarValue::Scalar(s) => s, _ => unreachable!(), @@ -103,10 +107,6 @@ pub(super) fn execute_conditional( first_array, &result_scalar.to_array_of_size(first_array.len())?, )?; - } else { - // If we only have arrays, start with the first array - // (We must have at least one array) - result = Arc::clone(first_array.unwrap()); } for array in arrays_iter { diff --git a/datafusion/functions/src/core/nullif.rs b/datafusion/functions/src/core/nullif.rs index f58ae857d4791..309d14e59d193 100644 --- a/datafusion/functions/src/core/nullif.rs +++ b/datafusion/functions/src/core/nullif.rs @@ -138,9 +138,10 @@ fn nullif_func(args: &[ColumnarValue]) -> Result { Ok(ColumnarValue::Array(array)) } (ColumnarValue::Scalar(lhs), ColumnarValue::Scalar(rhs)) => { - let val: ScalarValue = match lhs.eq(rhs) { - true => lhs.data_type().try_into()?, - false => lhs.clone(), + let val: ScalarValue = if lhs.eq(rhs) { + lhs.data_type().try_into()? + } else { + lhs.clone() }; Ok(ColumnarValue::Scalar(val)) diff --git a/datafusion/functions/src/datetime/common.rs b/datafusion/functions/src/datetime/common.rs index 118b6b371bc17..39707e907c53d 100644 --- a/datafusion/functions/src/datetime/common.rs +++ b/datafusion/functions/src/datetime/common.rs @@ -517,9 +517,8 @@ where if let Ok(inner) = r { val = Some(Ok(op2(inner))); break; - } else { - val = Some(r); } + val = Some(r); } } diff --git a/datafusion/functions/src/datetime/date_bin.rs b/datafusion/functions/src/datetime/date_bin.rs index 15cdecc3c2842..4e59e87cd1925 100644 --- a/datafusion/functions/src/datetime/date_bin.rs +++ b/datafusion/functions/src/datetime/date_bin.rs @@ -531,9 +531,8 @@ fn date_bin_impl( return not_impl_err!( "DATE_BIN stride does not support combination of month, day and nanosecond intervals" ); - } else { - Interval::Months(months as i64) } + Interval::Months(months as i64) } else { let nanos = (TimeDelta::try_days(days as i64).unwrap() + Duration::nanoseconds(nanos)) diff --git a/datafusion/functions/src/datetime/to_time.rs b/datafusion/functions/src/datetime/to_time.rs index f5fe59cbb87b0..85eb116cca92c 100644 --- a/datafusion/functions/src/datetime/to_time.rs +++ b/datafusion/functions/src/datetime/to_time.rs @@ -144,9 +144,9 @@ fn string_to_time(args: &[ColumnarValue]) -> Result { let formats = compile_formats(&formats); match &args[0] { - ColumnarValue::Scalar(ScalarValue::Utf8(s)) - | ColumnarValue::Scalar(ScalarValue::LargeUtf8(s)) - | ColumnarValue::Scalar(ScalarValue::Utf8View(s)) => { + ColumnarValue::Scalar( + ScalarValue::Utf8(s) | ScalarValue::LargeUtf8(s) | ScalarValue::Utf8View(s), + ) => { let result = s .as_ref() .map(|s| parse_time_with_formats(s, &formats)) @@ -175,14 +175,18 @@ fn collect_formats(args: &[ColumnarValue]) -> Result> { let mut formats = Vec::with_capacity(args.len() - 1); for (i, arg) in args[1..].iter().enumerate() { match arg { - ColumnarValue::Scalar(ScalarValue::Utf8(Some(s))) - | ColumnarValue::Scalar(ScalarValue::LargeUtf8(Some(s))) - | ColumnarValue::Scalar(ScalarValue::Utf8View(Some(s))) => { + ColumnarValue::Scalar( + ScalarValue::Utf8(Some(s)) + | ScalarValue::LargeUtf8(Some(s)) + | ScalarValue::Utf8View(Some(s)), + ) => { formats.push(s.as_str()); } - ColumnarValue::Scalar(ScalarValue::Utf8(None)) - | ColumnarValue::Scalar(ScalarValue::LargeUtf8(None)) - | ColumnarValue::Scalar(ScalarValue::Utf8View(None)) => { + ColumnarValue::Scalar( + ScalarValue::Utf8(None) + | ScalarValue::LargeUtf8(None) + | ScalarValue::Utf8View(None), + ) => { // Skip null format strings } ColumnarValue::Array(_) => { diff --git a/datafusion/functions/src/math/round.rs b/datafusion/functions/src/math/round.rs index 02c808a4d346e..1ef0919b577d6 100644 --- a/datafusion/functions/src/math/round.rs +++ b/datafusion/functions/src/math/round.rs @@ -1127,7 +1127,7 @@ mod test { assert!(result.is_err()); assert!(matches!( result, - Err(DataFusionError::ArrowError(_, _)) | Err(DataFusionError::Execution(_)) + Err(DataFusionError::ArrowError(_, _) | DataFusionError::Execution(_)) )); } } diff --git a/datafusion/functions/src/regex/regexpcount.rs b/datafusion/functions/src/regex/regexpcount.rs index 2920b687ed33f..e19c4ea358220 100644 --- a/datafusion/functions/src/regex/regexpcount.rs +++ b/datafusion/functions/src/regex/regexpcount.rs @@ -674,7 +674,7 @@ mod tests { let re = regexp_count_with_scalar_values(&[ ScalarValue::Utf8(Some(value.to_string())), - ScalarValue::Utf8(Some("".to_string())), + ScalarValue::Utf8(Some(String::new())), start_sv.clone(), ]); match re { @@ -686,7 +686,7 @@ mod tests { let re = regexp_count_with_scalar_values(&[ ScalarValue::LargeUtf8(Some(value.to_string())), - ScalarValue::LargeUtf8(Some("".to_string())), + ScalarValue::LargeUtf8(Some(String::new())), start_sv.clone(), ]); match re { @@ -698,7 +698,7 @@ mod tests { let re = regexp_count_with_scalar_values(&[ ScalarValue::Utf8View(Some(value.to_string())), - ScalarValue::Utf8View(Some("".to_string())), + ScalarValue::Utf8View(Some(String::new())), start_sv, ]); match re { diff --git a/datafusion/functions/src/regex/regexplike.rs b/datafusion/functions/src/regex/regexplike.rs index e7b31b767a4b0..d9e98734be922 100644 --- a/datafusion/functions/src/regex/regexplike.rs +++ b/datafusion/functions/src/regex/regexplike.rs @@ -187,16 +187,16 @@ impl ScalarUDFImpl for RegexpLikeFunc { let string = args.swap_remove(0); Ok(ExprSimplifyResult::Simplified(binary_expr( - if string_type != coerced_string_type { - cast(string, coerced_string_type) - } else { + if string_type == coerced_string_type { string + } else { + cast(string, coerced_string_type) }, op, - if regexp_type != coerced_regexp_type { - cast(regexp, coerced_regexp_type) - } else { + if regexp_type == coerced_regexp_type { regexp + } else { + cast(regexp, coerced_regexp_type) }, ))) } diff --git a/datafusion/functions/src/string/ascii.rs b/datafusion/functions/src/string/ascii.rs index db539a4d11719..8e99fb66afad2 100644 --- a/datafusion/functions/src/string/ascii.rs +++ b/datafusion/functions/src/string/ascii.rs @@ -242,7 +242,7 @@ mod tests { fn test_functions() -> Result<()> { test_ascii!(Some(String::from("x")), Ok(Some(120))); test_ascii!(Some(String::from("a")), Ok(Some(97))); - test_ascii!(Some(String::from("")), Ok(Some(0))); + test_ascii!(Some(String::new()), Ok(Some(0))); test_ascii!(Some(String::from("🚀")), Ok(Some(128640))); test_ascii!(Some(String::from("\n")), Ok(Some(10))); test_ascii!(Some(String::from("\t")), Ok(Some(9))); diff --git a/datafusion/functions/src/string/concat.rs b/datafusion/functions/src/string/concat.rs index aa42d918eb4b7..aa42dc64f29a3 100644 --- a/datafusion/functions/src/string/concat.rs +++ b/datafusion/functions/src/string/concat.rs @@ -319,7 +319,7 @@ pub(crate) fn simplify_concat(args: Vec) -> Result { } let mut new_args = Vec::with_capacity(args.len()); - let mut contiguous_scalar = "".to_string(); + let mut contiguous_scalar = String::new(); let return_type = { let data_types: Vec<_> = args @@ -369,7 +369,7 @@ pub(crate) fn simplify_concat(args: Vec) -> Result { .push(lit(ScalarValue::Utf8View(Some(contiguous_scalar)))), _ => unreachable!(), } - contiguous_scalar = "".to_string(); + contiguous_scalar = String::new(); } new_args.push(arg); } @@ -389,15 +389,15 @@ pub(crate) fn simplify_concat(args: Vec) -> Result { } } - if !args.eq(&new_args) { + if args.eq(&new_args) { + Ok(ExprSimplifyResult::Original(args)) + } else { Ok(ExprSimplifyResult::Simplified(Expr::ScalarFunction( ScalarFunction { func: concat(), args: new_args, }, ))) - } else { - Ok(ExprSimplifyResult::Original(args)) } } diff --git a/datafusion/functions/src/string/octet_length.rs b/datafusion/functions/src/string/octet_length.rs index 02df262ee27aa..73d52140d6573 100644 --- a/datafusion/functions/src/string/octet_length.rs +++ b/datafusion/functions/src/string/octet_length.rs @@ -185,9 +185,9 @@ mod tests { ); test_function!( OctetLengthFunc::new(), - vec![ColumnarValue::Scalar(ScalarValue::Utf8(Some( - String::from("") - )))], + vec![ColumnarValue::Scalar(ScalarValue::Utf8( + Some(String::new()) + ))], Ok(Some(0)), i32, Int32, @@ -224,7 +224,7 @@ mod tests { test_function!( OctetLengthFunc::new(), vec![ColumnarValue::Scalar(ScalarValue::Utf8View(Some( - String::from("") + String::new() )))], Ok(Some(0)), i32, diff --git a/datafusion/functions/src/string/replace.rs b/datafusion/functions/src/string/replace.rs index 549b8e1a3b0f9..0f84ed16e446c 100644 --- a/datafusion/functions/src/string/replace.rs +++ b/datafusion/functions/src/string/replace.rs @@ -429,7 +429,7 @@ mod tests { ReplaceFunc::new(), vec![ ColumnarValue::Scalar(ScalarValue::LargeUtf8(Some(String::from("abc")))), - ColumnarValue::Scalar(ScalarValue::LargeUtf8(Some(String::from("")))), + ColumnarValue::Scalar(ScalarValue::LargeUtf8(Some(String::new()))), ColumnarValue::Scalar(ScalarValue::LargeUtf8(Some(String::from("x")))), ], Ok(Some("abc")), diff --git a/datafusion/functions/src/string/split_part.rs b/datafusion/functions/src/string/split_part.rs index 9b73a1af88501..d11bb90c13e9b 100644 --- a/datafusion/functions/src/string/split_part.rs +++ b/datafusion/functions/src/string/split_part.rs @@ -747,7 +747,7 @@ mod tests { SplitPartFunc::new(), vec![ ColumnarValue::Scalar(ScalarValue::Utf8(Some(String::from("a,b")))), - ColumnarValue::Scalar(ScalarValue::Utf8(Some(String::from("")))), + ColumnarValue::Scalar(ScalarValue::Utf8(Some(String::new()))), ColumnarValue::Scalar(ScalarValue::Int64(Some(1))), ], Ok(Some("a,b")), @@ -759,7 +759,7 @@ mod tests { SplitPartFunc::new(), vec![ ColumnarValue::Scalar(ScalarValue::Utf8(Some(String::from("a,b")))), - ColumnarValue::Scalar(ScalarValue::Utf8(Some(String::from("")))), + ColumnarValue::Scalar(ScalarValue::Utf8(Some(String::new()))), ColumnarValue::Scalar(ScalarValue::Int64(Some(2))), ], Ok(Some("")), @@ -797,7 +797,7 @@ mod tests { SplitPartFunc::new(), vec![ ColumnarValue::Scalar(ScalarValue::Utf8(Some(String::from("a,b")))), - ColumnarValue::Scalar(ScalarValue::Utf8(Some(String::from("")))), + ColumnarValue::Scalar(ScalarValue::Utf8(Some(String::new()))), ColumnarValue::Scalar(ScalarValue::Int64(Some(-1))), ], Ok(Some("a,b")), @@ -821,7 +821,7 @@ mod tests { SplitPartFunc::new(), vec![ ColumnarValue::Scalar(ScalarValue::Utf8(Some(String::from("a,b")))), - ColumnarValue::Scalar(ScalarValue::Utf8(Some(String::from("")))), + ColumnarValue::Scalar(ScalarValue::Utf8(Some(String::new()))), ColumnarValue::Scalar(ScalarValue::Int64(Some(-2))), ], Ok(Some("")), diff --git a/datafusion/functions/src/strings.rs b/datafusion/functions/src/strings.rs index c788c6fb1f33f..9df357f2a7e73 100644 --- a/datafusion/functions/src/strings.rs +++ b/datafusion/functions/src/strings.rs @@ -1284,9 +1284,11 @@ impl ColumnarValueRef<'_> { convert_to_str: bool, ) -> Result>> { match col { - ColumnarValue::Scalar(ScalarValue::Utf8(maybe_value)) - | ColumnarValue::Scalar(ScalarValue::LargeUtf8(maybe_value)) - | ColumnarValue::Scalar(ScalarValue::Utf8View(maybe_value)) => { + ColumnarValue::Scalar( + ScalarValue::Utf8(maybe_value) + | ScalarValue::LargeUtf8(maybe_value) + | ScalarValue::Utf8View(maybe_value), + ) => { if let Some(s) = maybe_value { *data_size += s.len() * len * size_factor; Ok(Some(ColumnarValueRef::Scalar(s.as_bytes()))) @@ -1294,10 +1296,12 @@ impl ColumnarValueRef<'_> { Ok(None) } } - ColumnarValue::Scalar(ScalarValue::Binary(maybe_value)) - | ColumnarValue::Scalar(ScalarValue::LargeBinary(maybe_value)) - | ColumnarValue::Scalar(ScalarValue::BinaryView(maybe_value)) - | ColumnarValue::Scalar(ScalarValue::FixedSizeBinary(_, maybe_value)) => { + ColumnarValue::Scalar( + ScalarValue::Binary(maybe_value) + | ScalarValue::LargeBinary(maybe_value) + | ScalarValue::BinaryView(maybe_value) + | ScalarValue::FixedSizeBinary(_, maybe_value), + ) => { if let Some(b) = maybe_value { *data_size += b.len() * len * size_factor; Ok(Some(ColumnarValueRef::Scalar(b.as_slice()))) diff --git a/datafusion/functions/src/unicode/character_length.rs b/datafusion/functions/src/unicode/character_length.rs index 9f0d952a02636..e92ab2b494a1a 100644 --- a/datafusion/functions/src/unicode/character_length.rs +++ b/datafusion/functions/src/unicode/character_length.rs @@ -228,7 +228,7 @@ mod tests { test_character_length!(Some(String::from("josé")), Ok(Some(4))); // test long strings (more than 12 bytes for StringView) test_character_length!(Some(String::from("joséjoséjoséjosé")), Ok(Some(16))); - test_character_length!(Some(String::from("")), Ok(Some(0))); + test_character_length!(Some(String::new()), Ok(Some(0))); test_character_length!(None, Ok(None)); } diff --git a/datafusion/functions/src/unicode/find_in_set.rs b/datafusion/functions/src/unicode/find_in_set.rs index fa23532406ce1..b2acabf840212 100644 --- a/datafusion/functions/src/unicode/find_in_set.rs +++ b/datafusion/functions/src/unicode/find_in_set.rs @@ -435,7 +435,7 @@ mod tests { test_function!( FindInSetFunc::new(), vec![ - ColumnarValue::Scalar(ScalarValue::Utf8(Some(String::from("")))), + ColumnarValue::Scalar(ScalarValue::Utf8(Some(String::new()))), ColumnarValue::Scalar(ScalarValue::Utf8(Some(String::from("a,b,c")))), ], Ok(Some(0)), @@ -447,7 +447,7 @@ mod tests { FindInSetFunc::new(), vec![ ColumnarValue::Scalar(ScalarValue::Utf8(Some(String::from("a")))), - ColumnarValue::Scalar(ScalarValue::Utf8(Some(String::from("")))), + ColumnarValue::Scalar(ScalarValue::Utf8(Some(String::new()))), ], Ok(Some(0)), i32, diff --git a/datafusion/functions/src/unicode/initcap.rs b/datafusion/functions/src/unicode/initcap.rs index 0332ab5d4427f..ddbfa3eb1936a 100644 --- a/datafusion/functions/src/unicode/initcap.rs +++ b/datafusion/functions/src/unicode/initcap.rs @@ -398,7 +398,7 @@ mod tests { test_function!( InitcapFunc::new(), vec![ColumnarValue::Scalar(ScalarValue::Utf8View(Some( - "".to_string() + String::new() )))], Ok(Some("")), &str, diff --git a/datafusion/functions/src/unicode/left.rs b/datafusion/functions/src/unicode/left.rs index 0788e69d92528..bbf7d9ac3554f 100644 --- a/datafusion/functions/src/unicode/left.rs +++ b/datafusion/functions/src/unicode/left.rs @@ -281,7 +281,7 @@ mod tests { test_function!( LeftFunc::new(), vec![ - ColumnarValue::Scalar(ScalarValue::Utf8View(Some("".to_string()))), + ColumnarValue::Scalar(ScalarValue::Utf8View(Some(String::new()))), ColumnarValue::Scalar(ScalarValue::from(200i64)), ], Ok(Some("")), diff --git a/datafusion/functions/src/unicode/lpad.rs b/datafusion/functions/src/unicode/lpad.rs index 0ffd02714957c..616178513d52d 100644 --- a/datafusion/functions/src/unicode/lpad.rs +++ b/datafusion/functions/src/unicode/lpad.rs @@ -202,7 +202,9 @@ fn lpad_scalar_ascii<'a, V: StringArrayType<'a> + Copy, T: OffsetSizeTrait>( ) -> Result { // With a scalar `target_len` and `fill`, we can precompute a padding // buffer of `target_len` fill characters repeated cyclically. - let padding_buf = if !fill.is_empty() { + let padding_buf = if fill.is_empty() { + String::new() + } else { let mut buf = String::with_capacity(target_len); while buf.len() < target_len { let remaining = target_len - buf.len(); @@ -213,8 +215,6 @@ fn lpad_scalar_ascii<'a, V: StringArrayType<'a> + Copy, T: OffsetSizeTrait>( } } buf - } else { - String::new() }; // Each output row is exactly `target_len` ASCII bytes (padding + string). @@ -254,7 +254,9 @@ fn lpad_scalar_unicode<'a, V: StringArrayType<'a> + Copy, T: OffsetSizeTrait>( // of `target_len` fill characters repeated cyclically. Because Unicode // characters are variable-width, we build a byte-offset table to map from // character count to the corresponding byte position in the padding buffer. - let (padding_buf, char_byte_offsets) = if !fill_chars.is_empty() { + let (padding_buf, char_byte_offsets) = if fill_chars.is_empty() { + (String::new(), vec![0]) + } else { let mut buf = String::new(); let mut offsets = Vec::with_capacity(target_len + 1); offsets.push(0usize); @@ -263,8 +265,6 @@ fn lpad_scalar_unicode<'a, V: StringArrayType<'a> + Copy, T: OffsetSizeTrait>( offsets.push(buf.len()); } (buf, offsets) - } else { - (String::new(), vec![0]) }; // Each output row is `target_len` chars; multiply by 4 (max UTF-8 bytes @@ -725,7 +725,7 @@ mod tests { test_lpad!( Some("hi".into()), ScalarValue::Int64(Some(5i64)), - Some("".into()), + Some(String::new()), Ok(Some("hi")) ); test_lpad!( diff --git a/datafusion/functions/src/unicode/right.rs b/datafusion/functions/src/unicode/right.rs index 21fb0690a11a2..77d155cbe5aeb 100644 --- a/datafusion/functions/src/unicode/right.rs +++ b/datafusion/functions/src/unicode/right.rs @@ -281,7 +281,7 @@ mod tests { test_function!( RightFunc::new(), vec![ - ColumnarValue::Scalar(ScalarValue::Utf8View(Some("".to_string()))), + ColumnarValue::Scalar(ScalarValue::Utf8View(Some(String::new()))), ColumnarValue::Scalar(ScalarValue::from(200i64)), ], Ok(Some("")), diff --git a/datafusion/functions/src/unicode/rpad.rs b/datafusion/functions/src/unicode/rpad.rs index d89ebd0057f3c..5e16f0c992ead 100644 --- a/datafusion/functions/src/unicode/rpad.rs +++ b/datafusion/functions/src/unicode/rpad.rs @@ -202,7 +202,9 @@ fn rpad_scalar_ascii<'a, V: StringArrayType<'a> + Copy, T: OffsetSizeTrait>( ) -> Result { // With a scalar `target_len` and `fill`, we can precompute a padding // buffer of `target_len` fill characters repeated cyclically. - let padding_buf = if !fill.is_empty() { + let padding_buf = if fill.is_empty() { + String::new() + } else { let mut buf = String::with_capacity(target_len); while buf.len() < target_len { let remaining = target_len - buf.len(); @@ -213,8 +215,6 @@ fn rpad_scalar_ascii<'a, V: StringArrayType<'a> + Copy, T: OffsetSizeTrait>( } } buf - } else { - String::new() }; // Each output row is exactly `target_len` ASCII bytes (string + padding). @@ -255,7 +255,9 @@ fn rpad_scalar_unicode<'a, V: StringArrayType<'a> + Copy, T: OffsetSizeTrait>( // of `target_len` fill characters repeated cyclically. Because Unicode // characters are variable-width, we build a byte-offset table to map from // character count to the corresponding byte position in the padding buffer. - let (padding_buf, char_byte_offsets) = if !fill_chars.is_empty() { + let (padding_buf, char_byte_offsets) = if fill_chars.is_empty() { + (String::new(), vec![0]) + } else { let mut buf = String::new(); let mut offsets = Vec::with_capacity(target_len + 1); offsets.push(0usize); @@ -264,8 +266,6 @@ fn rpad_scalar_unicode<'a, V: StringArrayType<'a> + Copy, T: OffsetSizeTrait>( offsets.push(buf.len()); } (buf, offsets) - } else { - (String::new(), vec![0]) }; // Each output row is `target_len` chars; multiply by 4 (max UTF-8 bytes diff --git a/datafusion/functions/src/unicode/translate.rs b/datafusion/functions/src/unicode/translate.rs index 85e83897f41da..eae8fcdaa7aca 100644 --- a/datafusion/functions/src/unicode/translate.rs +++ b/datafusion/functions/src/unicode/translate.rs @@ -340,8 +340,11 @@ fn append_translated_ascii( input: &str, table: &AsciiTranslateTable, ) { - // Fast path: equal-length byte-to-byte map when no deletions. - if !table.has_delete { + if table.has_delete { + builder.append_with(|w| write_translated_ascii(w, input, table)); + } else { + // Fast path: equal-length byte-to-byte map when there are no deletions. + // // SAFETY: ASCII source bytes map to ASCII replacements; non-ASCII // bytes 128..256 map to themselves, so multi-byte UTF-8 sequences // pass through unchanged. Output length equals input length and @@ -349,8 +352,6 @@ fn append_translated_ascii( unsafe { builder.append_byte_map(input.as_bytes(), |b| table.map[b as usize]); } - } else { - builder.append_with(|w| write_translated_ascii(w, input, table)); } } diff --git a/datafusion/optimizer/src/analyzer/type_coercion.rs b/datafusion/optimizer/src/analyzer/type_coercion.rs index 0f82f0b0df764..25a8101cd0bf5 100644 --- a/datafusion/optimizer/src/analyzer/type_coercion.rs +++ b/datafusion/optimizer/src/analyzer/type_coercion.rs @@ -376,7 +376,9 @@ impl<'a> TypeCoercionRewriter<'a> { // handle special cases for // * Date +/- int => Date // * Date + time => Timestamp - let left_expr = if !left_cast_ok { + let left_expr = if left_cast_ok { + left.cast_to(&left_type, left_schema)? + } else { Self::coerce_date_time_math_op( left, &op, @@ -384,11 +386,11 @@ impl<'a> TypeCoercionRewriter<'a> { &left_type, &right_type, )? - } else { - left.cast_to(&left_type, left_schema)? }; - let right_expr = if !right_cast_ok { + let right_expr = if right_cast_ok { + right.cast_to(&right_type, right_schema)? + } else { Self::coerce_date_time_math_op( right, &op, @@ -396,8 +398,6 @@ impl<'a> TypeCoercionRewriter<'a> { &right_type, &left_type, )? - } else { - right.cast_to(&right_type, right_schema)? }; Ok((left_expr, right_expr)) @@ -2744,10 +2744,10 @@ mod test { data_type: &DataType, schema: &DFSchemaRef, ) -> Box { - if &expr.get_type(schema).unwrap() != data_type { - Box::new(cast(*expr, data_type.clone())) - } else { + if &expr.get_type(schema).unwrap() == data_type { expr + } else { + Box::new(cast(*expr, data_type.clone())) } } diff --git a/datafusion/optimizer/src/decorrelate.rs b/datafusion/optimizer/src/decorrelate.rs index 0c37f00b64355..371922672b59a 100644 --- a/datafusion/optimizer/src/decorrelate.rs +++ b/datafusion/optimizer/src/decorrelate.rs @@ -351,13 +351,13 @@ impl TreeNodeRewriter for PullUpCorrelatedExpr { } let new_plan = if alias.input.schema().fields().len() - != alias.schema.fields().len() + == alias.schema.fields().len() { + plan.clone() + } else { LogicalPlanBuilder::from((*alias.input).clone()) .alias(alias.alias.clone())? .build()? - } else { - plan.clone() }; self.correlated_subquery_cols_map @@ -368,10 +368,10 @@ impl TreeNodeRewriter for PullUpCorrelatedExpr { .insert(new_plan.clone(), input_map.clone()); } - if new_plan != plan { - Ok(Transformed::yes(new_plan)) - } else { + if new_plan == plan { Ok(Transformed::no(plan)) + } else { + Ok(Transformed::yes(new_plan)) } } LogicalPlan::Limit(limit) => { @@ -618,8 +618,9 @@ fn filter_exprs_evaluation_result_on_empty_batch( let result_expr = simplifier.simplify(result_expr)?; match &result_expr { // evaluate to false or null on empty batch, no need to pull up - Expr::Literal(ScalarValue::Null, _) - | Expr::Literal(ScalarValue::Boolean(Some(false)), _) => None, + Expr::Literal(ScalarValue::Null | ScalarValue::Boolean(Some(false)), _) => { + None + } // evaluate to true on empty batch, need to pull up the expr Expr::Literal(ScalarValue::Boolean(Some(true)), _) => { for (name, exprs) in input_expr_result_map_for_count_bug { diff --git a/datafusion/optimizer/src/decorrelate_predicate_subquery.rs b/datafusion/optimizer/src/decorrelate_predicate_subquery.rs index 66ea0806bd3b6..a74985e593974 100644 --- a/datafusion/optimizer/src/decorrelate_predicate_subquery.rs +++ b/datafusion/optimizer/src/decorrelate_predicate_subquery.rs @@ -273,9 +273,10 @@ fn build_join_top( }) .map_or(Ok(None), |v| v.map(Some))?; - let join_type = match query_info.negated { - true => JoinType::LeftAnti, - false => JoinType::LeftSemi, + let join_type = if query_info.negated { + JoinType::LeftAnti + } else { + JoinType::LeftSemi }; let subquery = query_info.query.subquery.as_ref(); let subquery_alias = alias.next("__correlated_sq"); @@ -439,13 +440,13 @@ fn build_join( .map(|(_, c)| Expr::Column(c)) .collect(); - let right_projected = if !right_proj_exprs.is_empty() { + let right_projected = if right_proj_exprs.is_empty() { + // Degenerate case: no right columns referenced by the predicate(s) + sub_query_alias.clone() + } else { LogicalPlanBuilder::from(sub_query_alias.clone()) .project(right_proj_exprs)? .build()? - } else { - // Degenerate case: no right columns referenced by the predicate(s) - sub_query_alias.clone() }; // Mark joins don't use null-aware semantics (they use three-valued logic with mark column) @@ -524,14 +525,20 @@ impl SubqueryInfo { pub fn expr(self) -> Expr { match self.where_in_expr { - Some(expr) => match self.negated { - true => not_in_subquery(expr, self.query.subquery), - false => in_subquery(expr, self.query.subquery), - }, - None => match self.negated { - true => not_exists(self.query.subquery), - false => exists(self.query.subquery), - }, + Some(expr) => { + if self.negated { + not_in_subquery(expr, self.query.subquery) + } else { + in_subquery(expr, self.query.subquery) + } + } + None => { + if self.negated { + not_exists(self.query.subquery) + } else { + exists(self.query.subquery) + } + } } } } diff --git a/datafusion/optimizer/src/eliminate_duplicated_expr.rs b/datafusion/optimizer/src/eliminate_duplicated_expr.rs index 97aa6e1d8480d..9114e64fd971b 100644 --- a/datafusion/optimizer/src/eliminate_duplicated_expr.rs +++ b/datafusion/optimizer/src/eliminate_duplicated_expr.rs @@ -94,10 +94,10 @@ impl OptimizerRule for EliminateDuplicatedExpr { unique_exprs }; - let transformed = if len != unique_exprs.len() { - Transformed::yes - } else { + let transformed = if len == unique_exprs.len() { Transformed::no + } else { + Transformed::yes }; if unique_exprs.is_empty() { @@ -122,10 +122,10 @@ impl OptimizerRule for EliminateDuplicatedExpr { .into_iter() .collect(); - let transformed = if len != unique_exprs.len() { - Transformed::yes - } else { + let transformed = if len == unique_exprs.len() { Transformed::no + } else { + Transformed::yes }; Aggregate::try_new(agg.input, unique_exprs, agg.aggr_expr) diff --git a/datafusion/optimizer/src/extract_equijoin_predicate.rs b/datafusion/optimizer/src/extract_equijoin_predicate.rs index 0a50761e8a9f7..b567d84191b52 100644 --- a/datafusion/optimizer/src/extract_equijoin_predicate.rs +++ b/datafusion/optimizer/src/extract_equijoin_predicate.rs @@ -123,9 +123,8 @@ impl OptimizerRule for ExtractEquijoinPredicate { } } - if !equijoin_predicates.is_empty() { - on.extend(equijoin_predicates); - Ok(Transformed::yes(LogicalPlan::Join(Join { + if equijoin_predicates.is_empty() { + Ok(Transformed::no(LogicalPlan::Join(Join { left, right, on, @@ -137,7 +136,8 @@ impl OptimizerRule for ExtractEquijoinPredicate { null_aware, }))) } else { - Ok(Transformed::no(LogicalPlan::Join(Join { + on.extend(equijoin_predicates); + Ok(Transformed::yes(LogicalPlan::Join(Join { left, right, on, diff --git a/datafusion/optimizer/src/optimize_projections/mod.rs b/datafusion/optimizer/src/optimize_projections/mod.rs index b0543a871f52b..b7363b36573c4 100644 --- a/datafusion/optimizer/src/optimize_projections/mod.rs +++ b/datafusion/optimizer/src/optimize_projections/mod.rs @@ -698,14 +698,15 @@ fn rewrite_expr(expr: Expr, input: &Projection) -> Result> { match expr { // remove any intermediate aliases if they do not carry metadata Expr::Alias(alias) => { - match alias + if alias .metadata .as_ref() .map(|h| h.is_empty()) .unwrap_or(true) { - true => Ok(Transformed::yes(*alias.expr)), - false => Ok(Transformed::no(Expr::Alias(alias))), + Ok(Transformed::yes(*alias.expr)) + } else { + Ok(Transformed::no(Expr::Alias(alias))) } } Expr::Column(col) => { diff --git a/datafusion/optimizer/src/optimizer.rs b/datafusion/optimizer/src/optimizer.rs index 46c01180958c9..33a9b6cf7ca6d 100644 --- a/datafusion/optimizer/src/optimizer.rs +++ b/datafusion/optimizer/src/optimizer.rs @@ -423,13 +423,10 @@ fn map_children_mut Result>( f(Arc::make_mut(input))? } LogicalPlan::Explain(Explain { plan, .. }) => f(Arc::make_mut(plan))?, - LogicalPlan::Ddl(DdlStatement::CreateMemoryTable(CreateMemoryTable { - input, - .. - })) - | LogicalPlan::Ddl(DdlStatement::CreateView(CreateView { input, .. })) => { - f(Arc::make_mut(input))? - } + LogicalPlan::Ddl( + DdlStatement::CreateMemoryTable(CreateMemoryTable { input, .. }) + | DdlStatement::CreateView(CreateView { input, .. }), + ) => f(Arc::make_mut(input))?, LogicalPlan::RecursiveQuery(RecursiveQuery { static_term, recursive_term, @@ -470,15 +467,17 @@ fn map_children_mut Result>( | LogicalPlan::EmptyRelation { .. } | LogicalPlan::Values { .. } | LogicalPlan::DescribeTable(_) - | LogicalPlan::Ddl(DdlStatement::CreateExternalTable(_)) - | LogicalPlan::Ddl(DdlStatement::CreateCatalogSchema(_)) - | LogicalPlan::Ddl(DdlStatement::CreateCatalog(_)) - | LogicalPlan::Ddl(DdlStatement::CreateIndex(_)) - | LogicalPlan::Ddl(DdlStatement::DropTable(_)) - | LogicalPlan::Ddl(DdlStatement::DropView(_)) - | LogicalPlan::Ddl(DdlStatement::DropCatalogSchema(_)) - | LogicalPlan::Ddl(DdlStatement::CreateFunction(_)) - | LogicalPlan::Ddl(DdlStatement::DropFunction(_)) + | LogicalPlan::Ddl( + DdlStatement::CreateExternalTable(_) + | DdlStatement::CreateCatalogSchema(_) + | DdlStatement::CreateCatalog(_) + | DdlStatement::CreateIndex(_) + | DdlStatement::DropTable(_) + | DdlStatement::DropView(_) + | DdlStatement::DropCatalogSchema(_) + | DdlStatement::CreateFunction(_) + | DdlStatement::DropFunction(_), + ) | LogicalPlan::Statement(_) => false, }) } diff --git a/datafusion/optimizer/src/propagate_empty_relation.rs b/datafusion/optimizer/src/propagate_empty_relation.rs index 18ddc361a0692..75e6432613f0a 100644 --- a/datafusion/optimizer/src/propagate_empty_relation.rs +++ b/datafusion/optimizer/src/propagate_empty_relation.rs @@ -255,13 +255,13 @@ fn empty_child(plan: &LogicalPlan) -> Result> { match plan.inputs()[..] { [child] => match child { LogicalPlan::EmptyRelation(empty) => { - if !empty.produce_one_row { + if empty.produce_one_row { + Ok(None) + } else { Ok(Some(LogicalPlan::EmptyRelation(EmptyRelation { produce_one_row: false, schema: Arc::clone(plan.schema()), }))) - } else { - Ok(None) } } _ => Ok(None), @@ -339,8 +339,7 @@ fn has_empty_grouping_set(group_expr: &[Expr]) -> bool { groups.iter().any(|g| g.is_empty()) } // Both ROLLUP and CUBE always include the empty grouping set (). - Some(Expr::GroupingSet(GroupingSet::Rollup(_))) - | Some(Expr::GroupingSet(GroupingSet::Cube(_))) => true, + Some(Expr::GroupingSet(GroupingSet::Rollup(_) | GroupingSet::Cube(_))) => true, _ => false, } } diff --git a/datafusion/optimizer/src/push_down_filter.rs b/datafusion/optimizer/src/push_down_filter.rs index 8a1dcc12ef874..5676af58cab3b 100644 --- a/datafusion/optimizer/src/push_down_filter.rs +++ b/datafusion/optimizer/src/push_down_filter.rs @@ -1174,9 +1174,8 @@ impl OptimizerRule for PushDownFilter { { filter.input = Arc::new(LogicalPlan::TableScan(scan)); return Ok(Transformed::no(LogicalPlan::Filter(filter))); - } else { - scan.filters = new_scan_filters; } + scan.filters = new_scan_filters; // Compose predicates to be of `Unsupported` or `Inexact` pushdown type, // and also include volatile and subquery-containing filters @@ -1230,10 +1229,10 @@ impl OptimizerRule for PushDownFilter { .into_iter() .zip(split_conjunction_owned(filter.predicate)) { - if !push { - keep_predicates.push(expr); - } else { + if push { push_predicates.push(expr); + } else { + keep_predicates.push(expr); } } diff --git a/datafusion/optimizer/src/scalar_subquery_to_join.rs b/datafusion/optimizer/src/scalar_subquery_to_join.rs index 44011a125ba96..6a966c4f0e4d5 100644 --- a/datafusion/optimizer/src/scalar_subquery_to_join.rs +++ b/datafusion/optimizer/src/scalar_subquery_to_join.rs @@ -223,10 +223,10 @@ impl OptimizerRule for ScalarSubqueryToJoin { for (expr, new_expr) in projection.expr.iter().zip(rewrite_exprs) { let old_expr_name = expr.schema_name().to_string(); let new_expr_name = new_expr.schema_name().to_string(); - if new_expr_name != old_expr_name { - proj_exprs.push(new_expr.alias(old_expr_name)) - } else { + if new_expr_name == old_expr_name { proj_exprs.push(new_expr); + } else { + proj_exprs.push(new_expr.alias(old_expr_name)) } } let new_plan = LogicalPlanBuilder::from(cur_input) diff --git a/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs b/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs index dadea4784802a..76b67df748129 100644 --- a/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs +++ b/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs @@ -705,9 +705,10 @@ impl ConstEvaluator { .ok() .and_then(|f| { let m = f.metadata(); - match m.is_empty() { - true => None, - false => Some(FieldMetadata::from(m)), + if m.is_empty() { + None + } else { + Some(FieldMetadata::from(m)) } }); let col_val = match phys_expr.evaluate(&DUMMY_BATCH) { @@ -843,13 +844,14 @@ impl TreeNodeRewriter for Simplifier<'_> { op: Eq, right, }) if (left == right) & !left.is_volatile() => { - Transformed::yes(match !info.nullable(&left)? { - true => lit(true), - false => Expr::BinaryExpr(BinaryExpr { + Transformed::yes(if info.nullable(&left)? { + Expr::BinaryExpr(BinaryExpr { left: Box::new(Expr::IsNotNull(left)), op: Or, right: Box::new(lit_bool_null()), - }), + }) + } else { + lit(true) }) } @@ -1488,12 +1490,10 @@ impl TreeNodeRewriter for Simplifier<'_> { // CASE WHEN false THEN A ELSE B END --> B if let Some(else_expr) = else_expr { return Ok(Transformed::yes(*else_expr)); - // CASE WHEN false THEN A END --> NULL - } else { - let null = - Expr::Literal(ScalarValue::try_new_null(&out_type)?, None); - return Ok(Transformed::yes(null)); } + // CASE WHEN false THEN A END --> NULL + let null = Expr::Literal(ScalarValue::try_new_null(&out_type)?, None); + return Ok(Transformed::yes(null)); } Transformed::yes(Expr::Case(Case { @@ -1669,9 +1669,7 @@ impl TreeNodeRewriter for Simplifier<'_> { // - when exp is not NULL, it's false // - when exp is NULL, it's NULL let result_for_non_null = lit(!like.negated); - Transformed::yes(if !info.nullable(&like.expr)? { - result_for_non_null - } else { + Transformed::yes(if info.nullable(&like.expr)? { Expr::Case(Case { expr: Some(Box::new(Expr::IsNotNull(like.expr))), when_then_expr: vec![( @@ -1680,6 +1678,8 @@ impl TreeNodeRewriter for Simplifier<'_> { )], else_expr: None, }) + } else { + result_for_non_null }) } Some(pattern_str) @@ -2306,14 +2306,14 @@ fn inlist_except(mut l1: InList, l2: &InList) -> Result { /// Returns expression testing a boolean `expr` for being exactly `true` (not `false` or NULL). fn is_exactly_true(expr: Expr, info: &SimplifyContext) -> Result { - if !info.nullable(&expr)? { - Ok(expr) - } else { + if info.nullable(&expr)? { Ok(Expr::BinaryExpr(BinaryExpr { left: Box::new(expr), op: Operator::IsNotDistinctFrom, right: Box::new(lit(true)), })) + } else { + Ok(expr) } } @@ -2333,10 +2333,10 @@ fn simplify_right_is_one_case( match BinaryTypeCoercer::new(&left_type, op, &right_type).get_result_type() { Ok(result_type) => { // Only cast if the types differ - if left_type != result_type { - Ok(Transformed::yes(Expr::Cast(Cast::new(left, result_type)))) - } else { + if left_type == result_type { Ok(Transformed::yes(*left)) + } else { + Ok(Transformed::yes(Expr::Cast(Cast::new(left, result_type)))) } } Err(_) => Ok(Transformed::yes(*left)), diff --git a/datafusion/optimizer/src/simplify_expressions/inlist_simplifier.rs b/datafusion/optimizer/src/simplify_expressions/inlist_simplifier.rs index 17112d4f0ae24..a5b27da3d8b18 100644 --- a/datafusion/optimizer/src/simplify_expressions/inlist_simplifier.rs +++ b/datafusion/optimizer/src/simplify_expressions/inlist_simplifier.rs @@ -55,8 +55,8 @@ impl TreeNodeRewriter for ShortenInListSimplifier { ) { let first_val = list[0].clone(); - if negated { - return Ok(Transformed::yes(list.iter().skip(1).cloned().fold( + return if negated { + Ok(Transformed::yes(list.iter().skip(1).cloned().fold( (*expr.clone()).not_eq(first_val), |acc, y| { // Note that `A and B and C and D` is a left-deep tree structure @@ -78,16 +78,16 @@ impl TreeNodeRewriter for ShortenInListSimplifier { // The code below maintain the left-deep tree structure. acc.and((*expr.clone()).not_eq(y)) }, - ))); + ))) } else { - return Ok(Transformed::yes(list.iter().skip(1).cloned().fold( + Ok(Transformed::yes(list.iter().skip(1).cloned().fold( (*expr.clone()).eq(first_val), |acc, y| { // Same reasoning as above acc.or((*expr.clone()).eq(y)) }, - ))); - } + ))) + }; } Ok(Transformed::no(expr)) diff --git a/datafusion/optimizer/src/simplify_expressions/simplify_exprs.rs b/datafusion/optimizer/src/simplify_expressions/simplify_exprs.rs index 1c5a4a1869ddb..3f051e9c8463a 100644 --- a/datafusion/optimizer/src/simplify_expressions/simplify_exprs.rs +++ b/datafusion/optimizer/src/simplify_expressions/simplify_exprs.rs @@ -205,10 +205,10 @@ fn rewrite_aggregate_non_aggregate_aggr_expr( let new_plan = LogicalPlan::Aggregate(Aggregate::try_new_with_schema( input, group_expr, aggr_expr, schema, )?); - return if !rewrote_aggs { - Ok(Transformed::no(new_plan)) - } else { + return if rewrote_aggs { Ok(Transformed::yes(new_plan)) + } else { + Ok(Transformed::no(new_plan)) }; } diff --git a/datafusion/optimizer/src/simplify_expressions/simplify_predicates.rs b/datafusion/optimizer/src/simplify_expressions/simplify_predicates.rs index 356f2711b708e..756cd8cba727c 100644 --- a/datafusion/optimizer/src/simplify_expressions/simplify_predicates.rs +++ b/datafusion/optimizer/src/simplify_expressions/simplify_predicates.rs @@ -116,14 +116,14 @@ fn simplify_column_predicates(predicates: Vec) -> Result> { match &pred { Expr::BinaryExpr(BinaryExpr { left: _, op, right }) => { match (op, right.as_literal().is_some()) { - (Operator::Gt, true) - | (Operator::Lt, false) - | (Operator::GtEq, true) - | (Operator::LtEq, false) => greater_predicates.push(pred), - (Operator::Lt, true) - | (Operator::Gt, false) - | (Operator::LtEq, true) - | (Operator::GtEq, false) => less_predicates.push(pred), + (Operator::Gt | Operator::GtEq, true) + | (Operator::Lt | Operator::LtEq, false) => { + greater_predicates.push(pred) + } + (Operator::Lt | Operator::LtEq, true) + | (Operator::Gt | Operator::GtEq, false) => { + less_predicates.push(pred) + } (Operator::Eq, _) => eq_predicates.push(pred), _ => unreachable!("Unexpected operator: {}", op), } @@ -275,20 +275,20 @@ mod tests { // Check that the cast predicate is preserved let has_cast_predicate = result.iter().any(|p| { - matches!(p, Expr::BinaryExpr(BinaryExpr { - left, - op: Operator::Lt, - right + matches!(p, Expr::BinaryExpr(BinaryExpr { + left, + op: Operator::Lt, + right }) if matches!(left.as_ref(), Expr::Cast(_)) && right == &Box::new(lit("abc"))) }); assert!(has_cast_predicate, "Cast predicate should be preserved"); // Check that we have the more restrictive column predicate (a < 5) let has_column_predicate = result.iter().any(|p| { - matches!(p, Expr::BinaryExpr(BinaryExpr { - left, - op: Operator::Lt, - right + matches!(p, Expr::BinaryExpr(BinaryExpr { + left, + op: Operator::Lt, + right }) if left == &Box::new(col("a")) && right == &Box::new(lit(5i32))) }); assert!(has_column_predicate, "Should have a < 5 predicate"); @@ -322,20 +322,20 @@ mod tests { // Check for a < 3 let has_a_predicate = result.iter().any(|p| { - matches!(p, Expr::BinaryExpr(BinaryExpr { - left, - op: Operator::Lt, - right + matches!(p, Expr::BinaryExpr(BinaryExpr { + left, + op: Operator::Lt, + right }) if left == &Box::new(col("a")) && right == &Box::new(lit(3i32))) }); assert!(has_a_predicate, "Should have a < 3 predicate"); // Check for b > 20 let has_b_predicate = result.iter().any(|p| { - matches!(p, Expr::BinaryExpr(BinaryExpr { - left, - op: Operator::Gt, - right + matches!(p, Expr::BinaryExpr(BinaryExpr { + left, + op: Operator::Gt, + right }) if left == &Box::new(col("b")) && right == &Box::new(lit(20i32))) }); assert!(has_b_predicate, "Should have b > 20 predicate"); diff --git a/datafusion/optimizer/src/simplify_expressions/utils.rs b/datafusion/optimizer/src/simplify_expressions/utils.rs index 89bb762d59ce2..1f79356321b9e 100644 --- a/datafusion/optimizer/src/simplify_expressions/utils.rs +++ b/datafusion/optimizer/src/simplify_expressions/utils.rs @@ -79,19 +79,19 @@ pub fn delete_xor_in_complex_expr(expr: &Expr, needle: &Expr, is_left: bool) -> if result_expr == *needle { return needle.clone(); } else if xor_counter % 2 == 0 { - if is_left { - return Expr::BinaryExpr(BinaryExpr::new( + return if is_left { + Expr::BinaryExpr(BinaryExpr::new( Box::new(needle.clone()), Operator::BitwiseXor, Box::new(result_expr), - )); + )) } else { - return Expr::BinaryExpr(BinaryExpr::new( + Expr::BinaryExpr(BinaryExpr::new( Box::new(result_expr), Operator::BitwiseXor, Box::new(needle.clone()), - )); - } + )) + }; } result_expr } @@ -228,12 +228,7 @@ pub fn is_eq_and_ne_with_different_literal(eq_expr: &Expr, ne_expr: &Expr) -> bo match expr { Expr::BinaryExpr(BinaryExpr { left, - op: Operator::Eq, - right, - }) - | Expr::BinaryExpr(BinaryExpr { - left, - op: Operator::NotEq, + op: Operator::Eq | Operator::NotEq, right, }) => match (left.as_ref(), right.as_ref()) { (Expr::Literal(_, _), var) => Some((var, left)), diff --git a/datafusion/optimizer/src/utils.rs b/datafusion/optimizer/src/utils.rs index d4ac31e8a517c..8c5394df5cebc 100644 --- a/datafusion/optimizer/src/utils.rs +++ b/datafusion/optimizer/src/utils.rs @@ -183,10 +183,9 @@ pub fn is_restrict_null_predicate<'a>( false } } - ColumnarValue::Scalar(scalar) => matches!( - scalar, - ScalarValue::Boolean(None) | ScalarValue::Boolean(Some(false)) - ), + ColumnarValue::Scalar(scalar) => { + matches!(scalar, ScalarValue::Boolean(None | Some(false))) + } }, ) } diff --git a/datafusion/physical-expr-common/src/binary_map.rs b/datafusion/physical-expr-common/src/binary_map.rs index 4523fee7cda43..50c6e4c19ca6f 100644 --- a/datafusion/physical-expr-common/src/binary_map.rs +++ b/datafusion/physical-expr-common/src/binary_map.rs @@ -571,7 +571,7 @@ where .field("buffer", &self.buffer) .field("random_state", &self.random_state) .field("hashes_buffer", &self.hashes_buffer) - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion/physical-expr-common/src/binary_view_map.rs b/datafusion/physical-expr-common/src/binary_view_map.rs index 6154616b6e9e7..42e073f38b225 100644 --- a/datafusion/physical-expr-common/src/binary_view_map.rs +++ b/datafusion/physical-expr-common/src/binary_view_map.rs @@ -503,7 +503,7 @@ where .field("completed_buffers", &self.completed.len()) .field("random_state", &self.random_state) .field("hashes_buffer", &self.hashes_buffer) - .finish() + .finish_non_exhaustive() } } @@ -740,7 +740,7 @@ mod tests { ])); let mut map = ArrowBytesViewMap::new(OutputType::Utf8View); - map.insert_if_new(&values, |_| (), |_| {}); + map.insert_if_new(&values, |_| (), |()| {}); // Make unused vector capacity explicit; the completed buffers were created // by the map's flush path. @@ -785,7 +785,7 @@ mod tests { assert_eq!(map.size() - legacy_size, retained_capacity_delta); let size_after_insert = map.size(); - map.insert_if_new(&values, |_| (), |_| {}); + map.insert_if_new(&values, |_| (), |()| {}); assert_eq!(map.size(), size_after_insert); } diff --git a/datafusion/physical-expr-common/src/datum.rs b/datafusion/physical-expr-common/src/datum.rs index d23fb30db6c4a..a286637dd99de 100644 --- a/datafusion/physical-expr-common/src/datum.rs +++ b/datafusion/physical-expr-common/src/datum.rs @@ -160,10 +160,7 @@ pub fn compare_op_for_nested( assert_or_internal_err!(l_len == r_len || is_l_scalar || is_r_scalar, "len mismatch"); - let len = match is_l_scalar { - true => r_len, - false => l_len, - }; + let len = if is_l_scalar { r_len } else { l_len }; // fast path, if compare with one null and operator is not 'distinct', then we can return null array directly if !matches!(op, Operator::IsDistinctFrom | Operator::IsNotDistinctFrom) diff --git a/datafusion/physical-expr-common/src/metrics/mod.rs b/datafusion/physical-expr-common/src/metrics/mod.rs index 516a947685406..05eb8c5d2314e 100644 --- a/datafusion/physical-expr-common/src/metrics/mod.rs +++ b/datafusion/physical-expr-common/src/metrics/mod.rs @@ -112,10 +112,10 @@ impl Display for Metric { let mut is_first = true; for i in iter { - if !is_first { - write!(f, ", ")?; - } else { + if is_first { is_first = false; + } else { + write!(f, ", ")?; } write!(f, "{i}")?; @@ -440,10 +440,10 @@ impl Display for MetricsSet { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let mut is_first = true; for i in self.metrics.iter() { - if !is_first { - write!(f, ", ")?; - } else { + if is_first { is_first = false; + } else { + write!(f, ", ")?; } write!(f, "{i}")?; diff --git a/datafusion/physical-expr-common/src/physical_expr.rs b/datafusion/physical-expr-common/src/physical_expr.rs index 3c35f88eeab45..b0e918c339370 100644 --- a/datafusion/physical-expr-common/src/physical_expr.rs +++ b/datafusion/physical-expr-common/src/physical_expr.rs @@ -139,7 +139,14 @@ pub trait PhysicalExpr: Any + Send + Sync + Display + Debug + DynEq + DynHash { } // Next, prepare the result array for each 'true' row in the selection vector. - let filtered_result = if !selection.has_true() { + let filtered_result = if selection.has_true() { + // If we reach this point, there's no other option than to filter the batch. + // This is a fairly costly operation since it requires creating partial copies + // (worst case of length `row_count - 1`) of all the arrays in the record batch. + // The resulting `filtered_batch` will contain one row per true in `selection`. + let filtered_batch = filter_record_batch(batch, selection)?; + self.evaluate(&filtered_batch)? + } else { // Do not call `evaluate` when the selection is empty. // `evaluate_selection` is used to conditionally evaluate expressions. // When the expression in question is fallible, evaluating it with an empty @@ -148,13 +155,6 @@ pub trait PhysicalExpr: Any + Send + Sync + Display + Debug + DynEq + DynHash { // Instead, create an empty array matching the expected return type. let datatype = self.data_type(batch.schema_ref().as_ref())?; ColumnarValue::Array(new_empty_array(&datatype)) - } else { - // If we reach this point, there's no other option than to filter the batch. - // This is a fairly costly operation since it requires creating partial copies - // (worst case of length `row_count - 1`) of all the arrays in the record batch. - // The resulting `filtered_batch` will contain one row per true in `selection`. - let filtered_batch = filter_record_batch(batch, selection)?; - self.evaluate(&filtered_batch)? }; // Finally, scatter the filtered result array so that the indices match the input rows again. diff --git a/datafusion/physical-expr/src/equivalence/class.rs b/datafusion/physical-expr/src/equivalence/class.rs index 06f384ac2db03..63966b4fd5edc 100644 --- a/datafusion/physical-expr/src/equivalence/class.rs +++ b/datafusion/physical-expr/src/equivalence/class.rs @@ -395,9 +395,8 @@ impl EquivalenceGroup { // If this class becomes trivial, remove it entirely: self.remove_class_at_idx(idx); continue; - } else { - cls.constant = None; } + cls.constant = None; } idx += 1; } diff --git a/datafusion/physical-expr/src/expressions/binary.rs b/datafusion/physical-expr/src/expressions/binary.rs index dfb1d136d0ff0..05355a97cc544 100644 --- a/datafusion/physical-expr/src/expressions/binary.rs +++ b/datafusion/physical-expr/src/expressions/binary.rs @@ -589,20 +589,18 @@ impl PhysicalExpr for BinaryExpr { ); } ColumnarValue::Scalar(scalar) => { - if let ScalarValue::Boolean(v) = scalar { + return if let ScalarValue::Boolean(v) = scalar { // A scalar RHS applies uniformly to all selected rows. if let Some(v) = v { - return Ok(uniform_pre_selection_result( - *v, fill_value, lhs, - )); + Ok(uniform_pre_selection_result(*v, fill_value, lhs)) } else { - return pre_selection_scatter(&mask, None, fill_value); + pre_selection_scatter(&mask, None, fill_value) } } else { - return internal_err!( + internal_err!( "Expected boolean scalar value, found: {right_ret:?}" - ); - } + ) + }; } } } @@ -1000,7 +998,14 @@ impl BinaryExpr { )) })?; - if !node.operands.is_empty() { + if node.operands.is_empty() { + // Legacy format with l/r fields. + let left = + ctx.decode_required_expression(node.l.as_deref(), "BinaryExpr", "left")?; + let right = + ctx.decode_required_expression(node.r.as_deref(), "BinaryExpr", "right")?; + Ok(Arc::new(BinaryExpr::new(left, op, right))) + } else { // New linearized format: reduce the flat operands list back into // a nested binary expression tree. let operands = ctx.decode_children_expressions(&node.operands)?; @@ -1017,13 +1022,6 @@ impl BinaryExpr { Arc::new(BinaryExpr::new(left, op, right)) as Arc }) .expect("Binary expression could not be reduced to a single expression.")) - } else { - // Legacy format with l/r fields. - let left = - ctx.decode_required_expression(node.l.as_deref(), "BinaryExpr", "left")?; - let right = - ctx.decode_required_expression(node.r.as_deref(), "BinaryExpr", "right")?; - Ok(Arc::new(BinaryExpr::new(left, op, right))) } } } @@ -1259,11 +1257,11 @@ fn check_short_circuit(lhs: &ColumnarValue, op: &Operator) -> ShortCircuitStrate // Return Left for: // - AND with false value // - OR with true value - if (is_and && !is_true) || (!is_and && *is_true) { - return ShortCircuitStrategy::ReturnLeft; + return if (is_and && !is_true) || (!is_and && *is_true) { + ShortCircuitStrategy::ReturnLeft } else { - return ShortCircuitStrategy::ReturnRight; - } + ShortCircuitStrategy::ReturnRight + }; } } } diff --git a/datafusion/physical-expr/src/expressions/case.rs b/datafusion/physical-expr/src/expressions/case.rs index e95e9e570cf1b..3e60d97299244 100644 --- a/datafusion/physical-expr/src/expressions/case.rs +++ b/datafusion/physical-expr/src/expressions/case.rs @@ -1304,7 +1304,7 @@ impl PhysicalExpr for CaseExpr { // There is at least one reachable nullable 'then' expression, so the case // expression itself is nullable. // Use `Result::map` to propagate the error from `nullable_then` if there is one. - nullable_then.map(|_| true) + nullable_then.map(|()| true) } else if let Some(e) = &self.body.else_expr { // There are no reachable nullable 'then' expressions, so all we still need to // check is the 'else' expression's nullability. @@ -1361,9 +1361,7 @@ impl PhysicalExpr for CaseExpr { self: Arc, children: Vec>, ) -> Result> { - if children.len() != self.children().len() { - internal_err!("CaseExpr: Wrong number of children") - } else { + if children.len() == self.children().len() { let (expr, when_then_expr, else_expr) = match (self.expr().is_some(), self.body.else_expr.is_some()) { (true, true) => ( @@ -1386,6 +1384,8 @@ impl PhysicalExpr for CaseExpr { when_then_expr.iter().cloned().tuples().collect(), else_expr.cloned(), )?)) + } else { + internal_err!("CaseExpr: Wrong number of children") } } diff --git a/datafusion/physical-expr/src/expressions/case/literal_lookup_table/primitive_lookup_table.rs b/datafusion/physical-expr/src/expressions/case/literal_lookup_table/primitive_lookup_table.rs index 36d282c2a402b..46748b586e28a 100644 --- a/datafusion/physical-expr/src/expressions/case/literal_lookup_table/primitive_lookup_table.rs +++ b/datafusion/physical-expr/src/expressions/case/literal_lookup_table/primitive_lookup_table.rs @@ -46,7 +46,7 @@ where fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("PrimitiveIndexMap") .field("map", &self.map) - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs b/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs index 15544d09e2b56..47089ecd93044 100644 --- a/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs +++ b/datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs @@ -109,7 +109,7 @@ impl std::fmt::Debug for DynamicFilterPhysicalExpr { .field("state_watch", &self.state_watch) .field("data_type", &self.data_type) .field("nullable", &self.nullable) - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion/physical-expr/src/expressions/in_list.rs b/datafusion/physical-expr/src/expressions/in_list.rs index 0a4ad0b804f0c..5bcf640d26d44 100644 --- a/datafusion/physical-expr/src/expressions/in_list.rs +++ b/datafusion/physical-expr/src/expressions/in_list.rs @@ -62,7 +62,7 @@ impl Debug for InListExpr { .field("expr", &self.expr) .field("list", &self.list) .field("negated", &self.negated) - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion/physical-expr/src/higher_order_function.rs b/datafusion/physical-expr/src/higher_order_function.rs index e28b38bd7c8c1..e926f4501c825 100644 --- a/datafusion/physical-expr/src/higher_order_function.rs +++ b/datafusion/physical-expr/src/higher_order_function.rs @@ -115,7 +115,7 @@ impl Debug for HigherOrderFunctionExpr { .field("args", &self.args) .field("lambda_positions", &lambda_positions) .field("return_field", &self.return_field) - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion/physical-expr/src/scalar_function.rs b/datafusion/physical-expr/src/scalar_function.rs index 6a5ab219aa8dd..757a7ac437a1d 100644 --- a/datafusion/physical-expr/src/scalar_function.rs +++ b/datafusion/physical-expr/src/scalar_function.rs @@ -64,7 +64,7 @@ impl Debug for ScalarFunctionExpr { .field("name", &self.name) .field("args", &self.args) .field("return_field", &self.return_field) - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion/physical-expr/src/utils/guarantee.rs b/datafusion/physical-expr/src/utils/guarantee.rs index 8b870c573d393..78dc16ccfa68d 100644 --- a/datafusion/physical-expr/src/utils/guarantee.rs +++ b/datafusion/physical-expr/src/utils/guarantee.rs @@ -349,11 +349,11 @@ impl<'a> GuaranteeBuilder<'a> { // e.g. `a IN (1,2,3) AND a IN (2,3,4)` is `a IN (2,3)` // otherwise, we invalidate the guarantee // e.g. `a IN (1,2,3) AND a IN (4,5,6)` is `a IN ()`, which is invalid - if !intersection.is_empty() { - existing.literals = intersection.into_iter().cloned().collect(); - } else { + if intersection.is_empty() { // at least one was not, so invalidate the guarantee *entry = None; + } else { + existing.literals = intersection.into_iter().cloned().collect(); } } } diff --git a/datafusion/physical-optimizer/src/ensure_requirements/enforce_distribution.rs b/datafusion/physical-optimizer/src/ensure_requirements/enforce_distribution.rs index 7b4c748030654..8eff70b5c7be8 100644 --- a/datafusion/physical-optimizer/src/ensure_requirements/enforce_distribution.rs +++ b/datafusion/physical-optimizer/src/ensure_requirements/enforce_distribution.rs @@ -230,17 +230,15 @@ pub fn adjust_input_keys_ordering( ) .map(Transformed::yes); } else if let Some(aggregate_exec) = plan.downcast_ref::() { - if !requirements.data.is_empty() { - if aggregate_exec.mode() == &AggregateMode::FinalPartitioned { - return reorder_aggregate_keys(requirements, aggregate_exec) - .map(Transformed::yes); - } else { - requirements.data.clear(); - } - } else { + if requirements.data.is_empty() { // Keep everything unchanged return Ok(Transformed::no(requirements)); } + if aggregate_exec.mode() == &AggregateMode::FinalPartitioned { + return reorder_aggregate_keys(requirements, aggregate_exec) + .map(Transformed::yes); + } + requirements.data.clear(); } else if let Some(proj) = plan.downcast_ref::() { let expr = proj.expr(); // For Projection, we need to transform the requirements to the columns before the Projection @@ -1165,10 +1163,10 @@ fn enforce_distribution_relationships( let (i, p, _) = native_children[0]; Some((*i, p.clone())) } else { - let pool = if !native_children.is_empty() { - native_children - } else { + let pool = if native_children.is_empty() { satisfied_children.iter().collect() + } else { + native_children }; let candidates: Vec<_> = pool .into_iter() diff --git a/datafusion/physical-optimizer/src/ensure_requirements/enforce_sorting/mod.rs b/datafusion/physical-optimizer/src/ensure_requirements/enforce_sorting/mod.rs index e9467d508aa10..fdc6c04d7b1b7 100644 --- a/datafusion/physical-optimizer/src/ensure_requirements/enforce_sorting/mod.rs +++ b/datafusion/physical-optimizer/src/ensure_requirements/enforce_sorting/mod.rs @@ -411,10 +411,10 @@ pub fn ensure_sorting( return Ok(Transformed::no(requirements)); } let maybe_requirements = analyze_immediate_sort_removal(requirements)?; - requirements = if !maybe_requirements.transformed { - maybe_requirements.data - } else { + requirements = if maybe_requirements.transformed { return Ok(maybe_requirements); + } else { + maybe_requirements.data }; let plan = &requirements.plan; diff --git a/datafusion/physical-optimizer/src/ensure_requirements/enforce_sorting/sort_pushdown.rs b/datafusion/physical-optimizer/src/ensure_requirements/enforce_sorting/sort_pushdown.rs index 1f7acd8f867a6..ef0d780ceed53 100644 --- a/datafusion/physical-optimizer/src/ensure_requirements/enforce_sorting/sort_pushdown.rs +++ b/datafusion/physical-optimizer/src/ensure_requirements/enforce_sorting/sort_pushdown.rs @@ -125,14 +125,14 @@ fn stronger_distribution(a: &Distribution, b: &Distribution) -> Distribution { (Distribution::SinglePartition, _) | (_, Distribution::SinglePartition) => { Distribution::SinglePartition } - (Distribution::HashPartitioned(exprs), _) - | (Distribution::KeyPartitioned(exprs), _) => { - Distribution::KeyPartitioned(exprs.clone()) - } - (_, Distribution::HashPartitioned(exprs)) - | (_, Distribution::KeyPartitioned(exprs)) => { - Distribution::KeyPartitioned(exprs.clone()) - } + ( + Distribution::HashPartitioned(exprs) | Distribution::KeyPartitioned(exprs), + _, + ) => Distribution::KeyPartitioned(exprs.clone()), + ( + _, + Distribution::HashPartitioned(exprs) | Distribution::KeyPartitioned(exprs), + ) => Distribution::KeyPartitioned(exprs.clone()), _ => Distribution::UnspecifiedDistribution, } } @@ -220,28 +220,27 @@ fn pushdown_sorts_helper( distribution_requirement: Distribution::UnspecifiedDistribution, }; return Ok(Transformed::yes(sort_push_down)); - } else { - // Sort was unnecessary, just propagate the stricter fetch and - // ordering requirements. Reset distribution to Unspecified - // because the sort we're removing may have been below a - // partition-merging node (like SortPreservingMergeExec) that - // already satisfies SinglePartition. - sort_push_down.data.fetch = min_fetch(sort_fetch, parent_fetch); - sort_push_down.data.distribution_requirement = - Distribution::UnspecifiedDistribution; - let current_is_stricter = eqp.requirements_compatible( - sort_ordering.clone().into(), - parent_requirement.first().clone(), - ); - sort_push_down.data.ordering_requirement = if current_is_stricter { - Some(OrderingRequirements::from(sort_ordering)) - } else { - Some(parent_requirement) - }; - // Recursive call to helper, so it doesn't transform_down and miss - // the new node (previous child of sort): - return pushdown_sorts_helper(sort_push_down); } + // Sort was unnecessary, just propagate the stricter fetch and + // ordering requirements. Reset distribution to Unspecified + // because the sort we're removing may have been below a + // partition-merging node (like SortPreservingMergeExec) that + // already satisfies SinglePartition. + sort_push_down.data.fetch = min_fetch(sort_fetch, parent_fetch); + sort_push_down.data.distribution_requirement = + Distribution::UnspecifiedDistribution; + let current_is_stricter = eqp.requirements_compatible( + sort_ordering.clone().into(), + parent_requirement.first().clone(), + ); + sort_push_down.data.ordering_requirement = if current_is_stricter { + Some(OrderingRequirements::from(sort_ordering)) + } else { + Some(parent_requirement) + }; + // Recursive call to helper, so it doesn't transform_down and miss + // the new node (previous child of sort): + return pushdown_sorts_helper(sort_push_down); } sort_push_down.plan = plan; diff --git a/datafusion/physical-optimizer/src/utils.rs b/datafusion/physical-optimizer/src/utils.rs index 04229e1cc2737..340896b0533a4 100644 --- a/datafusion/physical-optimizer/src/utils.rs +++ b/datafusion/physical-optimizer/src/utils.rs @@ -116,14 +116,14 @@ pub fn add_sort_above_with_check( sort_requirements: LexRequirement, fetch: Option, ) -> Result> { - if !node + if node .plan .equivalence_properties() .ordering_satisfy_requirement(sort_requirements.clone())? { - Ok(add_sort_above(node, sort_requirements, fetch)) - } else { Ok(node) + } else { + Ok(add_sort_above(node, sort_requirements, fetch)) } } diff --git a/datafusion/physical-plan/src/aggregates/aggregate_stream.rs b/datafusion/physical-plan/src/aggregates/aggregate_stream.rs index a3fc731cb8ed3..d0ea6c3dc9bf5 100644 --- a/datafusion/physical-plan/src/aggregates/aggregate_stream.rs +++ b/datafusion/physical-plan/src/aggregates/aggregate_stream.rs @@ -396,7 +396,7 @@ impl AggregateStream { match result .and_then(|allocated| this.reservation.try_grow(allocated)) { - Ok(_) => continue, + Ok(()) => continue, Err(e) => Err(e), } } diff --git a/datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs b/datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs index 257f13cbfaffb..55e03dd4e3c90 100644 --- a/datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs +++ b/datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs @@ -970,10 +970,8 @@ fn group_column_supported_type(data_type: &DataType) -> bool { // other unit combinations, so accepting them here would cause a // schema to be routed into GroupValuesColumn and then fail at // intern. Keep these two arms in lockstep with the dispatcher. - | DataType::Time32(TimeUnit::Second) - | DataType::Time32(TimeUnit::Millisecond) - | DataType::Time64(TimeUnit::Microsecond) - | DataType::Time64(TimeUnit::Nanosecond) + | DataType::Time32(TimeUnit::Second | TimeUnit::Millisecond) + | DataType::Time64(TimeUnit::Microsecond | TimeUnit::Nanosecond) | DataType::Timestamp(_, _) | DataType::Duration(_) | DataType::Interval(_) @@ -1180,10 +1178,10 @@ impl GroupValues for GroupValuesColumn { // `try_new` and the reset points in `emit` / `clear_shrink` keep // `self.group_values` populated with one builder per schema field, // so no lazy initialization is needed here. - if !STREAMING { - self.vectorized_intern(cols, groups) - } else { + if STREAMING { self.scalarized_intern(cols, groups) + } else { + self.vectorized_intern(cols, groups) } } diff --git a/datafusion/physical-plan/src/aggregates/grouped_hash_stream.rs b/datafusion/physical-plan/src/aggregates/grouped_hash_stream.rs index 3f6f3f8ce815b..713c5bef82a04 100644 --- a/datafusion/physical-plan/src/aggregates/grouped_hash_stream.rs +++ b/datafusion/physical-plan/src/aggregates/grouped_hash_stream.rs @@ -999,7 +999,7 @@ impl GroupedHashAggregateStream { let oom = match self.update_memory_reservation() { Err(e @ DataFusionError::ResourcesExhausted(_)) => e, Err(e) => return Err(e), - Ok(_) => return Ok(None), + Ok(()) => return Ok(None), }; match self.oom_mode { diff --git a/datafusion/physical-plan/src/aggregates/mod.rs b/datafusion/physical-plan/src/aggregates/mod.rs index f00f1a160e27a..73543204f5aab 100644 --- a/datafusion/physical-plan/src/aggregates/mod.rs +++ b/datafusion/physical-plan/src/aggregates/mod.rs @@ -1722,10 +1722,10 @@ impl AggregateExec { child_statistics: &Statistics, partition: Option, ) -> Precision { - let ndv = if !self.group_by.expr.is_empty() { - self.compute_group_ndv(child_statistics) - } else { + let ndv = if self.group_by.expr.is_empty() { None + } else { + self.compute_group_ndv(child_statistics) }; let limit = self.limit_options.as_ref().map(|lo| lo.limit); @@ -1921,10 +1921,10 @@ impl DisplayAs for AggregateExec { let format_expr_with_alias = |(e, alias): &(Arc, String)| -> String { let e = e.to_string(); - if &e != alias { - format!("{e} as {alias}") - } else { + if &e == alias { e + } else { + format!("{e} as {alias}") } }; @@ -1979,10 +1979,10 @@ impl DisplayAs for AggregateExec { let format_expr_with_alias = |(e, alias): &(Arc, String)| -> String { let expr_sql = fmt_sql(e.as_ref()).to_string(); - if &expr_sql != alias { - format!("{expr_sql} as {alias}") - } else { + if &expr_sql == alias { expr_sql + } else { + format!("{expr_sql} as {alias}") } }; diff --git a/datafusion/physical-plan/src/buffer.rs b/datafusion/physical-plan/src/buffer.rs index df87fee7da087..7f81b234997b8 100644 --- a/datafusion/physical-plan/src/buffer.rs +++ b/datafusion/physical-plan/src/buffer.rs @@ -429,7 +429,7 @@ impl MemoryBufferedStream { // in order to consider aborting the stream let item_or_err = tokio::select! { biased; - _ = batch_tx.closed() => break, + () = batch_tx.closed() => break, // Catch a panic in the input poll so it surfaces as a stream error // instead of dropping `batch_tx` and looking like a clean EOF. polled = AssertUnwindSafe(input.next()).catch_unwind() => { @@ -749,12 +749,13 @@ mod tests { async fn finished( buffered: &mut MemoryBufferedStream, ) -> Result<(), Box> { - match timeout(Duration::from_millis(1), buffered.next()) + if timeout(Duration::from_millis(1), buffered.next()) .await? .is_none() { - true => Ok(()), - false => internal_err!("Stream should have finished")?, + Ok(()) + } else { + internal_err!("Stream should have finished")? } } diff --git a/datafusion/physical-plan/src/column_rewriter.rs b/datafusion/physical-plan/src/column_rewriter.rs index e03f5ab5d3d9d..1caf4877e53e2 100644 --- a/datafusion/physical-plan/src/column_rewriter.rs +++ b/datafusion/physical-plan/src/column_rewriter.rs @@ -51,20 +51,20 @@ impl TreeNodeRewriter for PhysicalColumnRewriter<'_> { node: Self::Node, ) -> datafusion_common::Result> { if let Some(column) = node.downcast_ref::() { - if let Some(new_column) = self.column_map.get(column) { + return if let Some(new_column) = self.column_map.get(column) { // jump to prevent rewriting the new sub-expression again - return Ok(Transformed::new( + Ok(Transformed::new( Arc::clone(new_column), true, TreeNodeRecursion::Jump, - )); + )) } else { // Column not found in mapping - return Err(DataFusionError::Internal(format!( + Err(DataFusionError::Internal(format!( "Column {column:?} not found in column mapping {:?}", self.column_map - ))); - } + ))) + }; } Ok(Transformed::no(node)) } diff --git a/datafusion/physical-plan/src/display.rs b/datafusion/physical-plan/src/display.rs index 3a84886157272..a7ca3691c177f 100644 --- a/datafusion/physical-plan/src/display.rs +++ b/datafusion/physical-plan/src/display.rs @@ -681,7 +681,7 @@ impl ExecutionPlanVisitor for GraphvizVisitor<'_, '_> { let label = { format!("{}", Wrapper(plan, self.t)) }; let metrics = match self.show_metrics { - ShowMetrics::None => "".to_string(), + ShowMetrics::None => String::new(), ShowMetrics::Aggregated => { if let Some(metrics) = plan.metrics() { let mut metrics = metrics @@ -722,7 +722,7 @@ impl ExecutionPlanVisitor for GraphvizVisitor<'_, '_> { .map_err(|_e| fmt::Error)?; format!("statistics=[{stats}]") } else { - "".to_string() + String::new() }; let delimiter = if !metrics.is_empty() && !statistics.is_empty() { diff --git a/datafusion/physical-plan/src/filter.rs b/datafusion/physical-plan/src/filter.rs index 12771eec78470..45c6dc72d374a 100644 --- a/datafusion/physical-plan/src/filter.rs +++ b/datafusion/physical-plan/src/filter.rs @@ -521,11 +521,11 @@ impl DisplayAs for FilterExec { .join(", ") ) } else { - "".to_string() + String::new() }; let fetch = self .fetch - .map_or_else(|| "".to_string(), |f| format!(", fetch={f}")); + .map_or_else(String::new, |f| format!(", fetch={f}")); write!( f, "FilterExec: {}{}{}", diff --git a/datafusion/physical-plan/src/joins/asof_join.rs b/datafusion/physical-plan/src/joins/asof_join.rs index 7cc19e1153114..f452263717ba4 100644 --- a/datafusion/physical-plan/src/joins/asof_join.rs +++ b/datafusion/physical-plan/src/joins/asof_join.rs @@ -581,7 +581,7 @@ async fn collect_right_input( let batches = input .try_fold(Vec::new(), |mut batches, batch| { let batch_size = memory_counter.count_batch(&batch); - futures::future::ready(reservation.try_grow(batch_size).map(|_| { + futures::future::ready(reservation.try_grow(batch_size).map(|()| { metrics.build_mem_used.add(batch_size); batches.push(batch); batches diff --git a/datafusion/physical-plan/src/joins/hash_join/exec.rs b/datafusion/physical-plan/src/joins/hash_join/exec.rs index 1539fd4e73e98..66dc35f96f208 100644 --- a/datafusion/physical-plan/src/joins/hash_join/exec.rs +++ b/datafusion/physical-plan/src/joins/hash_join/exec.rs @@ -823,7 +823,7 @@ impl fmt::Debug for HashJoinExec { .field("null_equality", &self.null_equality) .field("cache", &self.cache) // Explicitly exclude dynamic_filter to avoid runtime state differences in tests - .finish() + .finish_non_exhaustive() } } @@ -1231,10 +1231,10 @@ impl DisplayAs for HashJoinExec { fn fmt_as(&self, t: DisplayFormatType, f: &mut fmt::Formatter) -> fmt::Result { match t { DisplayFormatType::Default | DisplayFormatType::Verbose => { - let display_filter = self.filter.as_ref().map_or_else( - || "".to_string(), - |f| format!(", filter={}", f.expression()), - ); + let display_filter = self + .filter + .as_ref() + .map_or_else(String::new, |f| format!(", filter={}", f.expression())); let display_projections = if self.contains_projection() { format!( ", projection=[{}]", @@ -1251,7 +1251,7 @@ impl DisplayAs for HashJoinExec { .join(", ") ) } else { - "".to_string() + String::new() }; let display_null_equality = if self.null_equality() == NullEquality::NullEqualsNull { diff --git a/datafusion/physical-plan/src/joins/nested_loop_join.rs b/datafusion/physical-plan/src/joins/nested_loop_join.rs index f63418f63e5f0..b66cacd4b8f71 100644 --- a/datafusion/physical-plan/src/joins/nested_loop_join.rs +++ b/datafusion/physical-plan/src/joins/nested_loop_join.rs @@ -489,10 +489,10 @@ impl DisplayAs for NestedLoopJoinExec { fn fmt_as(&self, t: DisplayFormatType, f: &mut Formatter) -> std::fmt::Result { match t { DisplayFormatType::Default | DisplayFormatType::Verbose => { - let display_filter = self.filter.as_ref().map_or_else( - || "".to_string(), - |f| format!(", filter={}", f.expression()), - ); + let display_filter = self + .filter + .as_ref() + .map_or_else(String::new, |f| format!(", filter={}", f.expression())); let display_projections = if self.contains_projection() { format!( ", projection=[{}]", @@ -509,7 +509,7 @@ impl DisplayAs for NestedLoopJoinExec { .join(", ") ) } else { - "".to_string() + String::new() }; write!( f, @@ -518,10 +518,10 @@ impl DisplayAs for NestedLoopJoinExec { ) } DisplayFormatType::TreeRender => { - if *self.join_type() != JoinType::Inner { - writeln!(f, "join_type={:?}", self.join_type) - } else { + if *self.join_type() == JoinType::Inner { Ok(()) + } else { + writeln!(f, "join_type={:?}", self.join_type) } } } @@ -2721,10 +2721,7 @@ impl NestedLoopJoinStream { return Ok(None); } - if !cur_right_bitmap.has_true() { - // If none of the pairs has passed the join predicate/filter - Ok(None) - } else { + if cur_right_bitmap.has_true() { // Use the optimized approach similar to build_intermediate_batch_for_single_left_row let join_batch = build_row_join_batch( &self.output_schema, @@ -2736,6 +2733,9 @@ impl NestedLoopJoinStream { JoinSide::Left, )?; Ok(join_batch) + } else { + // If none of the pairs has passed the join predicate/filter + Ok(None) } } diff --git a/datafusion/physical-plan/src/joins/sort_merge_join/exec.rs b/datafusion/physical-plan/src/joins/sort_merge_join/exec.rs index 911eca0a97928..55d02bccef1c9 100644 --- a/datafusion/physical-plan/src/joins/sort_merge_join/exec.rs +++ b/datafusion/physical-plan/src/joins/sort_merge_join/exec.rs @@ -433,10 +433,10 @@ impl DisplayAs for SortMergeJoinExec { Self::static_name(), self.join_type, on, - self.filter.as_ref().map_or_else( - || "".to_string(), - |f| format!(", filter={}", f.expression()) - ), + self.filter.as_ref().map_or_else(String::new, |f| format!( + ", filter={}", + f.expression() + )), display_null_equality, display_projections, ) diff --git a/datafusion/physical-plan/src/joins/sort_merge_join/materializing_stream.rs b/datafusion/physical-plan/src/joins/sort_merge_join/materializing_stream.rs index 96b903f63bc1e..4250fd3b6edd8 100644 --- a/datafusion/physical-plan/src/joins/sort_merge_join/materializing_stream.rs +++ b/datafusion/physical-plan/src/joins/sort_merge_join/materializing_stream.rs @@ -1075,7 +1075,7 @@ impl MaterializingSortMergeJoinStream { fn allocate_reservation(&mut self, mut buffered_batch: BufferedBatch) -> Result<()> { match self.reservation.try_grow(buffered_batch.size_estimation) { - Ok(_) => { + Ok(()) => { buffered_batch.reserved_amount = buffered_batch.size_estimation; self.join_metrics .peak_mem_used() @@ -1466,10 +1466,10 @@ impl MaterializingSortMergeJoinStream { let right_columns = create_unmatched_columns(&self.buffered_schema, left_indices.len()); - let columns = if self.join_type != JoinType::Right { - [left_columns, right_columns].concat() - } else { + let columns = if self.join_type == JoinType::Right { [right_columns, left_columns].concat() + } else { + [left_columns, right_columns].concat() }; let batch = RecordBatch::try_new(Arc::clone(&self.schema), columns)?; @@ -1553,83 +1553,81 @@ impl MaterializingSortMergeJoinStream { get_filter_columns(self.filter.as_ref(), &left_columns, &right_columns) }; - let columns = if self.join_type != JoinType::Right { - [left_columns, right_columns].concat() - } else { + let columns = if self.join_type == JoinType::Right { [right_columns, left_columns].concat() + } else { + [left_columns, right_columns].concat() }; let output_batch = RecordBatch::try_new(Arc::clone(&self.schema), columns)?; - if !filter_columns.is_empty() { - if let Some(f) = &self.filter { - let filter_batch = - RecordBatch::try_new(Arc::clone(f.schema()), filter_columns)?; - let filter_result = f - .expression() - .evaluate(&filter_batch)? - .into_array(filter_batch.num_rows())?; - - let filter_result_mask = - datafusion_common::cast::as_boolean_array(&filter_result)?; - - // Convert NULL filter results to false — NULL means "not satisfied" - // per SQL semantics, same as Left/Right outer joins. - let mask = if filter_result_mask.null_count() > 0 { - compute::prep_null_mask_filter(filter_result_mask) - } else { - filter_result_mask.clone() - }; + if filter_columns.is_empty() { + self.joined_record_batches + .push_batch_without_metadata(output_batch); + } else if let Some(f) = &self.filter { + let filter_batch = + RecordBatch::try_new(Arc::clone(f.schema()), filter_columns)?; + let filter_result = f + .expression() + .evaluate(&filter_batch)? + .into_array(filter_batch.num_rows())?; + + let filter_result_mask = + datafusion_common::cast::as_boolean_array(&filter_result)?; + + // Convert NULL filter results to false — NULL means "not satisfied" + // per SQL semantics, same as Left/Right outer joins. + let mask = if filter_result_mask.null_count() > 0 { + compute::prep_null_mask_filter(filter_result_mask) + } else { + filter_result_mask.clone() + }; - if self.deferred_filtering { - self.joined_record_batches.push_batch_with_filter_metadata( - output_batch, - &combined_left_indices, - &mask, - self.streamed_batch_counter, - self.join_type, - ); - } else { - let filtered_batch = filter_record_batch(&output_batch, &mask)?; - self.joined_record_batches - .push_batch_without_metadata(filtered_batch); - } + if self.deferred_filtering { + self.joined_record_batches.push_batch_with_filter_metadata( + output_batch, + &combined_left_indices, + &mask, + self.streamed_batch_counter, + self.join_type, + ); + } else { + let filtered_batch = filter_record_batch(&output_batch, &mask)?; + self.joined_record_batches + .push_batch_without_metadata(filtered_batch); + } - // Track which buffered rows had all filter matches fail, - // so full join can emit them as null-joined later. - if self.join_type == JoinType::Full { - let mut offset = 0usize; - for (batch_idx, _left, right) in matched_chunks { - let chunk_len = right.len(); - let buffered_batch = &mut self.buffered_data.batches[*batch_idx]; - - for i in 0..chunk_len { - if right.is_null(i) { - continue; + // Track which buffered rows had all filter matches fail, + // so full join can emit them as null-joined later. + if self.join_type == JoinType::Full { + let mut offset = 0usize; + for (batch_idx, _left, right) in matched_chunks { + let chunk_len = right.len(); + let buffered_batch = &mut self.buffered_data.batches[*batch_idx]; + + for i in 0..chunk_len { + if right.is_null(i) { + continue; + } + let idx = right.value(i) as usize; + match buffered_batch.join_filter_status[idx] { + FilterState::SomePassed => {} + _ if mask.value(offset + i) => { + buffered_batch.join_filter_status[idx] = + FilterState::SomePassed; } - let idx = right.value(i) as usize; - match buffered_batch.join_filter_status[idx] { - FilterState::SomePassed => {} - _ if mask.value(offset + i) => { - buffered_batch.join_filter_status[idx] = - FilterState::SomePassed; - } - _ => { - buffered_batch.join_filter_status[idx] = - FilterState::AllFailed; - } + _ => { + buffered_batch.join_filter_status[idx] = + FilterState::AllFailed; } } - offset += chunk_len; } - debug_assert_eq!( - offset, total_matched_rows, - "offset must advance through every chunk exactly once" - ); + offset += chunk_len; } + debug_assert_eq!( + offset, total_matched_rows, + "offset must advance through every chunk exactly once" + ); } - } else { - self.joined_record_batches - .push_batch_without_metadata(output_batch); } Ok(()) diff --git a/datafusion/physical-plan/src/joins/symmetric_hash_join.rs b/datafusion/physical-plan/src/joins/symmetric_hash_join.rs index aec693e839718..fbfb88a3162b1 100644 --- a/datafusion/physical-plan/src/joins/symmetric_hash_join.rs +++ b/datafusion/physical-plan/src/joins/symmetric_hash_join.rs @@ -368,10 +368,10 @@ impl DisplayAs for SymmetricHashJoinExec { fn fmt_as(&self, t: DisplayFormatType, f: &mut fmt::Formatter) -> fmt::Result { match t { DisplayFormatType::Default | DisplayFormatType::Verbose => { - let display_filter = self.filter.as_ref().map_or_else( - || "".to_string(), - |f| format!(", filter={}", f.expression()), - ); + let display_filter = self + .filter + .as_ref() + .map_or_else(String::new, |f| format!(", filter={}", f.expression())); let on = self .on .iter() diff --git a/datafusion/physical-plan/src/limit.rs b/datafusion/physical-plan/src/limit.rs index 73d98d8105215..6e029240cd9eb 100644 --- a/datafusion/physical-plan/src/limit.rs +++ b/datafusion/physical-plan/src/limit.rs @@ -672,9 +672,8 @@ impl LimitStream { Poll::Ready(Some(Ok(batch))) => { if batch.num_rows() > 0 { break poll; - } else { - // Continue to poll input stream } + // Continue to poll input stream } Poll::Ready(Some(Err(_e))) => break poll, Poll::Ready(None) => break poll, diff --git a/datafusion/physical-plan/src/memory.rs b/datafusion/physical-plan/src/memory.rs index 0b6bdf4490d8b..72a676a2180c4 100644 --- a/datafusion/physical-plan/src/memory.rs +++ b/datafusion/physical-plan/src/memory.rs @@ -270,7 +270,7 @@ impl fmt::Debug for LazyMemoryExec { f.debug_struct("LazyMemoryExec") .field("schema", &self.schema) .field("batch_generators", &self.batch_generators) - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion/physical-plan/src/operator_statistics/mod.rs b/datafusion/physical-plan/src/operator_statistics/mod.rs index b19f4e5fd4693..a5d649ffe6f86 100644 --- a/datafusion/physical-plan/src/operator_statistics/mod.rs +++ b/datafusion/physical-plan/src/operator_statistics/mod.rs @@ -1031,9 +1031,11 @@ impl StatisticsProvider for UnionStatisticsProvider { (Precision::Exact(a), Precision::Exact(b)) => { Precision::Exact(a.saturating_add(b)) } - (Precision::Inexact(a), Precision::Exact(b)) - | (Precision::Exact(a), Precision::Inexact(b)) - | (Precision::Inexact(a), Precision::Inexact(b)) => { + ( + Precision::Inexact(a), + Precision::Exact(b) | Precision::Inexact(b), + ) + | (Precision::Exact(a), Precision::Inexact(b)) => { Precision::Inexact(a.saturating_add(b)) } }) diff --git a/datafusion/physical-plan/src/projection.rs b/datafusion/physical-plan/src/projection.rs index c4096457c168a..1ca1d1a541dc5 100644 --- a/datafusion/physical-plan/src/projection.rs +++ b/datafusion/physical-plan/src/projection.rs @@ -309,10 +309,10 @@ impl DisplayAs for ProjectionExec { .iter() .map(|proj_expr| { let e = proj_expr.expr.to_string(); - if e != proj_expr.alias { - format!("{e} as {}", proj_expr.alias) - } else { + if e == proj_expr.alias { e + } else { + format!("{e} as {}", proj_expr.alias) } }) .collect(); diff --git a/datafusion/physical-plan/src/render_tree.rs b/datafusion/physical-plan/src/render_tree.rs index 40e2763698093..2fb220df8aa33 100644 --- a/datafusion/physical-plan/src/render_tree.rs +++ b/datafusion/physical-plan/src/render_tree.rs @@ -204,7 +204,7 @@ fn create_tree_recursive( if let Some((key, value)) = line.split_once('=') { extra_info.insert(key.to_string(), value.to_string()); } else { - extra_info.insert(line.to_string(), "".to_string()); + extra_info.insert(line.to_string(), String::new()); } } diff --git a/datafusion/physical-plan/src/repartition/mod.rs b/datafusion/physical-plan/src/repartition/mod.rs index 7822c6facd7c9..baedf9701f8a5 100644 --- a/datafusion/physical-plan/src/repartition/mod.rs +++ b/datafusion/physical-plan/src/repartition/mod.rs @@ -228,7 +228,7 @@ impl OutputChannel { // across an await point. let (payload, is_memory_batch) = { match self.reservation.try_grow(size) { - Ok(_) => (Ok(RepartitionBatch::Memory(batch)), true), + Ok(()) => (Ok(RepartitionBatch::Memory(batch)), true), Err(_) => match self.spill_writer.push_batch(&batch) { Ok(()) => (Ok(RepartitionBatch::Spilled), false), Err(err) => (Err(err), false), diff --git a/datafusion/physical-plan/src/sorts/cursor.rs b/datafusion/physical-plan/src/sorts/cursor.rs index 003de2375ad3f..4357dec657384 100644 --- a/datafusion/physical-plan/src/sorts/cursor.rs +++ b/datafusion/physical-plan/src/sorts/cursor.rs @@ -509,9 +509,10 @@ impl ArrayValues { reservation: MemoryReservation, ) -> Self { assert!(array.len() > 0, "Empty array passed to FieldCursor"); - let null_threshold = match options.nulls_first { - true => array.null_count(), - false => array.len() - array.null_count(), + let null_threshold = if options.nulls_first { + array.null_count() + } else { + array.len() - array.null_count() }; Self { @@ -562,18 +563,27 @@ impl CursorValues for ArrayValues { fn compare(l: &Self, l_idx: usize, r: &Self, r_idx: usize) -> Ordering { match (l.is_null(l_idx), r.is_null(r_idx)) { (true, true) => Ordering::Equal, - (true, false) => match l.options.nulls_first { - true => Ordering::Less, - false => Ordering::Greater, - }, - (false, true) => match l.options.nulls_first { - true => Ordering::Greater, - false => Ordering::Less, - }, - (false, false) => match l.options.descending { - true => T::compare(&r.values, r_idx, &l.values, l_idx), - false => T::compare(&l.values, l_idx, &r.values, r_idx), - }, + (true, false) => { + if l.options.nulls_first { + Ordering::Less + } else { + Ordering::Greater + } + } + (false, true) => { + if l.options.nulls_first { + Ordering::Greater + } else { + Ordering::Less + } + } + (false, false) => { + if l.options.descending { + T::compare(&r.values, r_idx, &l.values, l_idx) + } else { + T::compare(&l.values, l_idx, &r.values, r_idx) + } + } } } @@ -619,9 +629,10 @@ mod tests { values: ScalarBuffer, null_count: usize, ) -> Cursor>> { - let null_threshold = match options.nulls_first { - true => null_count, - false => values.len() - null_count, + let null_threshold = if options.nulls_first { + null_count + } else { + values.len() - null_count }; let memory_pool: Arc = Arc::new(GreedyMemoryPool::new(10000)); diff --git a/datafusion/physical-plan/src/sorts/multi_level_merge.rs b/datafusion/physical-plan/src/sorts/multi_level_merge.rs index 3ec52cc70c0a9..45132f72d4e4c 100644 --- a/datafusion/physical-plan/src/sorts/multi_level_merge.rs +++ b/datafusion/physical-plan/src/sorts/multi_level_merge.rs @@ -443,18 +443,18 @@ impl MultiLevelMergeBuilder { .with_round_robin_tie_breaker(self.enable_round_robin_tie_breaker) .with_streams(streams); - if !all_in_memory { - // Don't track memory used by this stream as we reserve that memory by worst case sceneries - // (reserving memory for the biggest batch in each stream) - // TODO - avoid this hack as this can be broken easily when `SortPreservingMergeStream` - // changes the implementation to use more/less memory - builder = builder.with_bypass_mempool(); - } else { + if all_in_memory { // If we are only merging in-memory streams, we need to use the memory reservation // because we don't know the maximum size of the batches in the streams. // Use take() to transfer any pre-reserved bytes so the merge can use them // as its initial budget without additional pool allocation. builder = builder.with_reservation(self.reservation.take()); + } else { + // Don't track memory used by this stream as we reserve that memory by worst case sceneries + // (reserving memory for the biggest batch in each stream) + // TODO - avoid this hack as this can be broken easily when `SortPreservingMergeStream` + // changes the implementation to use more/less memory + builder = builder.with_bypass_mempool(); } builder.build() @@ -500,7 +500,7 @@ impl MultiLevelMergeBuilder { // this is not and there should be some upper limit to memory // reservation so we won't starve the system. match try_grow_reservation_to_at_least(reservation, total_needed) { - Ok(_) => { + Ok(()) => { number_of_spills_to_read_for_current_phase += 1; } // If we can't grow the reservation, we need to stop diff --git a/datafusion/physical-plan/src/sorts/sort.rs b/datafusion/physical-plan/src/sorts/sort.rs index 04629f4608f64..5b8cf0632fed1 100644 --- a/datafusion/physical-plan/src/sorts/sort.rs +++ b/datafusion/physical-plan/src/sorts/sort.rs @@ -802,7 +802,7 @@ impl ExternalSorter { let size = get_reserved_bytes_for_record_batch(input)?; match self.reservation.try_grow(size) { - Ok(_) => Ok(()), + Ok(()) => Ok(()), Err(e) => { if self.in_mem_batches.is_empty() { return Err(Self::err_with_oom_context(e)); @@ -1229,7 +1229,9 @@ impl DisplayAs for SortExec { { write!(f, ", filter=[{current}]")?; } - if !self.common_sort_prefix.is_empty() { + if self.common_sort_prefix.is_empty() { + Ok(()) + } else { write!(f, ", sort_prefix=[")?; let mut first = true; for sort_expr in &self.common_sort_prefix { @@ -1241,8 +1243,6 @@ impl DisplayAs for SortExec { write!(f, "{sort_expr}")?; } write!(f, "]") - } else { - Ok(()) } } None => write!( @@ -1354,15 +1354,16 @@ impl ExecutionPlan for SortExec { self: Arc, children: Vec>, ) -> Result> { - match has_same_children_properties(self.as_ref(), &children)? { - true => self.replace_children( + if has_same_children_properties(self.as_ref(), &children)? { + self.replace_children( children, ReplaceChildrenOptions::new(ChildrenPropertiesMode::Keep), - ), - false => self.replace_children( + ) + } else { + self.replace_children( children, ReplaceChildrenOptions::new(ChildrenPropertiesMode::Recompute), - ), + ) } } diff --git a/datafusion/physical-plan/src/sorts/stream.rs b/datafusion/physical-plan/src/sorts/stream.rs index bb9c00949369e..0233759f8c057 100644 --- a/datafusion/physical-plan/src/sorts/stream.rs +++ b/datafusion/physical-plan/src/sorts/stream.rs @@ -79,7 +79,7 @@ impl FusedStreams { // Skip empty batches Poll::Ready(Some(Ok(b))) if b.num_rows() == 0 => {} Poll::Ready(Some(Ok(_))) => return poll_result, - Poll::Ready(None) | Poll::Ready(Some(Err(_))) => { + Poll::Ready(None | Some(Err(_))) => { let stream_schema = self.0[stream_idx].get_ref().schema(); // Replace the stream with an empty stream, so we can drop memory usage @@ -229,7 +229,7 @@ impl std::fmt::Debug for FieldCursorStream { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("PrimitiveCursorStream") .field("num_streams", &self.streams) - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion/physical-plan/src/spill/spill_pool.rs b/datafusion/physical-plan/src/spill/spill_pool.rs index 56d8d11201f46..43250d33d29dd 100644 --- a/datafusion/physical-plan/src/spill/spill_pool.rs +++ b/datafusion/physical-plan/src/spill/spill_pool.rs @@ -213,9 +213,7 @@ impl SpillPoolSink { let mut shared = self.shared.lock(); // Create new file if there is none available to append to - let write_file = if !shared.open_write_files.is_empty() { - shared.open_write_files.pop_front().unwrap() - } else { + let write_file = if shared.open_write_files.is_empty() { let spill_manager = Arc::clone(&shared.spill_manager); // Release shared lock before disk I/O (fine-grained locking) drop(shared); @@ -240,6 +238,8 @@ impl SpillPoolSink { shared.files.push_back(Arc::clone(&file_shared)); shared.wake(); // Wake readers waiting for new files file_shared + } else { + shared.open_write_files.pop_front().unwrap() }; // Release shared lock before file I/O (fine-grained locking) @@ -730,11 +730,10 @@ impl Stream for SpillPoolReader { // Clear current file and continue loop to get next file self.current_file = None; continue; - } else { - // Stream exhausted but writer not finished - unexpected - // This shouldn't happen with proper coordination - return Poll::Ready(None); } + // Stream exhausted but writer not finished - unexpected + // This shouldn't happen with proper coordination + return Poll::Ready(None); } Poll::Pending => { // File not ready yet (waiting for writer) diff --git a/datafusion/physical-plan/src/stream.rs b/datafusion/physical-plan/src/stream.rs index bc549f442001c..ea757551625b7 100644 --- a/datafusion/physical-plan/src/stream.rs +++ b/datafusion/physical-plan/src/stream.rs @@ -451,7 +451,7 @@ impl std::fmt::Debug for RecordBatchStreamAdapter { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("RecordBatchStreamAdapter") .field("schema", &self.schema) - .finish() + .finish_non_exhaustive() } } diff --git a/datafusion/physical-plan/src/test/exec.rs b/datafusion/physical-plan/src/test/exec.rs index f9517469d55ab..043c81012b881 100644 --- a/datafusion/physical-plan/src/test/exec.rs +++ b/datafusion/physical-plan/src/test/exec.rs @@ -1069,12 +1069,11 @@ impl Stream for PanicStream { self.ready = false; let batch = RecordBatch::new_empty(Arc::clone(&self.schema)); return Poll::Ready(Some(Ok(batch))); - } else { - self.ready = true; - // get called again - cx.waker().wake_by_ref(); - return Poll::Pending; } + self.ready = true; + // get called again + cx.waker().wake_by_ref(); + return Poll::Pending; } panic!("PanickingStream did panic: {}", self.partition) } diff --git a/datafusion/physical-plan/src/topk/mod.rs b/datafusion/physical-plan/src/topk/mod.rs index 0ca700cb37655..c97aba3552f5b 100644 --- a/datafusion/physical-plan/src/topk/mod.rs +++ b/datafusion/physical-plan/src/topk/mod.rs @@ -815,13 +815,12 @@ impl TopK { (&batch).record_output(&metrics.baseline); batches.push(Ok(batch)); break; - } else { - let head = batch.slice(0, batch_size); - (&head).record_output(&metrics.baseline); - batches.push(Ok(head)); - let remaining_length = batch.num_rows() - batch_size; - batch = batch.slice(batch_size, remaining_length); } + let head = batch.slice(0, batch_size); + (&head).record_output(&metrics.baseline); + batches.push(Ok(head)); + let remaining_length = batch.num_rows() - batch_size; + batch = batch.slice(batch_size, remaining_length); } } Ok(Box::pin(RecordBatchStreamAdapter::new( diff --git a/datafusion/physical-plan/src/union.rs b/datafusion/physical-plan/src/union.rs index 557d8f21d136b..c4240eeac29f5 100644 --- a/datafusion/physical-plan/src/union.rs +++ b/datafusion/physical-plan/src/union.rs @@ -394,9 +394,8 @@ impl ExecutionPlan for UnionExec { baseline_metrics, None, ))); - } else { - partition -= input.output_partitioning().partition_count(); } + partition -= input.output_partitioning().partition_count(); } warn!("Error in Union: Partition {partition} not found"); diff --git a/datafusion/physical-plan/src/unnest.rs b/datafusion/physical-plan/src/unnest.rs index d93e0280515c6..0f631b950983a 100644 --- a/datafusion/physical-plan/src/unnest.rs +++ b/datafusion/physical-plan/src/unnest.rs @@ -1004,9 +1004,10 @@ fn build_batch( // Original batch has the same columns // All unnesting results are written to temp_batch for depth in (1..=max_recursion).rev() { - let input = match depth == max_recursion { - true => batch.columns(), - false => &flatten_arrs, + let input = if depth == max_recursion { + batch.columns() + } else { + &flatten_arrs }; // Only sound for a single non-recursive level: with recursion the deeper // levels' lengths depend on arrays that do not exist yet, which is also why 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 0c6e77a1d6f70..a65114a1da179 100644 --- a/datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs +++ b/datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs @@ -327,9 +327,7 @@ impl BoundedWindowAggExec { } pub fn partition_keys(&self) -> Vec> { - if !self.can_repartition { - vec![] - } else { + if self.can_repartition { let all_partition_keys = self .window_expr() .iter() @@ -340,6 +338,8 @@ impl BoundedWindowAggExec { .into_iter() .min_by_key(|s| s.len()) .unwrap_or_else(Vec::new) + } else { + vec![] } } diff --git a/datafusion/physical-plan/src/windows/mod.rs b/datafusion/physical-plan/src/windows/mod.rs index 3f33dfedfd850..efda9ab2ce31d 100644 --- a/datafusion/physical-plan/src/windows/mod.rs +++ b/datafusion/physical-plan/src/windows/mod.rs @@ -147,8 +147,8 @@ fn window_expr_from_aggregate_expr( // Is there a potentially unlimited sized window frame? let unbounded_window = window_frame.is_ever_expanding(); - if !unbounded_window { - Arc::new(SlidingAggregateWindowExpr::new( + if unbounded_window { + Arc::new(PlainAggregateWindowExpr::new( aggregate, partition_by, order_by, @@ -156,7 +156,7 @@ fn window_expr_from_aggregate_expr( filter, )) } else { - Arc::new(PlainAggregateWindowExpr::new( + Arc::new(SlidingAggregateWindowExpr::new( aggregate, partition_by, order_by, diff --git a/datafusion/physical-plan/src/windows/window_agg_exec.rs b/datafusion/physical-plan/src/windows/window_agg_exec.rs index dba4c063d1911..9720c08d1599f 100644 --- a/datafusion/physical-plan/src/windows/window_agg_exec.rs +++ b/datafusion/physical-plan/src/windows/window_agg_exec.rs @@ -148,9 +148,7 @@ impl WindowAggExec { } pub fn partition_keys(&self) -> Vec> { - if !self.can_repartition { - vec![] - } else { + if self.can_repartition { let all_partition_keys = self .window_expr() .iter() @@ -161,6 +159,8 @@ impl WindowAggExec { .into_iter() .min_by_key(|s| s.len()) .unwrap_or_else(Vec::new) + } else { + vec![] } } } diff --git a/datafusion/proto-models/src/from_proto.rs b/datafusion/proto-models/src/from_proto.rs index 74ead8c52049b..5afaab533d2d9 100644 --- a/datafusion/proto-models/src/from_proto.rs +++ b/datafusion/proto-models/src/from_proto.rs @@ -191,27 +191,27 @@ impl From for NullEquality { impl From<&CsvOptionsProto> for CsvOptions { fn from(proto: &CsvOptionsProto) -> Self { CsvOptions { - has_header: if !proto.has_header.is_empty() { - Some(proto.has_header[0] != 0) - } else { + has_header: if proto.has_header.is_empty() { None + } else { + Some(proto.has_header[0] != 0) }, delimiter: proto.delimiter.first().copied().unwrap_or(b','), quote: proto.quote.first().copied().unwrap_or(b'"'), - terminator: if !proto.terminator.is_empty() { - Some(proto.terminator[0]) - } else { + terminator: if proto.terminator.is_empty() { None - }, - escape: if !proto.escape.is_empty() { - Some(proto.escape[0]) } else { - None + Some(proto.terminator[0]) }, - double_quote: if !proto.double_quote.is_empty() { - Some(proto.double_quote[0] != 0) + escape: if proto.escape.is_empty() { + None } else { + Some(proto.escape[0]) + }, + double_quote: if proto.double_quote.is_empty() { None + } else { + Some(proto.double_quote[0] != 0) }, compression: match proto.compression { 0 => CompressionTypeVariant::GZIP, @@ -256,10 +256,10 @@ impl From<&CsvOptionsProto> for CsvOptions { } else { Some(proto.null_regex.clone()) }, - comment: if !proto.comment.is_empty() { - Some(proto.comment[0]) - } else { + comment: if proto.comment.is_empty() { None + } else { + Some(proto.comment[0]) }, newlines_in_values: if proto.newlines_in_values.is_empty() { None diff --git a/datafusion/proto/src/logical_plan/from_proto.rs b/datafusion/proto/src/logical_plan/from_proto.rs index a1a1ff6f04fe4..71eb39b899d29 100644 --- a/datafusion/proto/src/logical_plan/from_proto.rs +++ b/datafusion/proto/src/logical_plan/from_proto.rs @@ -205,7 +205,7 @@ pub fn parse_expr( let window_frame = WindowFrame::try_from(window_frame.clone())?; window_frame .regularize_order_bys(&mut order_by) - .map(|_| window_frame) + .map(|()| window_frame) }) .transpose()? .ok_or_else(|| { diff --git a/datafusion/proto/src/logical_plan/mod.rs b/datafusion/proto/src/logical_plan/mod.rs index 7fa6fd4ed5822..b80a4b140b80c 100644 --- a/datafusion/proto/src/logical_plan/mod.rs +++ b/datafusion/proto/src/logical_plan/mod.rs @@ -807,10 +807,10 @@ impl AsLogicalPlan for LogicalPlanNode { "Protobuf deserialization error, CreateExternalTableNode was missing required table constraints." ) })?; - let definition = if !create_extern_table.definition.is_empty() { - Some(create_extern_table.definition.clone()) - } else { + let definition = if create_extern_table.definition.is_empty() { None + } else { + Some(create_extern_table.definition.clone()) }; let mut order_exprs = vec![]; @@ -874,10 +874,10 @@ impl AsLogicalPlan for LogicalPlanNode { "Protobuf deserialization error, CreateViewNode has invalid LogicalPlan input." ))? .try_into_logical_plan(ctx, extension_codec)?; - let definition = if !create_view.definition.is_empty() { - Some(create_view.definition.clone()) - } else { + let definition = if create_view.definition.is_empty() { None + } else { + Some(create_view.definition.clone()) }; Ok(LogicalPlan::Ddl(DdlStatement::CreateView(CreateView { @@ -1136,10 +1136,10 @@ impl AsLogicalPlan for LogicalPlanNode { let input: LogicalPlan = into_logical_plan!(scan.input, ctx, extension_codec)?; - let definition = if !scan.definition.is_empty() { - Some(scan.definition.clone()) - } else { + let definition = if scan.definition.is_empty() { None + } else { + Some(scan.definition.clone()) }; let provider = ViewTable::new(input, definition); diff --git a/datafusion/proto/src/physical_plan/mod.rs b/datafusion/proto/src/physical_plan/mod.rs index 26d1a8ed83d36..65e6b25c4084b 100644 --- a/datafusion/proto/src/physical_plan/mod.rs +++ b/datafusion/proto/src/physical_plan/mod.rs @@ -1283,7 +1283,7 @@ pub trait PhysicalPlanNodeExt: Sized { let mut buf: Vec = vec![]; match codec.try_encode(Arc::clone(&plan_clone), &mut buf, proto_converter) { - Ok(_) => { + Ok(()) => { let inputs: Vec = plan_clone .children() .into_iter() @@ -1990,7 +1990,7 @@ impl ComposedPhysicalExtensionCodec { // find the encoder for (position, codec) in self.codecs.iter().enumerate() { match encode(codec.as_ref(), &mut data) { - Ok(_) => { + Ok(()) => { encoder_position = Some(position as u32); break; } diff --git a/datafusion/proto/src/physical_plan/to_proto.rs b/datafusion/proto/src/physical_plan/to_proto.rs index 5ae57752de676..94fa42b09b48b 100644 --- a/datafusion/proto/src/physical_plan/to_proto.rs +++ b/datafusion/proto/src/physical_plan/to_proto.rs @@ -323,7 +323,7 @@ pub fn serialize_physical_expr_with_converter( } else { let mut buf: Vec = vec![]; match codec.try_encode_expr(value, &mut buf, &ctx) { - Ok(_) => { + Ok(()) => { let inputs: Vec = value .children() .into_iter() diff --git a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs index 7c916db9c3cd1..7636cb06aa7f5 100644 --- a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs +++ b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs @@ -1673,7 +1673,7 @@ async fn roundtrip_logical_plan_prepared_statement_with_metadata() -> Result<()> .unwrap(); let prepared = LogicalPlanBuilder::new(plan) .prepare( - "".to_string(), + String::new(), vec![ Field::new("", DataType::Int32, true) .with_metadata( diff --git a/datafusion/pruning/src/pruning_predicate.rs b/datafusion/pruning/src/pruning_predicate.rs index a30d52bd34825..1d3a213c443d2 100644 --- a/datafusion/pruning/src/pruning_predicate.rs +++ b/datafusion/pruning/src/pruning_predicate.rs @@ -1634,12 +1634,12 @@ fn build_predicate_expression( } if let Some(not) = expr.downcast_ref::() { // match !col (don't do so recursively) - if let Some(col) = not.arg().downcast_ref::() { - return build_single_column_expr(col, schema, required_columns, true) - .unwrap_or_else(|| unhandled_hook.handle(expr)); + return if let Some(col) = not.arg().downcast_ref::() { + build_single_column_expr(col, schema, required_columns, true) + .unwrap_or_else(|| unhandled_hook.handle(expr)) } else { - return unhandled_hook.handle(expr); - } + unhandled_hook.handle(expr) + }; } if let Some(in_list) = expr.downcast_ref::() { // Keep the existing expression shape for lists of at most 20 values. @@ -1682,9 +1682,8 @@ fn build_predicate_expression( unhandled_hook, max_in_list_size, ); - } else { - return unhandled_hook.handle(expr); } + return unhandled_hook.handle(expr); } let (left, op, right) = { diff --git a/datafusion/spark/src/function/conversion/cast.rs b/datafusion/spark/src/function/conversion/cast.rs index 45d1b336261d7..6e7fc194e4a49 100644 --- a/datafusion/spark/src/function/conversion/cast.rs +++ b/datafusion/spark/src/function/conversion/cast.rs @@ -177,9 +177,11 @@ fn get_target_type_from_scalar_args( let type_arg = scalar_args.get(1).and_then(|opt| *opt); match type_arg { - Some(ScalarValue::Utf8(Some(s))) - | Some(ScalarValue::LargeUtf8(Some(s))) - | Some(ScalarValue::Utf8View(Some(s))) => parse_target_type(s, timezone), + Some( + ScalarValue::Utf8(Some(s)) + | ScalarValue::LargeUtf8(Some(s)) + | ScalarValue::Utf8View(Some(s)), + ) => parse_target_type(s, timezone), _ => exec_err!( "spark_cast requires second argument to be a string of target data type ex: timestamp" ), diff --git a/datafusion/spark/src/function/datetime/date_part.rs b/datafusion/spark/src/function/datetime/date_part.rs index 91bdb9a55318b..7ad400f51801a 100644 --- a/datafusion/spark/src/function/datetime/date_part.rs +++ b/datafusion/spark/src/function/datetime/date_part.rs @@ -99,9 +99,11 @@ impl ScalarUDFImpl for SparkDatePart { let [part_expr, date_expr] = take_function_args(self.name(), args)?; let part = match part_expr.as_literal() { - Some(ScalarValue::Utf8(Some(v))) - | Some(ScalarValue::Utf8View(Some(v))) - | Some(ScalarValue::LargeUtf8(Some(v))) => v.to_lowercase(), + Some( + ScalarValue::Utf8(Some(v)) + | ScalarValue::Utf8View(Some(v)) + | ScalarValue::LargeUtf8(Some(v)), + ) => v.to_lowercase(), _ => { return internal_err!( "First argument of `DATE_PART` must be non-null scalar Utf8" diff --git a/datafusion/spark/src/function/datetime/date_trunc.rs b/datafusion/spark/src/function/datetime/date_trunc.rs index c8b0fbca36165..5c2c7a4e2e2f7 100644 --- a/datafusion/spark/src/function/datetime/date_trunc.rs +++ b/datafusion/spark/src/function/datetime/date_trunc.rs @@ -97,9 +97,11 @@ impl ScalarUDFImpl for SparkDateTrunc { let [fmt_expr, ts_expr] = take_function_args(self.name(), args)?; let fmt = match fmt_expr.as_literal() { - Some(ScalarValue::Utf8(Some(v))) - | Some(ScalarValue::Utf8View(Some(v))) - | Some(ScalarValue::LargeUtf8(Some(v))) => v.to_lowercase(), + Some( + ScalarValue::Utf8(Some(v)) + | ScalarValue::Utf8View(Some(v)) + | ScalarValue::LargeUtf8(Some(v)), + ) => v.to_lowercase(), _ => { return plan_err!( "First argument of `DATE_TRUNC` must be non-null scalar Utf8" diff --git a/datafusion/spark/src/function/datetime/time_trunc.rs b/datafusion/spark/src/function/datetime/time_trunc.rs index a66b8e94685aa..d4c239ae6347f 100644 --- a/datafusion/spark/src/function/datetime/time_trunc.rs +++ b/datafusion/spark/src/function/datetime/time_trunc.rs @@ -91,9 +91,11 @@ impl ScalarUDFImpl for SparkTimeTrunc { let fmt_expr = &args[0]; let fmt = match fmt_expr.as_literal() { - Some(ScalarValue::Utf8(Some(v))) - | Some(ScalarValue::Utf8View(Some(v))) - | Some(ScalarValue::LargeUtf8(Some(v))) => v.to_lowercase(), + Some( + ScalarValue::Utf8(Some(v)) + | ScalarValue::Utf8View(Some(v)) + | ScalarValue::LargeUtf8(Some(v)), + ) => v.to_lowercase(), _ => { return plan_err!( "First argument of `TIME_TRUNC` must be non-null scalar Utf8" diff --git a/datafusion/spark/src/function/datetime/trunc.rs b/datafusion/spark/src/function/datetime/trunc.rs index 9d7da5969a525..f8193f5566c9a 100644 --- a/datafusion/spark/src/function/datetime/trunc.rs +++ b/datafusion/spark/src/function/datetime/trunc.rs @@ -95,9 +95,11 @@ impl ScalarUDFImpl for SparkTrunc { let [dt_expr, fmt_expr] = take_function_args(self.name(), args)?; let fmt = match fmt_expr.as_literal() { - Some(ScalarValue::Utf8(Some(v))) - | Some(ScalarValue::Utf8View(Some(v))) - | Some(ScalarValue::LargeUtf8(Some(v))) => v.to_lowercase(), + Some( + ScalarValue::Utf8(Some(v)) + | ScalarValue::Utf8View(Some(v)) + | ScalarValue::LargeUtf8(Some(v)), + ) => v.to_lowercase(), _ => { return plan_err!( "Second argument of `TRUNC` must be non-null scalar Utf8" diff --git a/datafusion/spark/src/function/hash/xxhash64.rs b/datafusion/spark/src/function/hash/xxhash64.rs index 9d02a51b2217e..76c75fa03b917 100644 --- a/datafusion/spark/src/function/hash/xxhash64.rs +++ b/datafusion/spark/src/function/hash/xxhash64.rs @@ -108,10 +108,7 @@ fn create_xxhash64_hashes_dictionary( first_col: bool, ) -> Result<()> { let dict_array = array.as_any().downcast_ref::>().unwrap(); - if !first_col { - let unpacked = take(dict_array.values().as_ref(), dict_array.keys(), None)?; - create_xxhash64_hashes(&[unpacked], hashes_buffer)?; - } else { + if first_col { // Hash each dictionary value once, then look up by key. This avoids // redundant hashing of large dictionary entries (e.g. long strings). let dict_values = Arc::clone(dict_array.values()); @@ -124,6 +121,9 @@ fn create_xxhash64_hashes_dictionary( } // No update for Null keys, consistent with other types. } + } else { + let unpacked = take(dict_array.values().as_ref(), dict_array.keys(), None)?; + create_xxhash64_hashes(&[unpacked], hashes_buffer)?; } Ok(()) } diff --git a/datafusion/spark/src/function/string/char.rs b/datafusion/spark/src/function/string/char.rs index 5d6de3ae368e3..0f6efd23ee4d0 100644 --- a/datafusion/spark/src/function/string/char.rs +++ b/datafusion/spark/src/function/string/char.rs @@ -86,9 +86,9 @@ fn spark_chr(args: &[ColumnarValue]) -> Result { } ColumnarValue::Scalar(ScalarValue::Int64(Some(value))) => { if value < 0 { - Ok(ColumnarValue::Scalar(ScalarValue::Utf8(Some( - "".to_string(), - )))) + Ok(ColumnarValue::Scalar(ScalarValue::Utf8( + Some(String::new()), + ))) } else { match core::char::from_u32((value % 256) as u32) { Some(ch) => Ok(ColumnarValue::Scalar(ScalarValue::Utf8(Some( diff --git a/datafusion/spark/src/function/string/format_string.rs b/datafusion/spark/src/function/string/format_string.rs index 131d1c14dfe5b..5b6de1e2f86b1 100644 --- a/datafusion/spark/src/function/string/format_string.rs +++ b/datafusion/spark/src/function/string/format_string.rs @@ -119,9 +119,11 @@ impl ScalarUDFImpl for FormatStringFunc { ColumnarValue::Scalar(ScalarValue::Utf8View(None)) => { Ok(ColumnarValue::Scalar(ScalarValue::Utf8View(None))) } - ColumnarValue::Scalar(ScalarValue::Utf8(Some(fmt))) - | ColumnarValue::Scalar(ScalarValue::LargeUtf8(Some(fmt))) - | ColumnarValue::Scalar(ScalarValue::Utf8View(Some(fmt))) => { + ColumnarValue::Scalar( + ScalarValue::Utf8(Some(fmt)) + | ScalarValue::LargeUtf8(Some(fmt)) + | ScalarValue::Utf8View(Some(fmt)), + ) => { let formatter = Formatter::parse(fmt, &data_types)?; let mut result = Vec::with_capacity(len.unwrap_or(1)); for i in 0..len.unwrap_or(1) { @@ -1817,13 +1819,13 @@ impl ConversionSpecifier { let (prefix, suffix) = if negative && self.negative_in_parentheses { ("(".to_owned(), ")".to_owned()) } else if negative { - ("-".to_owned(), "".to_owned()) + ("-".to_owned(), String::new()) } else if self.force_sign { - ("+".to_owned(), "".to_owned()) + ("+".to_owned(), String::new()) } else if self.space_sign { - (" ".to_owned(), "".to_owned()) + (" ".to_owned(), String::new()) } else { - ("".to_owned(), "".to_owned()) + (String::new(), String::new()) }; self.format_decimal_integer(writer, abs_val, prefix, &suffix); diff --git a/datafusion/spark/src/function/string/length.rs b/datafusion/spark/src/function/string/length.rs index 8c5539a0577d8..8e19e84edfc4c 100644 --- a/datafusion/spark/src/function/string/length.rs +++ b/datafusion/spark/src/function/string/length.rs @@ -270,7 +270,7 @@ mod tests { test_spark_length_string!(Some(String::from("josé")), Ok(Some(4))); // test long strings (more than 12 bytes for StringView) test_spark_length_string!(Some(String::from("joséjoséjoséjosé")), Ok(Some(16))); - test_spark_length_string!(Some(String::from("")), Ok(Some(0))); + test_spark_length_string!(Some(String::new()), Ok(Some(0))); test_spark_length_string!(None, Ok(None)); test_spark_length_binary!(Some(String::from("chars").into_bytes()), Ok(Some(5))); @@ -280,7 +280,7 @@ mod tests { Some(String::from("joséjoséjoséjosé").into_bytes()), Ok(Some(20)) ); - test_spark_length_binary!(Some(String::from("").into_bytes()), Ok(Some(0))); + test_spark_length_binary!(Some(String::new().into_bytes()), Ok(Some(0))); test_spark_length_binary!(None, Ok(None)); Ok(()) diff --git a/datafusion/spark/src/function/string/luhn_check.rs b/datafusion/spark/src/function/string/luhn_check.rs index 9241f5e70d085..564c9c223cd0c 100644 --- a/datafusion/spark/src/function/string/luhn_check.rs +++ b/datafusion/spark/src/function/string/luhn_check.rs @@ -101,16 +101,18 @@ impl ScalarUDFImpl for SparkLuhnCheck { exec_err!("Unsupported data type {other:?} for function `luhn_check`") } }, - ColumnarValue::Scalar(ScalarValue::Utf8(Some(s))) - | ColumnarValue::Scalar(ScalarValue::LargeUtf8(Some(s))) - | ColumnarValue::Scalar(ScalarValue::Utf8View(Some(s))) => Ok( - ColumnarValue::Scalar(ScalarValue::Boolean(Some(luhn_check_impl(s)))), - ), - ColumnarValue::Scalar(ScalarValue::Utf8(None)) - | ColumnarValue::Scalar(ScalarValue::LargeUtf8(None)) - | ColumnarValue::Scalar(ScalarValue::Utf8View(None)) => { - Ok(ColumnarValue::Scalar(ScalarValue::Boolean(None))) - } + ColumnarValue::Scalar( + ScalarValue::Utf8(Some(s)) + | ScalarValue::LargeUtf8(Some(s)) + | ScalarValue::Utf8View(Some(s)), + ) => Ok(ColumnarValue::Scalar(ScalarValue::Boolean(Some( + luhn_check_impl(s), + )))), + ColumnarValue::Scalar( + ScalarValue::Utf8(None) + | ScalarValue::LargeUtf8(None) + | ScalarValue::Utf8View(None), + ) => Ok(ColumnarValue::Scalar(ScalarValue::Boolean(None))), other => { exec_err!("Unsupported data type {other:?} for function `luhn_check`") } diff --git a/datafusion/spark/src/function/url/parse_url.rs b/datafusion/spark/src/function/url/parse_url.rs index c385f43e49343..84a929b915816 100644 --- a/datafusion/spark/src/function/url/parse_url.rs +++ b/datafusion/spark/src/function/url/parse_url.rs @@ -82,7 +82,11 @@ impl ParseUrl { fn parse(value: &str, part: &str, key: Option<&str>) -> Result> { let url: std::result::Result = Url::parse(value); if url == Err(ParseError::RelativeUrlWithoutBase) { - return if !value.contains("://") { + return if value.contains("://") { + Err(exec_datafusion_err!( + "The url is invalid: {value}. Use `try_parse_url` to tolerate invalid URL and return NULL instead. SQLSTATE: 22P02" + )) + } else { // Schemeless URLs are treated as relative URIs (like java.net.URI). // Manually parse path, query, and fragment components. let (without_fragment, fragment) = match value.split_once('#') { @@ -107,10 +111,6 @@ impl ParseUrl { // HOST, PROTOCOL, AUTHORITY, USERINFO → NULL _ => None, }) - } else { - Err(exec_datafusion_err!( - "The url is invalid: {value}. Use `try_parse_url` to tolerate invalid URL and return NULL instead. SQLSTATE: 22P02" - )) }; } url.map_err(|e| exec_datafusion_err!("{e:?}")) @@ -404,7 +404,7 @@ mod tests { fn test_parse_path_empty_vs_root() -> Result<()> { assert_eq!( ParseUrl::parse("https://example.com", "PATH", None)?, - Some("".to_string()) + Some(String::new()) ); assert_eq!( ParseUrl::parse("https://example.com/", "PATH", None)?, @@ -430,7 +430,7 @@ mod tests { ); assert_eq!( ParseUrl::parse("http://ex.com?key=", "QUERY", Some("key"))?, - Some("".to_string()) + Some(String::new()) ); assert_eq!( ParseUrl::parse("http://ex.com?keyonly", "QUERY", Some("keyonly"))?, @@ -449,10 +449,10 @@ mod tests { #[test] fn test_parse_empty_path_file() -> Result<()> { - assert_eq!(ParseUrl::parse("", "PATH", None)?, Some("".to_string())); + assert_eq!(ParseUrl::parse("", "PATH", None)?, Some(String::new())); assert_eq!( ParseUrl::parse("http://example.com", "FILE", None)?, - Some("".to_string()) + Some(String::new()) ); assert_eq!( ParseUrl::parse("http://example.com?foo=bar", "FILE", None)?, @@ -460,7 +460,7 @@ mod tests { ); assert_eq!( ParseUrl::parse("http://example.com#fragment", "FILE", None)?, - Some("".to_string()) + Some(String::new()) ); assert_eq!( ParseUrl::parse("http://example.com/?foo=bar", "FILE", None)?, diff --git a/datafusion/sql/src/expr/function.rs b/datafusion/sql/src/expr/function.rs index f1ab0db470008..75ba99a1caca4 100644 --- a/datafusion/sql/src/expr/function.rs +++ b/datafusion/sql/src/expr/function.rs @@ -289,9 +289,9 @@ impl SqlToRel<'_, S> { && args.iter().all(|arg| { matches!( arg.get_type(schema), - Ok(DataType::List(_)) - | Ok(DataType::LargeList(_)) - | Ok(DataType::FixedSizeList(_, _)) + Ok(DataType::List(_) + | DataType::LargeList(_) + | DataType::FixedSizeList(_, _)) ) }); @@ -345,20 +345,19 @@ impl SqlToRel<'_, S> { if name.eq_ignore_ascii_case(inner.name()) { return Ok(Expr::ScalarFunction(inner)); - } else { - // If the function is called by an alias, a verbose string representation is created - // (e.g., "my_alias(arg1, arg2)") and the expression is wrapped in an `Alias` - // to ensure the output column name matches the user's query. - let arg_names = inner - .args - .iter() - .map(|arg| arg.to_string()) - .collect::>() - .join(","); - let verbose_alias = format!("{name}({arg_names})"); - - return Ok(Expr::ScalarFunction(inner).alias(verbose_alias)); } + // If the function is called by an alias, a verbose string representation is created + // (e.g., "my_alias(arg1, arg2)") and the expression is wrapped in an `Alias` + // to ensure the output column name matches the user's query. + let arg_names = inner + .args + .iter() + .map(|arg| arg.to_string()) + .collect::>() + .join(","); + let verbose_alias = format!("{name}({arg_names})"); + + return Ok(Expr::ScalarFunction(inner).alias(verbose_alias)); } if let Some(fm) = self.context_provider.get_higher_order_meta(&name) { @@ -523,20 +522,19 @@ impl SqlToRel<'_, S> { if name.eq_ignore_ascii_case(inner.name()) { return Ok(Expr::HigherOrderFunction(inner)); - } else { - // If the function is called by an alias, a verbose string representation is created - // (e.g., "my_alias(arg1, arg2)") and the expression is wrapped in an `Alias` - // to ensure the output column name matches the user's query. - let arg_names = inner - .args - .iter() - .map(|arg| arg.to_string()) - .collect::>() - .join(","); - let verbose_alias = format!("{name}({arg_names})"); - - return Ok(Expr::HigherOrderFunction(inner).alias(verbose_alias)); } + // If the function is called by an alias, a verbose string representation is created + // (e.g., "my_alias(arg1, arg2)") and the expression is wrapped in an `Alias` + // to ensure the output column name matches the user's query. + let arg_names = inner + .args + .iter() + .map(|arg| arg.to_string()) + .collect::>() + .join(","); + let verbose_alias = format!("{name}({arg_names})"); + + return Ok(Expr::HigherOrderFunction(inner).alias(verbose_alias)); } // Build Unnest expression. @@ -607,7 +605,7 @@ impl SqlToRel<'_, S> { let window_frame: WindowFrame = window_frame.clone().try_into()?; window_frame .regularize_order_bys(&mut order_by) - .map(|_| window_frame) + .map(|()| window_frame) }) .transpose()?; @@ -695,21 +693,20 @@ impl SqlToRel<'_, S> { if name.eq_ignore_ascii_case(inner.fun.name()) { return Ok(Expr::WindowFunction(Box::new(inner))); - } else { - // If the function is called by an alias, a verbose string representation is created - // (e.g., "my_alias(arg1, arg2)") and the expression is wrapped in an `Alias` - // to ensure the output column name matches the user's query. - let arg_names = inner - .params - .args - .iter() - .map(|arg| arg.to_string()) - .collect::>() - .join(","); - let verbose_alias = format!("{name}({arg_names})"); - - return Ok(Expr::WindowFunction(Box::new(inner)).alias(verbose_alias)); } + // If the function is called by an alias, a verbose string representation is created + // (e.g., "my_alias(arg1, arg2)") and the expression is wrapped in an `Alias` + // to ensure the output column name matches the user's query. + let arg_names = inner + .params + .args + .iter() + .map(|arg| arg.to_string()) + .collect::>() + .join(","); + let verbose_alias = format!("{name}({arg_names})"); + + return Ok(Expr::WindowFunction(Box::new(inner)).alias(verbose_alias)); } } else { // User defined aggregate functions (UDAF) have precedence in case it has the same name as a scalar built-in function @@ -749,7 +746,9 @@ impl SqlToRel<'_, S> { } let order_by: Vec = if supports_within_group { - if !within_group.is_empty() { + if within_group.is_empty() { + vec![] + } else { // WITHIN GROUP syntax let sorts = self.order_by_to_sort_expr( within_group, @@ -780,8 +779,6 @@ impl SqlToRel<'_, S> { args = std::iter::once(value_expr).chain(args).collect(); sorts - } else { - vec![] } } else { // Normal aggregate behavior @@ -851,21 +848,20 @@ impl SqlToRel<'_, S> { if name.eq_ignore_ascii_case(inner.func.name()) { return Ok(Expr::AggregateFunction(inner)); - } else { - // If the function is called by an alias, a verbose string representation is created - // (e.g., "my_alias(arg1, arg2)") and the expression is wrapped in an `Alias` - // to ensure the output column name matches the user's query. - let arg_names = inner - .params - .args - .iter() - .map(|arg| arg.to_string()) - .collect::>() - .join(","); - let verbose_alias = format!("{name}({arg_names})"); - - return Ok(Expr::AggregateFunction(inner).alias(verbose_alias)); } + // If the function is called by an alias, a verbose string representation is created + // (e.g., "my_alias(arg1, arg2)") and the expression is wrapped in an `Alias` + // to ensure the output column name matches the user's query. + let arg_names = inner + .params + .args + .iter() + .map(|arg| arg.to_string()) + .collect::>() + .join(","); + let verbose_alias = format!("{name}({arg_names})"); + + return Ok(Expr::AggregateFunction(inner).alias(verbose_alias)); } } @@ -877,19 +873,15 @@ impl SqlToRel<'_, S> { .map(|part| part.as_ident().cloned().ok_or(())) .collect::, ()>>(); if let Ok(ids) = maybe_ids { - if ids.len() == 1 { - return self.sql_identifier_to_expr( + return if ids.len() == 1 { + self.sql_identifier_to_expr( ids.into_iter().next().unwrap(), schema, planner_context, - ); + ) } else { - return self.sql_compound_identifier_to_expr( - ids, - schema, - planner_context, - ); - } + self.sql_compound_identifier_to_expr(ids, schema, planner_context) + }; } } diff --git a/datafusion/sql/src/expr/grouping_set.rs b/datafusion/sql/src/expr/grouping_set.rs index bedbf2a7d3470..90ce673885425 100644 --- a/datafusion/sql/src/expr/grouping_set.rs +++ b/datafusion/sql/src/expr/grouping_set.rs @@ -48,12 +48,12 @@ impl SqlToRel<'_, S> { let args: Result> = exprs .into_iter() .map(|v| { - if v.len() != 1 { + if v.len() == 1 { + self.sql_expr_to_logical_expr(v[0].clone(), schema, planner_context) + } else { plan_err!( "Tuple expressions are not supported for Rollup expressions" ) - } else { - self.sql_expr_to_logical_expr(v[0].clone(), schema, planner_context) } }) .collect(); @@ -69,10 +69,10 @@ impl SqlToRel<'_, S> { let args: Result> = exprs .into_iter() .map(|v| { - if v.len() != 1 { - plan_err!("Tuple expressions not are supported for Cube expressions") - } else { + if v.len() == 1 { self.sql_expr_to_logical_expr(v[0].clone(), schema, planner_context) + } else { + plan_err!("Tuple expressions not are supported for Cube expressions") } }) .collect(); diff --git a/datafusion/sql/src/expr/mod.rs b/datafusion/sql/src/expr/mod.rs index f1661028ed051..7af76441644ad 100644 --- a/datafusion/sql/src/expr/mod.rs +++ b/datafusion/sql/src/expr/mod.rs @@ -802,11 +802,11 @@ impl SqlToRel<'_, S> { values: Vec, ) -> Result { match values.first() { - Some(SQLExpr::Identifier(_)) - | Some(SQLExpr::Value(_)) - | Some(SQLExpr::CompoundIdentifier(_)) => { - self.parse_struct(schema, planner_context, values, &[]) - } + Some( + SQLExpr::Identifier(_) + | SQLExpr::Value(_) + | SQLExpr::CompoundIdentifier(_), + ) => self.parse_struct(schema, planner_context, values, &[]), None => not_impl_err!("Empty tuple not supported yet"), _ => { not_impl_err!("Only identifiers and literals are supported in tuples") diff --git a/datafusion/sql/src/expr/value.rs b/datafusion/sql/src/expr/value.rs index 1307e917e4251..d0354b319f089 100644 --- a/datafusion/sql/src/expr/value.rs +++ b/datafusion/sql/src/expr/value.rs @@ -296,9 +296,8 @@ fn interval_literal(interval_value: SQLExpr, negative: bool) -> Result { return not_impl_err!( "Unsupported interval argument. Long number not supported: {interval_value:?}" ); - } else { - v.to_string() } + v.to_string() } SQLExpr::UnaryOp { op, expr } => { let negative = match op { diff --git a/datafusion/sql/src/parser.rs b/datafusion/sql/src/parser.rs index fcf4708f1bf94..503660d3a8de4 100644 --- a/datafusion/sql/src/parser.rs +++ b/datafusion/sql/src/parser.rs @@ -601,10 +601,10 @@ impl<'a> DFParser<'a> { token: &Token, ) -> Result<(), DataFusionError> { let next_token = self.parser.peek_token_ref(); - if next_token.token != *token { - self.expected(expected, next_token) - } else { + if next_token.token == *token { Ok(()) + } else { + self.expected(expected, next_token) } } @@ -756,9 +756,8 @@ impl<'a> DFParser<'a> { let token = self.parser.peek_token(); if token == Token::EOF || token == Token::SemiColon { break; - } else { - return self.expected("end of statement or ;", &token)?; } + return self.expected("end of statement or ;", &token)?; } } @@ -1208,9 +1207,8 @@ impl<'a> DFParser<'a> { let token = self.parser.peek_token(); if token == Token::EOF || token == Token::SemiColon { break; - } else { - return self.expected("end of statement or ;", &token)?; } + return self.expected("end of statement or ;", &token)?; } } diff --git a/datafusion/sql/src/planner.rs b/datafusion/sql/src/planner.rs index a3e6d75fdbfac..1d10ac8999685 100644 --- a/datafusion/sql/src/planner.rs +++ b/datafusion/sql/src/planner.rs @@ -612,14 +612,14 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> { Expr::Column(col) => match &col.relation { Some(r) => schema.field_with_qualified_name(r, &col.name).map(|_| ()), None => { - if !schema.fields_with_unqualified_name(&col.name).is_empty() { - Ok(()) - } else { + if schema.fields_with_unqualified_name(&col.name).is_empty() { Err(field_not_found( col.relation.clone(), col.name.as_str(), schema, )) + } else { + Ok(()) } } } diff --git a/datafusion/sql/src/relation/join.rs b/datafusion/sql/src/relation/join.rs index 475d9a5b38099..abc001567e3ee 100644 --- a/datafusion/sql/src/relation/join.rs +++ b/datafusion/sql/src/relation/join.rs @@ -133,11 +133,7 @@ impl SqlToRel<'_, S> { .into_iter() .map(|object_name| { let ObjectName(mut object_names) = object_name; - if object_names.len() != 1 { - not_impl_err!( - "Invalid identifier in USING clause. Expected single identifier, got {}", ObjectName(object_names) - ) - } else { + if object_names.len() == 1 { let id = object_names.swap_remove(0); id.as_ident() .ok_or_else(|| { @@ -146,6 +142,10 @@ impl SqlToRel<'_, S> { ) }) .map(|ident| Column::from_name(self.ident_normalizer.normalize(ident.clone()))) + } else { + not_impl_err!( + "Invalid identifier in USING clause. Expected single identifier, got {}", ObjectName(object_names) + ) } }) .collect::>>()?; diff --git a/datafusion/sql/src/select.rs b/datafusion/sql/src/select.rs index bbd9d203eb124..e5f4dc74f2925 100644 --- a/datafusion/sql/src/select.rs +++ b/datafusion/sql/src/select.rs @@ -520,7 +520,9 @@ impl SqlToRel<'_, S> { }; // DISTRIBUTE BY - let plan = if !select.distribute_by.is_empty() { + let plan = if select.distribute_by.is_empty() { + plan + } else { let x = select .distribute_by .iter() @@ -535,8 +537,6 @@ impl SqlToRel<'_, S> { LogicalPlanBuilder::from(plan) .repartition(Partitioning::DistributeBy(x))? .build()? - } else { - plan }; let plan = self.order_by(plan, order_by_rex)?; @@ -818,57 +818,56 @@ impl SqlToRel<'_, S> { if unnest_columns.is_empty() { break; - } else { - let mut unnest_options = UnnestOptions::new().with_preserve_nulls(false); - - #[allow(clippy::allow_attributes, clippy::mutable_key_type)] - // Expr contains Arc with interior mutability but is intentionally used as hash key - let mut projection_exprs = match &aggr_expr_using_columns { - Some(exprs) => (*exprs).clone(), - None => { - #[allow(clippy::allow_attributes, clippy::mutable_key_type)] - let mut columns = HashSet::new(); - for expr in &aggr_expr { - expr.apply(|expr| { - if let Expr::Column(c) = expr { - columns.insert(Expr::Column(c.clone())); - } - Ok(TreeNodeRecursion::Continue) - }) - // As the closure always returns Ok, this "can't" error - .expect("Unexpected error"); - } - aggr_expr_using_columns = Some(columns.clone()); - columns - } - }; - projection_exprs.extend(inner_projection_exprs); - - let mut unnest_col_vec = vec![]; - - for (col, maybe_list_unnest) in unnest_columns.into_iter() { - if let Some(list_unnest) = maybe_list_unnest { - unnest_options = list_unnest.into_iter().fold( - unnest_options, - |options, unnest_list| { - options.with_recursions(RecursionUnnestOption { - input_column: col.clone(), - output_column: unnest_list.output_column, - depth: unnest_list.depth, - }) - }, - ); + } + let mut unnest_options = UnnestOptions::new().with_preserve_nulls(false); + + #[allow(clippy::allow_attributes, clippy::mutable_key_type)] + // Expr contains Arc with interior mutability but is intentionally used as hash key + let mut projection_exprs = match &aggr_expr_using_columns { + Some(exprs) => (*exprs).clone(), + None => { + #[allow(clippy::allow_attributes, clippy::mutable_key_type)] + let mut columns = HashSet::new(); + for expr in &aggr_expr { + expr.apply(|expr| { + if let Expr::Column(c) = expr { + columns.insert(Expr::Column(c.clone())); + } + Ok(TreeNodeRecursion::Continue) + }) + // As the closure always returns Ok, this "can't" error + .expect("Unexpected error"); } - unnest_col_vec.push(col); + aggr_expr_using_columns = Some(columns.clone()); + columns } + }; + projection_exprs.extend(inner_projection_exprs); - intermediate_plan = LogicalPlanBuilder::from(intermediate_plan) - .project(projection_exprs)? - .unnest_columns_with_options(unnest_col_vec, unnest_options)? - .build()?; + let mut unnest_col_vec = vec![]; - intermediate_select_exprs = outer_projection_exprs; + for (col, maybe_list_unnest) in unnest_columns.into_iter() { + if let Some(list_unnest) = maybe_list_unnest { + unnest_options = list_unnest.into_iter().fold( + unnest_options, + |options, unnest_list| { + options.with_recursions(RecursionUnnestOption { + input_column: col.clone(), + output_column: unnest_list.output_column, + depth: unnest_list.depth, + }) + }, + ); + } + unnest_col_vec.push(col); } + + intermediate_plan = LogicalPlanBuilder::from(intermediate_plan) + .project(projection_exprs)? + .unnest_columns_with_options(unnest_col_vec, unnest_options)? + .build()?; + + intermediate_select_exprs = outer_projection_exprs; } Ok((intermediate_plan, intermediate_select_exprs)) diff --git a/datafusion/sql/src/statement.rs b/datafusion/sql/src/statement.rs index 1a9072212f2f3..1e2994811a74d 100644 --- a/datafusion/sql/src/statement.rs +++ b/datafusion/sql/src/statement.rs @@ -1412,7 +1412,7 @@ impl SqlToRel<'_, S> { .map(|t| { let name = match t.name.clone() { Some(name) => name.value, - None => "".to_string(), + None => String::new(), }; Arc::new(Field::new(name, t.data_type.clone(), true)) }) @@ -2004,14 +2004,14 @@ impl SqlToRel<'_, S> { return plan_err!("Unsupported Value {}", value); }; - if !(&key.contains('.')) { + if key.contains('.') { + options_map.insert(key.to_lowercase(), value_string); + } else { // If config does not belong to any namespace, assume it is // a format option and apply the format prefix for backwards // compatibility. let renamed_key = format!("format.{key}"); options_map.insert(renamed_key.to_lowercase(), value_string); - } else { - options_map.insert(key.to_lowercase(), value_string); } } @@ -2875,9 +2875,8 @@ impl SqlToRel<'_, S> { return schema_err!(SchemaError::DuplicateUnqualifiedField { name: c, }); - } else { - value_indices[column_index] = Some(i); } + value_indices[column_index] = Some(i); Ok(Arc::clone(table_schema.field(column_index))) }) .collect::>>()?; @@ -3036,7 +3035,7 @@ impl SqlToRel<'_, S> { _ => return plan_err!("Unsupported SHOW FUNCTIONS filter"), } } else { - "".to_string() + String::new() }; // Scalar / aggregate / window functions are resolved by joining @@ -3180,7 +3179,7 @@ FROM ( None => Ok(()), // BEGIN TRANSACTION Some(BeginTransactionKind::Transaction) => Ok(()), - Some(BeginTransactionKind::Work) | Some(BeginTransactionKind::Tran) => { + Some(BeginTransactionKind::Work | BeginTransactionKind::Tran) => { not_impl_err!("Transaction kind not supported: {kind:?}") } } diff --git a/datafusion/sql/src/unparser/expr.rs b/datafusion/sql/src/unparser/expr.rs index cd3ac1f3a455b..44e30f08e0892 100644 --- a/datafusion/sql/src/unparser/expr.rs +++ b/datafusion/sql/src/unparser/expr.rs @@ -3140,12 +3140,12 @@ mod tests { } fn string_literal_to_sql(&self, s: &str) -> Option { - if !s.is_ascii() { + if s.is_ascii() { + None + } else { Some(ast::Expr::value(ast::Value::NationalStringLiteral( s.to_string(), ))) - } else { - None } } } diff --git a/datafusion/sql/src/unparser/plan.rs b/datafusion/sql/src/unparser/plan.rs index 30320acbb24dc..5c954eccfa192 100644 --- a/datafusion/sql/src/unparser/plan.rs +++ b/datafusion/sql/src/unparser/plan.rs @@ -555,12 +555,12 @@ impl Unparser<'_> { // Generate the alias up front so that peel_to_unnest_with_modifiers // can rewrite ORDER BY placeholder columns to alias.VALUE. if self.dialect.unnest_as_lateral_flatten() && unnest_input_type.is_some() { - let flatten_alias_name = if !select.already_projected() { - select.next_flatten_alias() - } else { + let flatten_alias_name = if select.already_projected() { select .current_flatten_alias() .unwrap_or_else(|| select.next_flatten_alias()) + } else { + select.next_flatten_alias() }; if let Some((unnest, unnest_plan)) = self.peel_to_unnest_with_modifiers( @@ -1325,11 +1325,10 @@ impl Unparser<'_> { relation, )?; - let left_projection: Option> = if !already_projected - { - Some(select.pop_projections()) - } else { + let left_projection: Option> = if already_projected { None + } else { + Some(select.pop_projections()) }; let right_plan = Self::extract_join_input_table_scan_filters( @@ -1371,11 +1370,11 @@ impl Unparser<'_> { join_filters.as_ref(), )?; - let right_projection: Option> = if !already_projected + let right_projection: Option> = if already_projected { - Some(select.pop_projections()) - } else { None + } else { + Some(select.pop_projections()) }; match join.join_type { diff --git a/datafusion/sql/src/utils.rs b/datafusion/sql/src/utils.rs index 0f0b5fe44c77e..5fb82079b2f6b 100644 --- a/datafusion/sql/src/utils.rs +++ b/datafusion/sql/src/utils.rs @@ -711,7 +711,12 @@ pub(crate) fn rewrite_recursive_unnest_bottom_up( tnr: _, } = original_expr.clone().rewrite(&mut rewriter)?; - if !transformed { + if transformed { + if let Some(transformed_root_exprs) = rewriter.transformed_root_exprs { + return Ok(transformed_root_exprs); + } + Ok(vec![transformed_expr]) + } else { // TODO: remove the next line after `Expr::Wildcard` is removed #[expect(deprecated)] if matches!(&transformed_expr, Expr::Column(_)) @@ -726,11 +731,6 @@ pub(crate) fn rewrite_recursive_unnest_bottom_up( push_projection_dedupl(inner_projection_exprs, transformed_expr); Ok(vec![Expr::Column(Column::from_name(column_name))]) } - } else { - if let Some(transformed_root_exprs) = rewriter.transformed_root_exprs { - return Ok(transformed_root_exprs); - } - Ok(vec![transformed_expr]) } } diff --git a/datafusion/sql/tests/sql_integration.rs b/datafusion/sql/tests/sql_integration.rs index 00103bfd9f56a..4f42b35ba9dbf 100644 --- a/datafusion/sql/tests/sql_integration.rs +++ b/datafusion/sql/tests/sql_integration.rs @@ -5532,7 +5532,7 @@ fn test_using_join_wildcard_schema_semi_anti() { let s_columns = &["s.x1", "s.x2", "s.x3"]; let t_columns = &["t.x1", "t.x2", "t.x3"]; - let sql = "WITH + let sql = "WITH s AS (SELECT 1 AS x1, 2 AS x2, 3 AS x3), t AS (SELECT 1 AS x1, 4 AS x2, 5 AS x3) SELECT * FROM s LEFT SEMI JOIN t USING (x1)"; @@ -5774,7 +5774,7 @@ impl HigherOrderUDFImpl for MockArrayReduce { None, ]) } - (1, Some(accumulator)) | (0, Some(accumulator)) => { + (0 | 1, Some(accumulator)) => { // now we can use the merge output as it's accumulator and // as the finish parameter LambdaParametersProgress::Complete(vec![ diff --git a/datafusion/sqllogictest/bin/sqllogictests.rs b/datafusion/sqllogictest/bin/sqllogictests.rs index 71573c5e535df..92da3e6afb6b7 100644 --- a/datafusion/sqllogictest/bin/sqllogictests.rs +++ b/datafusion/sqllogictest/bin/sqllogictests.rs @@ -390,13 +390,13 @@ async fn run_tests() -> Result<()> { terminate_postgres_container().await?; // report on any errors - if !errors.is_empty() { + if errors.is_empty() { + Ok(()) + } else { for e in &errors { println!("{e}"); } exec_err!("{} failures", errors.len()) - } else { - Ok(()) } } diff --git a/datafusion/substrait/src/logical_plan/consumer/expr/literal.rs b/datafusion/substrait/src/logical_plan/consumer/expr/literal.rs index d7d7a69581f05..5aaf572113ce7 100644 --- a/datafusion/substrait/src/logical_plan/consumer/expr/literal.rs +++ b/datafusion/substrait/src/logical_plan/consumer/expr/literal.rs @@ -393,9 +393,8 @@ pub(crate) fn from_substrait_literal( return substrait_err!( "Cannot set subseconds field of IntervalDayToSecond without setting precision" ); - } else { - 0_i32 } + 0_i32 } Some(PrecisionMode::Precision(0)) => *subseconds as i32 * 1000, Some(PrecisionMode::Precision(3)) => *subseconds as i32, diff --git a/datafusion/substrait/src/logical_plan/consumer/expr/subquery.rs b/datafusion/substrait/src/logical_plan/consumer/expr/subquery.rs index 83cf8400eebfc..25a1be8001785 100644 --- a/datafusion/substrait/src/logical_plan/consumer/expr/subquery.rs +++ b/datafusion/substrait/src/logical_plan/consumer/expr/subquery.rs @@ -51,11 +51,7 @@ pub async fn from_subquery( match &subquery.subquery_type { Some(subquery_type) => match subquery_type { SubqueryType::InPredicate(in_predicate) => { - if in_predicate.needles.len() != 1 { - substrait_err!( - "InPredicate Subquery type must have exactly one Needle expression" - ) - } else { + if in_predicate.needles.len() == 1 { let needle_expr = &in_predicate.needles[0]; let haystack_expr = &in_predicate.haystack; if let Some(haystack_expr) = haystack_expr { @@ -81,6 +77,10 @@ pub async fn from_subquery( "InPredicate Subquery type must have a Haystack expression" ) } + } else { + substrait_err!( + "InPredicate Subquery type must have exactly one Needle expression" + ) } } SubqueryType::Scalar(query) => { diff --git a/datafusion/substrait/src/logical_plan/consumer/rel/project_rel.rs b/datafusion/substrait/src/logical_plan/consumer/rel/project_rel.rs index 5aea6c809b701..1219785722bd0 100644 --- a/datafusion/substrait/src/logical_plan/consumer/rel/project_rel.rs +++ b/datafusion/substrait/src/logical_plan/consumer/rel/project_rel.rs @@ -64,10 +64,10 @@ pub async fn from_project_rel( explicit_exprs.push(name_tracker.get_uniquely_named_expr(e)?); } - let input = if !window_exprs.is_empty() { - LogicalPlanBuilder::window_plan(input, window_exprs)? - } else { + let input = if window_exprs.is_empty() { input + } else { + LogicalPlanBuilder::window_plan(input, window_exprs)? }; let mut final_exprs: Vec = vec![]; diff --git a/datafusion/substrait/src/logical_plan/consumer/rel/read_rel.rs b/datafusion/substrait/src/logical_plan/consumer/rel/read_rel.rs index 2cad1440807a5..fc938ca49c233 100644 --- a/datafusion/substrait/src/logical_plan/consumer/rel/read_rel.rs +++ b/datafusion/substrait/src/logical_plan/consumer/rel/read_rel.rs @@ -145,7 +145,9 @@ pub async fn from_read_rel( })); } - let values = if !vt.expressions.is_empty() { + let values = if vt.expressions.is_empty() { + convert_literal_rows(consumer, vt, named_struct)? + } else { let mut exprs = vec![]; for row in &vt.expressions { if row.fields.len() != substrait_schema.fields().len() { @@ -195,8 +197,6 @@ pub async fn from_read_rel( exprs.push(row_exprs); } exprs - } else { - convert_literal_rows(consumer, vt, named_struct)? }; Ok(LogicalPlan::Values(Values { diff --git a/datafusion/substrait/src/logical_plan/consumer/utils.rs b/datafusion/substrait/src/logical_plan/consumer/utils.rs index 824c79452d86e..e4d25eac02d71 100644 --- a/datafusion/substrait/src/logical_plan/consumer/utils.rs +++ b/datafusion/substrait/src/logical_plan/consumer/utils.rs @@ -270,13 +270,13 @@ pub(super) fn rename_expressions( .zip(new_schema_fields) .map(|(old_expr, new_field)| { // Check if type (i.e. nested struct field names) match, use Cast to rename if needed - let new_expr = if &old_expr.get_type(input_schema)? != new_field.data_type() { + let new_expr = if &old_expr.get_type(input_schema)? == new_field.data_type() { + old_expr + } else { Expr::Cast(Cast::new( Box::new(old_expr), new_field.data_type().to_owned(), )) - } else { - old_expr }; // Alias column if needed to fix the top-level name match &new_expr { diff --git a/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs b/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs index c9f874dd9b095..26222dee96fbb 100644 --- a/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs +++ b/datafusion/substrait/tests/cases/roundtrip_logical_plan.rs @@ -2417,7 +2417,7 @@ fn check_post_join_filters(rel: &Rel) -> Result<()> { // recursively check JoinRels match check_post_join_filters(join.left.as_ref().unwrap().as_ref()) { Err(e) => Err(e), - Ok(_) => { + Ok(()) => { check_post_join_filters(join.right.as_ref().unwrap().as_ref()) } } @@ -2453,7 +2453,7 @@ fn check_post_join_filters(rel: &Rel) -> Result<()> { } Ok(()) } - Some(RelType::ExtensionLeaf(_)) | Some(RelType::Read(_)) => Ok(()), + Some(RelType::ExtensionLeaf(_) | RelType::Read(_)) => Ok(()), _ => not_impl_err!( "Unsupported Reltype: {:?} in post join filter check", rel.rel_type diff --git a/test-utils/src/array_gen/string.rs b/test-utils/src/array_gen/string.rs index 896182290ccca..cfc99e2ee7a64 100644 --- a/test-utils/src/array_gen/string.rs +++ b/test-utils/src/array_gen/string.rs @@ -92,7 +92,7 @@ impl StringArrayGenerator { fn random_string(rng: &mut StdRng, max_len: usize) -> String { // pick characters at random (not just ascii) match max_len { - 0 => "".to_string(), + 0 => String::new(), 1 => String::from(rng.random::()), _ => { let len = rng.random_range(1..=max_len);