From a9722a1171446f3f42326f392441b57037f59331 Mon Sep 17 00:00:00 2001 From: foskey51 Date: Sat, 8 Nov 2025 16:10:04 +0000 Subject: [PATCH 1/2] chore: enforce clippy lint needless_pass_by_value for datafusion-sql --- datafusion/sql/src/cte.rs | 12 ++++----- datafusion/sql/src/expr/binary_op.rs | 4 +-- datafusion/sql/src/expr/mod.rs | 14 +++++----- datafusion/sql/src/expr/subquery.rs | 8 +++--- datafusion/sql/src/lib.rs | 3 +++ datafusion/sql/src/parser.rs | 38 ++++++++++++++-------------- datafusion/sql/src/statement.rs | 12 ++++----- 7 files changed, 47 insertions(+), 44 deletions(-) diff --git a/datafusion/sql/src/cte.rs b/datafusion/sql/src/cte.rs index aceec676761c..8ccab9dd9a0b 100644 --- a/datafusion/sql/src/cte.rs +++ b/datafusion/sql/src/cte.rs @@ -46,7 +46,7 @@ impl SqlToRel<'_, S> { // Create a logical plan for the CTE let cte_plan = if is_recursive { - self.recursive_cte(cte_name.clone(), *cte.query, planner_context)? + self.recursive_cte(&cte_name, *cte.query, planner_context)? } else { self.non_recursive_cte(*cte.query, planner_context)? }; @@ -70,7 +70,7 @@ impl SqlToRel<'_, S> { fn recursive_cte( &self, - cte_name: String, + cte_name: &str, mut cte_query: Query, planner_context: &mut PlannerContext, ) -> Result { @@ -136,7 +136,7 @@ impl SqlToRel<'_, S> { // Step 2.1: Create a table source for the temporary relation let work_table_source = self .context_provider - .create_cte_work_table(&cte_name, Arc::clone(static_plan.schema().inner()))?; + .create_cte_work_table(cte_name, Arc::clone(static_plan.schema().inner()))?; // Step 2.2: Create a temporary relation logical plan that will be used // as the input to the recursive term @@ -147,14 +147,14 @@ impl SqlToRel<'_, S> { )? .build()?; - let name = cte_name.clone(); + let name = cte_name.to_string(); // Step 2.3: Register the temporary relation in the planning context // For all the self references in the variadic term, we'll replace it // with the temporary relation we created above by temporarily registering // it as a CTE. This temporary relation in the planning context will be // replaced by the actual CTE plan once we're done with the planning. - planner_context.insert_cte(cte_name.clone(), work_table_plan); + planner_context.insert_cte(cte_name.to_string(), work_table_plan); // ---------- Step 3: Compile the recursive term ------------------ // this uses the named_relation we inserted above to resolve the @@ -166,7 +166,7 @@ impl SqlToRel<'_, S> { // if not, it is a non-recursive CTE if !has_work_table_reference(&recursive_plan, &work_table_source) { // Remove the work table plan from the context - planner_context.remove_cte(&cte_name); + planner_context.remove_cte(cte_name); // Compile it as a non-recursive CTE return self.set_operation_to_plan( SetOperator::Union, diff --git a/datafusion/sql/src/expr/binary_op.rs b/datafusion/sql/src/expr/binary_op.rs index 1c06f5ee926f..f0ca54161782 100644 --- a/datafusion/sql/src/expr/binary_op.rs +++ b/datafusion/sql/src/expr/binary_op.rs @@ -21,8 +21,8 @@ use datafusion_expr::Operator; use sqlparser::ast::BinaryOperator; impl SqlToRel<'_, S> { - pub(crate) fn parse_sql_binary_op(&self, op: BinaryOperator) -> Result { - match op { + pub(crate) fn parse_sql_binary_op(&self, op: &BinaryOperator) -> Result { + match *op { BinaryOperator::Gt => Ok(Operator::Gt), BinaryOperator::GtEq => Ok(Operator::GtEq), BinaryOperator::Lt => Ok(Operator::Lt), diff --git a/datafusion/sql/src/expr/mod.rs b/datafusion/sql/src/expr/mod.rs index 715a02db8b02..785c3362c8a9 100644 --- a/datafusion/sql/src/expr/mod.rs +++ b/datafusion/sql/src/expr/mod.rs @@ -140,7 +140,7 @@ impl SqlToRel<'_, S> { let RawBinaryExpr { op, left, right } = binary_expr; Ok(Expr::BinaryExpr(BinaryExpr::new( Box::new(left), - self.parse_sql_binary_op(op)?, + self.parse_sql_binary_op(&op)?, Box::new(right), ))) } @@ -270,7 +270,7 @@ impl SqlToRel<'_, S> { expr, data_type, format, - } => self.sql_cast_to_expr(*expr, data_type, format, schema, planner_context), + } => self.sql_cast_to_expr(*expr, &data_type, format, schema, planner_context), SQLExpr::Cast { kind: CastKind::TryCast | CastKind::SafeCast, @@ -553,7 +553,7 @@ impl SqlToRel<'_, S> { } SQLExpr::Struct { values, fields } => { - self.parse_struct(schema, planner_context, values, fields) + self.parse_struct(schema, planner_context, values, &fields) } SQLExpr::Position { expr, r#in } => { self.sql_position_to_expr(*expr, *r#in, schema, planner_context) @@ -639,7 +639,7 @@ impl SqlToRel<'_, S> { schema: &DFSchema, planner_context: &mut PlannerContext, values: Vec, - fields: Vec, + fields: &[StructField], ) -> Result { if !fields.is_empty() { return not_impl_err!("Struct fields are not supported yet"); @@ -673,7 +673,7 @@ impl SqlToRel<'_, S> { Some(SQLExpr::Identifier(_)) | Some(SQLExpr::Value(_)) | Some(SQLExpr::CompoundIdentifier(_)) => { - self.parse_struct(schema, planner_context, values, vec![]) + self.parse_struct(schema, planner_context, values, &[]) } None => not_impl_err!("Empty tuple not supported yet"), _ => { @@ -979,7 +979,7 @@ impl SqlToRel<'_, S> { fn sql_cast_to_expr( &self, expr: SQLExpr, - data_type: SQLDataType, + data_type: &SQLDataType, format: Option, schema: &DFSchema, planner_context: &mut PlannerContext, @@ -988,7 +988,7 @@ impl SqlToRel<'_, S> { return not_impl_err!("CAST with format is not supported: {format}"); } - let dt = self.convert_data_type_to_field(&data_type)?; + let dt = self.convert_data_type_to_field(data_type)?; let expr = self.sql_expr_to_logical_expr(expr, schema, planner_context)?; // numeric constants are treated as seconds (rather as nanoseconds) diff --git a/datafusion/sql/src/expr/subquery.rs b/datafusion/sql/src/expr/subquery.rs index 24bb813634cc..4bca6f7e49ba 100644 --- a/datafusion/sql/src/expr/subquery.rs +++ b/datafusion/sql/src/expr/subquery.rs @@ -74,7 +74,7 @@ impl SqlToRel<'_, S> { self.validate_single_column( &sub_plan, - spans.clone(), + &spans, "Too many columns! The subquery should only return one column", "Select only one column in the subquery", )?; @@ -116,7 +116,7 @@ impl SqlToRel<'_, S> { self.validate_single_column( &sub_plan, - spans.clone(), + &spans, "Too many columns! The subquery should only return one column", "Select only one column in the subquery", )?; @@ -131,7 +131,7 @@ impl SqlToRel<'_, S> { fn validate_single_column( &self, sub_plan: &LogicalPlan, - spans: Spans, + spans: &Spans, error_message: &str, help_message: &str, ) -> Result<()> { @@ -148,7 +148,7 @@ impl SqlToRel<'_, S> { fn build_multi_column_diagnostic( &self, - spans: Spans, + spans: &Spans, error_message: &str, help_message: &str, ) -> Diagnostic { diff --git a/datafusion/sql/src/lib.rs b/datafusion/sql/src/lib.rs index da15b90d22a8..9f8105e9a85b 100644 --- a/datafusion/sql/src/lib.rs +++ b/datafusion/sql/src/lib.rs @@ -23,6 +23,9 @@ // Make sure fast / cheap clones on Arc are explicit: // https://github.com/apache/datafusion/issues/11143 #![deny(clippy::clone_on_ref_ptr)] +// https://github.com/apache/datafusion/issues/18503 +#![deny(clippy::needless_pass_by_value)] +#![cfg_attr(test, allow(clippy::needless_pass_by_value))] //! This crate provides: //! diff --git a/datafusion/sql/src/parser.rs b/datafusion/sql/src/parser.rs index 99d7467e1b7c..05dd87890763 100644 --- a/datafusion/sql/src/parser.rs +++ b/datafusion/sql/src/parser.rs @@ -456,7 +456,7 @@ impl<'a> DFParser<'a> { break; } if expecting_statement_delimiter { - return self.expected("end of statement", self.parser.peek_token()); + return self.expected("end of statement", &self.parser.peek_token()); } let statement = self.parse_statement()?; @@ -470,7 +470,7 @@ impl<'a> DFParser<'a> { fn expected( &self, expected: &str, - found: TokenWithSpan, + found: &TokenWithSpan, ) -> Result { let sql_parser_span = found.span; let span = Span::try_from_sqlparser_span(sql_parser_span); @@ -488,11 +488,11 @@ impl<'a> DFParser<'a> { fn expect_token( &mut self, expected: &str, - token: Token, + token: &Token, ) -> Result<(), DataFusionError> { let next_token = self.parser.peek_token_ref(); - if next_token.token != token { - self.expected(expected, next_token.clone()) + if next_token.token != *token { + self.expected(expected, next_token) } else { Ok(()) } @@ -553,7 +553,7 @@ impl<'a> DFParser<'a> { /// contains any trailing, unparsed tokens. pub fn parse_into_expr(&mut self) -> Result { let expr = self.parse_expr()?; - self.expect_token("end of expression", Token::EOF)?; + self.expect_token("end of expression", &Token::EOF)?; Ok(expr) } @@ -638,7 +638,7 @@ impl<'a> DFParser<'a> { 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)?; } } } @@ -675,7 +675,7 @@ impl<'a> DFParser<'a> { // Unquoted namespaced keys have to conform to the syntax // "[\.]*". If we have a key that breaks this // pattern, error out: - return self.expected("key name", next_token); + return self.expected("key name", &next_token); } } Ok(parts.join(".")) @@ -683,7 +683,7 @@ impl<'a> DFParser<'a> { Token::SingleQuotedString(s) => Ok(s), Token::DoubleQuotedString(s) => Ok(s), Token::EscapedStringLiteral(s) => Ok(s), - _ => self.expected("key name", next_token), + _ => self.expected("key name", &next_token), } } @@ -702,7 +702,7 @@ impl<'a> DFParser<'a> { Token::DoubleQuotedString(s) => Ok(Value::DoubleQuotedString(s)), Token::EscapedStringLiteral(s) => Ok(Value::EscapedStringLiteral(s)), Token::Number(n, l) => Ok(Value::Number(n, l)), - _ => self.expected("string or numeric value", next_token), + _ => self.expected("string or numeric value", &next_token), } } @@ -732,7 +732,7 @@ impl<'a> DFParser<'a> { Token::Word(w) => Ok(w.value), Token::SingleQuotedString(w) => Ok(w), Token::DoubleQuotedString(w) => Ok(w), - _ => self.expected("an explain format such as TREE", next_token), + _ => self.expected("an explain format such as TREE", &next_token), }?; Ok(Some(format)) } @@ -777,7 +777,7 @@ impl<'a> DFParser<'a> { let identifier = self.parser.parse_identifier()?; partitions.push(identifier.to_string()); } else { - return self.expected("partition name", self.parser.peek_token()); + return self.expected("partition name", &self.parser.peek_token()); } let comma = self.parser.consume_token(&Token::Comma); if self.parser.consume_token(&Token::RParen) { @@ -786,7 +786,7 @@ impl<'a> DFParser<'a> { } else if !comma { return self.expected( "',' or ')' after partition definition", - self.parser.peek_token(), + &self.parser.peek_token(), ); } } @@ -857,7 +857,7 @@ impl<'a> DFParser<'a> { } else { return self.expected( "column name or constraint definition", - self.parser.peek_token(), + &self.parser.peek_token(), ); } let comma = self.parser.consume_token(&Token::Comma); @@ -867,7 +867,7 @@ impl<'a> DFParser<'a> { } else if !comma { return self.expected( "',' or ')' after column definition", - self.parser.peek_token(), + &self.parser.peek_token(), ); } } @@ -887,7 +887,7 @@ impl<'a> DFParser<'a> { } else { return self.expected( "constraint details after CONSTRAINT ", - self.parser.peek_token(), + &self.parser.peek_token(), ); } } else if let Some(option) = self.parser.parse_optional_column_option()? { @@ -1012,7 +1012,7 @@ impl<'a> DFParser<'a> { 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)?; } } } @@ -1051,7 +1051,7 @@ impl<'a> DFParser<'a> { let token = self.parser.next_token(); match &token.token { Token::Word(w) => parse_file_type(&w.value), - _ => self.expected("one of ARROW, PARQUET, NDJSON, or CSV", token), + _ => self.expected("one of ARROW, PARQUET, NDJSON, or CSV", &token), } } @@ -1074,7 +1074,7 @@ impl<'a> DFParser<'a> { } else if !comma { return self.expected( "',' or ')' after option definition", - self.parser.peek_token(), + &self.parser.peek_token(), ); } } diff --git a/datafusion/sql/src/statement.rs b/datafusion/sql/src/statement.rs index 81381bf49fc5..d09923690f86 100644 --- a/datafusion/sql/src/statement.rs +++ b/datafusion/sql/src/statement.rs @@ -1037,7 +1037,7 @@ impl SqlToRel<'_, S> { if limit.is_some() { return not_impl_err!("Update-limit clause not supported")?; } - self.update_to_plan(table, assignments, update_from, selection) + self.update_to_plan(table, &assignments, update_from, selection) } Statement::Delete(Delete { @@ -1070,7 +1070,7 @@ impl SqlToRel<'_, S> { } let table_name = self.get_delete_target(from)?; - self.delete_to_plan(table_name, selection) + self.delete_to_plan(&table_name, selection) } Statement::StartTransaction { @@ -1100,7 +1100,7 @@ impl SqlToRel<'_, S> { if has_end_keyword { return not_impl_err!("Transaction with END keyword not supported"); } - self.validate_transaction_kind(transaction)?; + self.validate_transaction_kind(transaction.as_ref())?; let isolation_level: ast::TransactionIsolationLevel = modes .iter() .filter_map(|m: &TransactionMode| match m { @@ -1903,7 +1903,7 @@ impl SqlToRel<'_, S> { fn delete_to_plan( &self, - table_name: ObjectName, + table_name: &ObjectName, predicate_expr: Option, ) -> Result { // Do a table lookup to verify the table exists @@ -1947,7 +1947,7 @@ impl SqlToRel<'_, S> { fn update_to_plan( &self, table: TableWithJoins, - assignments: Vec, + assignments: &[Assignment], from: Option, predicate_expr: Option, ) -> Result { @@ -2353,7 +2353,7 @@ ON p.function_name = r.routine_name fn validate_transaction_kind( &self, - kind: Option, + kind: Option<&BeginTransactionKind>, ) -> Result<()> { match kind { // BEGIN From 123127f1d996c088ef054bde5c431e979e26ade6 Mon Sep 17 00:00:00 2001 From: foskey51 Date: Sat, 8 Nov 2025 17:08:58 +0000 Subject: [PATCH 2/2] resolve cargo fmt error --- datafusion/sql/src/expr/mod.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/datafusion/sql/src/expr/mod.rs b/datafusion/sql/src/expr/mod.rs index 785c3362c8a9..9725025d599f 100644 --- a/datafusion/sql/src/expr/mod.rs +++ b/datafusion/sql/src/expr/mod.rs @@ -270,7 +270,9 @@ impl SqlToRel<'_, S> { expr, data_type, format, - } => self.sql_cast_to_expr(*expr, &data_type, format, schema, planner_context), + } => { + self.sql_cast_to_expr(*expr, &data_type, format, schema, planner_context) + } SQLExpr::Cast { kind: CastKind::TryCast | CastKind::SafeCast,