From 1e8d072234ba14bb693d7016f88b34a139268dac Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Fri, 13 Dec 2024 20:42:23 -0500 Subject: [PATCH] Box wildcard options --- datafusion/expr/src/expr.rs | 4 +--- datafusion/expr/src/expr_fn.rs | 8 ++++---- .../src/analyzer/inline_table_scan.rs | 8 ++------ .../proto/src/logical_plan/from_proto.rs | 2 +- .../tests/cases/roundtrip_logical_plan.rs | 12 +++--------- datafusion/sql/src/expr/function.rs | 19 +++++-------------- datafusion/sql/src/expr/mod.rs | 4 ++-- datafusion/sql/src/unparser/expr.rs | 12 +++--------- 8 files changed, 21 insertions(+), 48 deletions(-) diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs index d0e98327f34b..414b5e2b5969 100644 --- a/datafusion/expr/src/expr.rs +++ b/datafusion/expr/src/expr.rs @@ -222,8 +222,6 @@ use sqlparser::ast::{ /// // to 42 = 5 AND b = 6 /// assert_eq!(rewritten.data, lit(42).eq(lit(5)).and(col("b").eq(lit(6)))); #[derive(Clone, PartialEq, Eq, PartialOrd, Hash, Debug)] -// TODO make the enum smaller with more boxing (looks like Wildcard is now bigger) -#[allow(clippy::large_enum_variant)] pub enum Expr { /// An expression with a specific name. Alias(Alias), @@ -315,7 +313,7 @@ pub enum Expr { /// plan into physical plan. Wildcard { qualifier: Option, - options: WildcardOptions, + options: Box, }, /// List of grouping set expressions. Only valid in the context of an aggregate /// GROUP BY expression list diff --git a/datafusion/expr/src/expr_fn.rs b/datafusion/expr/src/expr_fn.rs index a44dd24039dc..a2de5e7b259f 100644 --- a/datafusion/expr/src/expr_fn.rs +++ b/datafusion/expr/src/expr_fn.rs @@ -123,7 +123,7 @@ pub fn placeholder(id: impl Into) -> Expr { pub fn wildcard() -> Expr { Expr::Wildcard { qualifier: None, - options: WildcardOptions::default(), + options: Box::new(WildcardOptions::default()), } } @@ -131,7 +131,7 @@ pub fn wildcard() -> Expr { pub fn wildcard_with_options(options: WildcardOptions) -> Expr { Expr::Wildcard { qualifier: None, - options, + options: Box::new(options), } } @@ -148,7 +148,7 @@ pub fn wildcard_with_options(options: WildcardOptions) -> Expr { pub fn qualified_wildcard(qualifier: impl Into) -> Expr { Expr::Wildcard { qualifier: Some(qualifier.into()), - options: WildcardOptions::default(), + options: Box::new(WildcardOptions::default()), } } @@ -159,7 +159,7 @@ pub fn qualified_wildcard_with_options( ) -> Expr { Expr::Wildcard { qualifier: Some(qualifier.into()), - options, + options: Box::new(options), } } diff --git a/datafusion/optimizer/src/analyzer/inline_table_scan.rs b/datafusion/optimizer/src/analyzer/inline_table_scan.rs index 342d85a915b4..95781b395f3c 100644 --- a/datafusion/optimizer/src/analyzer/inline_table_scan.rs +++ b/datafusion/optimizer/src/analyzer/inline_table_scan.rs @@ -23,8 +23,7 @@ use crate::analyzer::AnalyzerRule; use datafusion_common::config::ConfigOptions; use datafusion_common::tree_node::{Transformed, TransformedResult, TreeNode}; use datafusion_common::{Column, Result}; -use datafusion_expr::expr::WildcardOptions; -use datafusion_expr::{logical_plan::LogicalPlan, Expr, LogicalPlanBuilder}; +use datafusion_expr::{logical_plan::LogicalPlan, wildcard, Expr, LogicalPlanBuilder}; /// Analyzed rule that inlines TableScan that provide a [`LogicalPlan`] /// (DataFrame / ViewTable) @@ -93,10 +92,7 @@ fn generate_projection_expr( ))); } } else { - exprs.push(Expr::Wildcard { - qualifier: None, - options: WildcardOptions::default(), - }); + exprs.push(wildcard()); } Ok(exprs) } diff --git a/datafusion/proto/src/logical_plan/from_proto.rs b/datafusion/proto/src/logical_plan/from_proto.rs index 301efc42a7c4..6ab3e0c9096c 100644 --- a/datafusion/proto/src/logical_plan/from_proto.rs +++ b/datafusion/proto/src/logical_plan/from_proto.rs @@ -513,7 +513,7 @@ pub fn parse_expr( let qualifier = qualifier.to_owned().map(|x| x.try_into()).transpose()?; Ok(Expr::Wildcard { qualifier, - options: WildcardOptions::default(), + options: Box::new(WildcardOptions::default()), }) } ExprType::ScalarUdfExpr(protobuf::ScalarUdfExprNode { diff --git a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs index f793e96f612b..c0885ece08bc 100644 --- a/datafusion/proto/tests/cases/roundtrip_logical_plan.rs +++ b/datafusion/proto/tests/cases/roundtrip_logical_plan.rs @@ -67,7 +67,7 @@ use datafusion_common::{ use datafusion_expr::dml::CopyTo; use datafusion_expr::expr::{ self, Between, BinaryExpr, Case, Cast, GroupingSet, InList, Like, ScalarFunction, - Unnest, WildcardOptions, + Unnest, }; use datafusion_expr::logical_plan::{Extension, UserDefinedLogicalNodeCore}; use datafusion_expr::{ @@ -2061,10 +2061,7 @@ fn roundtrip_unnest() { #[test] fn roundtrip_wildcard() { - let test_expr = Expr::Wildcard { - qualifier: None, - options: WildcardOptions::default(), - }; + let test_expr = wildcard(); let ctx = SessionContext::new(); roundtrip_expr_test(test_expr, ctx); @@ -2072,10 +2069,7 @@ fn roundtrip_wildcard() { #[test] fn roundtrip_qualified_wildcard() { - let test_expr = Expr::Wildcard { - qualifier: Some("foo".into()), - options: WildcardOptions::default(), - }; + let test_expr = qualified_wildcard("foo"); let ctx = SessionContext::new(); roundtrip_expr_test(test_expr, ctx); diff --git a/datafusion/sql/src/expr/function.rs b/datafusion/sql/src/expr/function.rs index 15b415e3dc42..da1a4ba81f5a 100644 --- a/datafusion/sql/src/expr/function.rs +++ b/datafusion/sql/src/expr/function.rs @@ -22,11 +22,11 @@ use datafusion_common::{ internal_datafusion_err, internal_err, not_impl_err, plan_datafusion_err, plan_err, DFSchema, Dependency, Result, }; -use datafusion_expr::expr::WildcardOptions; use datafusion_expr::expr::{ScalarFunction, Unnest}; use datafusion_expr::planner::PlannerResult; use datafusion_expr::{ - expr, Expr, ExprFunctionExt, ExprSchemable, WindowFrame, WindowFunctionDefinition, + expr, qualified_wildcard, wildcard, Expr, ExprFunctionExt, ExprSchemable, + WindowFrame, WindowFunctionDefinition, }; use sqlparser::ast::{ DuplicateTreatment, Expr as SQLExpr, Function as SQLFunction, FunctionArg, @@ -418,17 +418,11 @@ impl SqlToRel<'_, S> { name: _, arg: FunctionArgExpr::Wildcard, operator: _, - } => Ok(Expr::Wildcard { - qualifier: None, - options: WildcardOptions::default(), - }), + } => Ok(wildcard()), FunctionArg::Unnamed(FunctionArgExpr::Expr(arg)) => { self.sql_expr_to_logical_expr(arg, schema, planner_context) } - FunctionArg::Unnamed(FunctionArgExpr::Wildcard) => Ok(Expr::Wildcard { - qualifier: None, - options: WildcardOptions::default(), - }), + FunctionArg::Unnamed(FunctionArgExpr::Wildcard) => Ok(wildcard()), FunctionArg::Unnamed(FunctionArgExpr::QualifiedWildcard(object_name)) => { let qualifier = self.object_name_to_table_reference(object_name)?; // Sanity check on qualifier with schema @@ -436,10 +430,7 @@ impl SqlToRel<'_, S> { if qualified_indices.is_empty() { return plan_err!("Invalid qualifier {qualifier}"); } - Ok(Expr::Wildcard { - qualifier: Some(qualifier), - options: WildcardOptions::default(), - }) + Ok(qualified_wildcard(qualifier)) } _ => not_impl_err!("Unsupported qualified wildcard argument: {sql:?}"), } diff --git a/datafusion/sql/src/expr/mod.rs b/datafusion/sql/src/expr/mod.rs index dce123f08ac3..a651d8fa5d35 100644 --- a/datafusion/sql/src/expr/mod.rs +++ b/datafusion/sql/src/expr/mod.rs @@ -595,11 +595,11 @@ impl SqlToRel<'_, S> { } SQLExpr::Wildcard(_token) => Ok(Expr::Wildcard { qualifier: None, - options: WildcardOptions::default(), + options: Box::new(WildcardOptions::default()), }), SQLExpr::QualifiedWildcard(object_name, _token) => Ok(Expr::Wildcard { qualifier: Some(self.object_name_to_table_reference(object_name)?), - options: WildcardOptions::default(), + options: Box::new(WildcardOptions::default()), }), SQLExpr::Tuple(values) => self.parse_tuple(schema, planner_context, values), _ => not_impl_err!("Unsupported ast node in sqltorel: {sql:?}"), diff --git a/datafusion/sql/src/unparser/expr.rs b/datafusion/sql/src/unparser/expr.rs index 06d5d1a207bb..f09de133b571 100644 --- a/datafusion/sql/src/unparser/expr.rs +++ b/datafusion/sql/src/unparser/expr.rs @@ -1688,7 +1688,7 @@ mod tests { let dummy_logical_plan = table_scan(Some("t"), &dummy_schema, None)? .project(vec![Expr::Wildcard { qualifier: None, - options: WildcardOptions::default(), + options: Box::new(WildcardOptions::default()), }])? .filter(col("a").eq(lit(1)))? .build()?; @@ -1880,10 +1880,7 @@ mod tests { (sum(col("a")), r#"sum(a)"#), ( count_udaf() - .call(vec![Expr::Wildcard { - qualifier: None, - options: WildcardOptions::default(), - }]) + .call(vec![wildcard()]) .distinct() .build() .unwrap(), @@ -1891,10 +1888,7 @@ mod tests { ), ( count_udaf() - .call(vec![Expr::Wildcard { - qualifier: None, - options: WildcardOptions::default(), - }]) + .call(vec![wildcard()]) .filter(lit(true)) .build() .unwrap(),