From 0d67b4844c99c9603b515fec1403a35585992306 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Tue, 4 Mar 2025 17:10:12 -0500 Subject: [PATCH 1/3] Do not double alias Exprs --- datafusion/expr/src/expr.rs | 32 +++++++++++++++++-- .../sqllogictest/test_files/aggregate.slt | 13 ++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs index 3323ea1614fd..7253018f343a 100644 --- a/datafusion/expr/src/expr.rs +++ b/datafusion/expr/src/expr.rs @@ -412,6 +412,19 @@ impl Alias { name: name.into(), } } + + /// Return true if this alias represents the unaliased `name` + pub fn eq_name(&self, name: &str) -> bool { + self.eq_qualified_name(None, name) + } + + pub fn eq_qualified_name( + &self, + relation: Option<&TableReference>, + name: &str, + ) -> bool { + self.relation.as_ref() == relation && self.name == name + } } /// Binary expression @@ -1270,6 +1283,8 @@ impl Expr { /// Ensure `expr` has the name as `original_name` by adding an /// alias if necessary. + /// + // TODO: remove? and deprecate?? pub fn alias_if_changed(self, original_name: String) -> Result { let new_name = self.name_for_alias()?; if new_name == original_name { @@ -1281,7 +1296,12 @@ impl Expr { /// Return `self AS name` alias expression pub fn alias(self, name: impl Into) -> Expr { - Expr::Alias(Alias::new(self, None::<&str>, name.into())) + let name = name.into(); + // don't re-alias if already aliased to the same name + if matches!(&self, Expr::Alias(alias) if alias.eq_name(&name)) { + return self; + } + Expr::Alias(Alias::new(self, None::<&str>, name)) } /// Return `self AS name` alias expression with a specific qualifier @@ -1290,7 +1310,14 @@ impl Expr { relation: Option>, name: impl Into, ) -> Expr { - Expr::Alias(Alias::new(self, relation, name.into())) + let relation = relation.map(|r| r.into()); + let name = name.into(); + // don't re-alias if already aliased to the same name + if matches!(&self, Expr::Alias(alias) if alias.eq_qualified_name(relation.as_ref(), &name)) + { + return self; + } + Expr::Alias(Alias::new(self, relation, name)) } /// Remove an alias from an expression if one exists. @@ -2831,7 +2858,6 @@ pub fn physical_name(expr: &Expr) -> Result { _ => Ok(expr.schema_name().to_string()), } } - #[cfg(test)] mod test { use crate::expr_fn::col; diff --git a/datafusion/sqllogictest/test_files/aggregate.slt b/datafusion/sqllogictest/test_files/aggregate.slt index bc43f6bc8e61..efd31731482a 100644 --- a/datafusion/sqllogictest/test_files/aggregate.slt +++ b/datafusion/sqllogictest/test_files/aggregate.slt @@ -6640,3 +6640,16 @@ SELECT a, median(b), arrow_typeof(median(b)) FROM group_median_all_nulls GROUP B ---- group0 NULL Int32 group1 NULL Int32 + +# should not be a double alias +query TT +explain SELECT count(*) order by count(*); +---- +logical_plan +01)Sort: count(*) ASC NULLS LAST +02)--Projection: count(Int64(1)) AS count(*) +03)----Aggregate: groupBy=[[]], aggr=[[count(Int64(1))]] +04)------EmptyRelation +physical_plan +01)ProjectionExec: expr=[1 as count(*)] +02)--PlaceholderRowExec From 4ce0741b430c1027b049d49554f6f1fbd930f3d2 Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Tue, 4 Mar 2025 17:51:58 -0500 Subject: [PATCH 2/3] DRY --- datafusion/expr/src/expr.rs | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/datafusion/expr/src/expr.rs b/datafusion/expr/src/expr.rs index 7253018f343a..ab0689e5bfd4 100644 --- a/datafusion/expr/src/expr.rs +++ b/datafusion/expr/src/expr.rs @@ -1296,12 +1296,7 @@ impl Expr { /// Return `self AS name` alias expression pub fn alias(self, name: impl Into) -> Expr { - let name = name.into(); - // don't re-alias if already aliased to the same name - if matches!(&self, Expr::Alias(alias) if alias.eq_name(&name)) { - return self; - } - Expr::Alias(Alias::new(self, None::<&str>, name)) + self.alias_qualified(None as Option<&str>, name) } /// Return `self AS name` alias expression with a specific qualifier @@ -1310,14 +1305,14 @@ impl Expr { relation: Option>, name: impl Into, ) -> Expr { - let relation = relation.map(|r| r.into()); - let name = name.into(); - // don't re-alias if already aliased to the same name - if matches!(&self, Expr::Alias(alias) if alias.eq_qualified_name(relation.as_ref(), &name)) - { - return self; - } - Expr::Alias(Alias::new(self, relation, name)) + // Don't nest aliases + let Expr::Alias(mut alias) = self else { + return Expr::Alias(Alias::new(self, relation, name)); + }; + + alias.relation = relation.map(|r| r.into()); + alias.name = name.into(); + Expr::Alias(alias) } /// Remove an alias from an expression if one exists. From 838f2071f0b0cc473b209a1c5798b6d3abde229d Mon Sep 17 00:00:00 2001 From: Andrew Lamb Date: Tue, 4 Mar 2025 18:59:51 -0500 Subject: [PATCH 3/3] update test --- datafusion/optimizer/tests/optimizer_integration.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/datafusion/optimizer/tests/optimizer_integration.rs b/datafusion/optimizer/tests/optimizer_integration.rs index 66bd6b75123e..8c1c25e28267 100644 --- a/datafusion/optimizer/tests/optimizer_integration.rs +++ b/datafusion/optimizer/tests/optimizer_integration.rs @@ -312,7 +312,7 @@ fn eliminate_redundant_null_check_on_count() { GROUP BY col_int32 HAVING c IS NOT NULL"; let plan = test_sql(sql).unwrap(); - let expected = "Projection: test.col_int32, count(Int64(1)) AS count(*) AS c\ + let expected = "Projection: test.col_int32, count(Int64(1)) AS c\ \n Aggregate: groupBy=[[test.col_int32]], aggr=[[count(Int64(1))]]\ \n TableScan: test projection=[col_int32]"; assert_eq!(expected, format!("{plan}"));