From b1be115457e8ec151ae6935cf0c8649d780df7a1 Mon Sep 17 00:00:00 2001 From: jackwener Date: Thu, 17 Nov 2022 09:25:17 +0800 Subject: [PATCH 1/2] fix conflict and UT, cleanup redundant legacy code --- datafusion/expr/src/lib.rs | 2 +- datafusion/expr/src/logical_plan/builder.rs | 28 ++++--------------- datafusion/optimizer/src/filter_push_down.rs | 23 +-------------- .../optimizer/src/propagate_empty_relation.rs | 7 ++--- 4 files changed, 10 insertions(+), 50 deletions(-) diff --git a/datafusion/expr/src/lib.rs b/datafusion/expr/src/lib.rs index 45c281551d186..c8958cb233e74 100644 --- a/datafusion/expr/src/lib.rs +++ b/datafusion/expr/src/lib.rs @@ -67,7 +67,7 @@ pub use function::{ }; pub use literal::{lit, lit_timestamp_nano, Literal, TimestampLiteral}; pub use logical_plan::{ - builder::{build_join_schema, union_with_alias, UNNAMED_TABLE}, + builder::{build_join_schema, union, UNNAMED_TABLE}, Aggregate, CreateCatalog, CreateCatalogSchema, CreateExternalTable, CreateMemoryTable, CreateView, CrossJoin, Distinct, DropTable, DropView, EmptyRelation, Explain, Extension, Filter, Join, JoinConstraint, JoinType, Limit, diff --git a/datafusion/expr/src/logical_plan/builder.rs b/datafusion/expr/src/logical_plan/builder.rs index dedd8d7b8b74f..a3c3e464187d3 100644 --- a/datafusion/expr/src/logical_plan/builder.rs +++ b/datafusion/expr/src/logical_plan/builder.rs @@ -416,19 +416,7 @@ impl LogicalPlanBuilder { /// Apply a union, preserving duplicate rows pub fn union(&self, plan: LogicalPlan) -> Result { - Ok(Self::from(union_with_alias(self.plan.clone(), plan, None)?)) - } - - pub fn union_with_alias( - &self, - plan: LogicalPlan, - alias: Option, - ) -> Result { - Ok(Self::from(union_with_alias( - self.plan.clone(), - plan, - alias, - )?)) + Ok(Self::from(union(self.plan.clone(), plan)?)) } /// Apply a union, removing duplicate rows @@ -445,7 +433,7 @@ impl LogicalPlanBuilder { }; Ok(Self::from(LogicalPlan::Distinct(Distinct { - input: Arc::new(union_with_alias(left_plan, right_plan, None)?), + input: Arc::new(union(left_plan, right_plan)?), }))) } @@ -892,10 +880,9 @@ pub fn project_with_column_index_alias( } /// Union two logical plans with an optional alias. -pub fn union_with_alias( +pub fn union( left_plan: LogicalPlan, right_plan: LogicalPlan, - alias: Option, ) -> Result { let left_col_num = left_plan.schema().fields().len(); @@ -927,7 +914,7 @@ pub fn union_with_alias( })?; Ok(DFField::new( - alias.as_deref(), + None, left_field.name(), data_type, nullable, @@ -962,14 +949,9 @@ pub fn union_with_alias( return Err(DataFusionError::Plan("Empty UNION".to_string())); } - let union_schema = Arc::new(match alias { - Some(ref alias) => union_schema.replace_qualifier(alias.as_str()), - None => union_schema.strip_qualifiers(), - }); - Ok(LogicalPlan::Union(Union { inputs, - schema: union_schema, + schema: Arc::new(union_schema), })) } diff --git a/datafusion/optimizer/src/filter_push_down.rs b/datafusion/optimizer/src/filter_push_down.rs index 7bcd03565909c..cfffaa4321c48 100644 --- a/datafusion/optimizer/src/filter_push_down.rs +++ b/datafusion/optimizer/src/filter_push_down.rs @@ -834,7 +834,7 @@ mod tests { use datafusion_common::DFSchema; use datafusion_expr::{ and, col, in_list, in_subquery, lit, - logical_plan::{builder::union_with_alias, JoinType}, + logical_plan::{JoinType}, sum, Expr, LogicalPlanBuilder, Operator, TableSource, TableType, }; use std::sync::Arc; @@ -1183,27 +1183,6 @@ mod tests { Ok(()) } - #[test] - fn union_all_with_alias() -> Result<()> { - let table_scan = test_table_scan()?; - let union = - union_with_alias(table_scan.clone(), table_scan, Some("t".to_string()))?; - - let plan = LogicalPlanBuilder::from(union) - .filter(col("t.a").eq(lit(1i64)))? - .build()?; - - // filter appears below Union without relation qualifier - let expected = "\ - Union\ - \n Filter: a = Int64(1)\ - \n TableScan: test\ - \n Filter: a = Int64(1)\ - \n TableScan: test"; - assert_optimized_plan_eq(&plan, expected); - Ok(()) - } - #[test] fn union_all_on_projection() -> Result<()> { let table_scan = test_table_scan()?; diff --git a/datafusion/optimizer/src/propagate_empty_relation.rs b/datafusion/optimizer/src/propagate_empty_relation.rs index 59f88cef67161..5d697e07ddb61 100644 --- a/datafusion/optimizer/src/propagate_empty_relation.rs +++ b/datafusion/optimizer/src/propagate_empty_relation.rs @@ -120,14 +120,13 @@ impl OptimizerRule for PropagateEmptyRelation { Ok(LogicalPlan::Projection(Projection::new_from_schema( Arc::new(child), optimized_children_plan.schema().clone(), - union.alias.clone(), + None, ))) } } else { Ok(LogicalPlan::Union(Union { inputs: new_inputs, schema: union.schema.clone(), - alias: union.alias.clone(), })) } } @@ -384,10 +383,10 @@ mod tests { .build()?; let plan = LogicalPlanBuilder::from(left) - .union_with_alias(right, Some("union".to_string()))? + .union(right)? .build()?; - let expected = "Projection: union.a, union.b, union.c, alias=union\ + let expected = "Projection: a, b, c\ \n TableScan: test"; assert_together_optimized_plan_eq(&plan, expected); Ok(()) From 3284fbaa6d97da045c8afd8f967ac8a32dda35cf Mon Sep 17 00:00:00 2001 From: jackwener Date: Thu, 17 Nov 2022 09:35:10 +0800 Subject: [PATCH 2/2] fix fmt --- datafusion/expr/src/logical_plan/builder.rs | 5 +---- datafusion/optimizer/src/filter_push_down.rs | 5 ++--- datafusion/optimizer/src/propagate_empty_relation.rs | 4 +--- 3 files changed, 4 insertions(+), 10 deletions(-) diff --git a/datafusion/expr/src/logical_plan/builder.rs b/datafusion/expr/src/logical_plan/builder.rs index a3c3e464187d3..1bbaf6da94825 100644 --- a/datafusion/expr/src/logical_plan/builder.rs +++ b/datafusion/expr/src/logical_plan/builder.rs @@ -880,10 +880,7 @@ pub fn project_with_column_index_alias( } /// Union two logical plans with an optional alias. -pub fn union( - left_plan: LogicalPlan, - right_plan: LogicalPlan, -) -> Result { +pub fn union(left_plan: LogicalPlan, right_plan: LogicalPlan) -> Result { let left_col_num = left_plan.schema().fields().len(); // the 2 queries should have same number of columns diff --git a/datafusion/optimizer/src/filter_push_down.rs b/datafusion/optimizer/src/filter_push_down.rs index cfffaa4321c48..9ba30fae5d25c 100644 --- a/datafusion/optimizer/src/filter_push_down.rs +++ b/datafusion/optimizer/src/filter_push_down.rs @@ -833,9 +833,8 @@ mod tests { use async_trait::async_trait; use datafusion_common::DFSchema; use datafusion_expr::{ - and, col, in_list, in_subquery, lit, - logical_plan::{JoinType}, - sum, Expr, LogicalPlanBuilder, Operator, TableSource, TableType, + and, col, in_list, in_subquery, lit, logical_plan::JoinType, sum, Expr, + LogicalPlanBuilder, Operator, TableSource, TableType, }; use std::sync::Arc; diff --git a/datafusion/optimizer/src/propagate_empty_relation.rs b/datafusion/optimizer/src/propagate_empty_relation.rs index 5d697e07ddb61..c1a95c8d3001f 100644 --- a/datafusion/optimizer/src/propagate_empty_relation.rs +++ b/datafusion/optimizer/src/propagate_empty_relation.rs @@ -382,9 +382,7 @@ mod tests { .filter(Expr::Literal(ScalarValue::Boolean(Some(false))))? .build()?; - let plan = LogicalPlanBuilder::from(left) - .union(right)? - .build()?; + let plan = LogicalPlanBuilder::from(left).union(right)?.build()?; let expected = "Projection: a, b, c\ \n TableScan: test";