This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-25521-4fe342d715ed3078494c772038e1dfe4b31adc98 in repository https://gitbox.apache.org/repos/asf/datafusion.git
commit 2424daeb18d526c30d5351655289af71d6c05844 Author: Stefan Wang <[email protected]> AuthorDate: Tue Sep 22 03:24:13 2026 +0000 fix: preserve computed names in derived SQL projections (#25521) ## Which issue does this PR close? Related to https://github.com/apache/datafusion/issues/21066. ## Rationale for this change An outer query can reference a computed result by its logical output name while the generated inner query leaves that result unnamed. The database cannot resolve the outer reference. For example, BigQuery output refers to `sum_40j1_46j1_id_41` without defining it in the derived table. ## What changes are included in this PR? Derived projections now emit matching output names whether or not the table has an alias. The unparser waits until the final table alias is known, so a table-column alias list still supplies the names. Explicit expression aliases and plain column references keep their existing SQL. ## What is the testing strategy for this PR? The regression exercises PostgreSQL and BigQuery output through the unparser. Existing column-alias and UNNEST snapshots remain unchanged. Roundtrip tests still compare the whole logical plan after removing only unqualified, metadata-free column self-aliases. ### Testing Done SQLite 3.51.0 executes the emitted BigQuery SQL without rewriting it. This uses local SQLite, not the Google BigQuery service. The pre-fix query below was captured from https://github.com/apache/datafusion/commit/4abdeef1e03e7d37be7a0dd1683985e9e3c8a7d1; the fixed query comes directly from the regression run. | # | Scenario | Command | Result | |---|---|---|---| | 1 | Recorded pre-fix SQL | `python3 - "$log"` below | `no such column: sum_40j1_46j1_id_41` | | 2 | Freshly emitted fixed SQL | `python3 - "$log"` below | `[(3,)]` | Run from this checkout: ```bash set -euo pipefail log=$(mktemp) CARGO_BUILD_JOBS=2 cargo test --locked --profile ci \ -p datafusion -p datafusion-sql --features datafusion-sql/recursive_protection \ --test sql_integration unparse_preserves_derived_aggregate_output_name \ -- --nocapture > "$log" 2>&1 cat "$log" python3 - "$log" <<'PY' import sqlite3 import sys from pathlib import Path before = ( "SELECT `sum_40j1_46j1_id_41` FROM " "(SELECT sum(`j1`.`j1_id`) AS `visible`, sum(`j1`.`j1_id`) FROM `j1`)" ) emitted: list[str] = [ line.removeprefix("BIGQUERY_SQL=") for line in Path(sys.argv[1]).read_text().splitlines() if line.startswith("BIGQUERY_SQL=") ] assert len(emitted) == 1, emitted print(f"SQLite version: {sqlite3.sqlite_version}") with sqlite3.connect(database=":memory:") as connection: connection.executescript( "CREATE TABLE j1 (j1_id INTEGER); INSERT INTO j1 VALUES (1), (2);" ) print(f"before SQL: {before}") try: connection.execute(before).fetchall() except sqlite3.OperationalError as error: assert str(error) == "no such column: sum_40j1_46j1_id_41", error print(f"before error: {error}") else: raise AssertionError("The recorded pre-fix query unexpectedly succeeded.") print(f"after SQL: {emitted[0]}") rows = connection.execute(emitted[0]).fetchall() print(f"after rows: {rows}") assert rows == [(3,)], rows PY ``` <details> <summary>Raw logs</summary> ```text SQLite version: 3.51.0 before SQL: SELECT `sum_40j1_46j1_id_41` FROM (SELECT sum(`j1`.`j1_id`) AS `visible`, sum(`j1`.`j1_id`) FROM `j1`) before error: no such column: sum_40j1_46j1_id_41 after SQL: SELECT `sum_40j1_46j1_id_41` FROM (SELECT sum(`j1`.`j1_id`) AS `visible`, sum(`j1`.`j1_id`) AS `sum_40j1_46j1_id_41` FROM `j1`) after rows: [(3,)] ``` </details> ## Are there any user-facing changes? Generated derived projections expose the computed column names used by outer queries, including when the dialect does not require a table alias. SQL input validation and public APIs are unchanged. --------- Signed-off-by: 1fanwang <[email protected]> --- datafusion/sql/src/unparser/ast.rs | 45 ++++++++++++++++++++---- datafusion/sql/src/unparser/plan.rs | 30 ++++++++++------ datafusion/sql/tests/cases/plan_to_sql.rs | 57 ++++++++++++++++++++++++++++++- 3 files changed, 115 insertions(+), 17 deletions(-) diff --git a/datafusion/sql/src/unparser/ast.rs b/datafusion/sql/src/unparser/ast.rs index c335d3ee4c..38b51afe3e 100644 --- a/datafusion/sql/src/unparser/ast.rs +++ b/datafusion/sql/src/unparser/ast.rs @@ -677,6 +677,7 @@ pub struct DerivedRelationBuilder { lateral: Option<bool>, subquery: Option<Box<ast::Query>>, alias: Option<ast::TableAlias>, + projection_names: Vec<ast::Ident>, } impl DerivedRelationBuilder { @@ -692,18 +693,49 @@ impl DerivedRelationBuilder { self.alias = value; self } + pub(super) fn projection_names(&mut self, value: Vec<ast::Ident>) -> &mut Self { + self.projection_names = value; + self + } fn build(&self) -> Result<ast::TableFactor, BuilderError> { + let mut subquery = match self.subquery { + Some(ref value) => value.clone(), + None => { + return Err(Into::into(UninitializedFieldError::from("subquery"))); + } + }; + if self + .alias + .as_ref() + .is_none_or(|alias| alias.columns.is_empty()) + && let ast::SetExpr::Select(select) = subquery.body.as_mut() + && select.projection.len() == self.projection_names.len() + { + for (item, alias) in select.projection.iter_mut().zip(&self.projection_names) + { + if let ast::SelectItem::UnnamedExpr(expr) = item { + let preserves_name = match expr { + ast::Expr::Identifier(name) => name.value == alias.value, + ast::Expr::CompoundIdentifier(names) => { + names.last().is_some_and(|name| name.value == alias.value) + } + _ => false, + }; + if !preserves_name { + *item = ast::SelectItem::ExprWithAlias { + expr: expr.clone(), + alias: alias.clone(), + }; + } + } + } + } Ok(ast::TableFactor::Derived { lateral: match self.lateral { Some(ref value) => *value, None => return Err(Into::into(UninitializedFieldError::from("lateral"))), }, - subquery: match self.subquery { - Some(ref value) => value.clone(), - None => { - return Err(Into::into(UninitializedFieldError::from("subquery"))); - } - }, + subquery, alias: self.alias.clone(), sample: None, }) @@ -713,6 +745,7 @@ impl DerivedRelationBuilder { lateral: Default::default(), subquery: Default::default(), alias: Default::default(), + projection_names: Default::default(), } } } diff --git a/datafusion/sql/src/unparser/plan.rs b/datafusion/sql/src/unparser/plan.rs index 69babae4e0..70a802629c 100644 --- a/datafusion/sql/src/unparser/plan.rs +++ b/datafusion/sql/src/unparser/plan.rs @@ -593,7 +593,18 @@ impl Unparser<'_> { alias: Option<ast::TableAlias>, lateral: bool, ) -> Result<()> { + let preserve_names = matches!(plan, LogicalPlan::Projection(_)) + && alias.as_ref().is_none_or(|alias| alias.columns.is_empty()); let mut derived_builder = DerivedRelationBuilder::default(); + if preserve_names { + derived_builder.projection_names( + plan.schema() + .fields() + .iter() + .map(|field| self.column_alias_to_sql(field.name())) + .collect::<Result<Vec<_>>>()?, + ); + } derived_builder.lateral(lateral).alias(alias).subquery({ let inner_statement = self.plan_to_sql(plan)?; if let ast::Statement::Query(inner_query) = inner_statement { @@ -2693,18 +2704,9 @@ impl Unparser<'_> { Expr::Alias(Alias { expr, name, .. }) => { let inner = self.expr_to_sql(expr)?; - // Determine the alias name to use - let col_name = if let Some(rewritten_name) = - self.dialect.col_alias_overrides(name)? - { - rewritten_name.to_string() - } else { - name.to_string() - }; - Ok(ast::SelectItem::ExprWithAlias { expr: inner, - alias: self.new_ident_quoted_if_needs(col_name), + alias: self.column_alias_to_sql(name)?, }) } _ => { @@ -2715,6 +2717,14 @@ impl Unparser<'_> { } } + fn column_alias_to_sql(&self, name: &str) -> Result<Ident> { + let name = self + .dialect + .col_alias_overrides(name)? + .unwrap_or_else(|| name.to_string()); + Ok(self.new_ident_quoted_if_needs(name)) + } + fn sorts_to_sql(&self, sort_exprs: &[SortExpr]) -> Result<OrderByKind> { Ok(OrderByKind::Expressions( sort_exprs diff --git a/datafusion/sql/tests/cases/plan_to_sql.rs b/datafusion/sql/tests/cases/plan_to_sql.rs index 191425416c..de22321327 100644 --- a/datafusion/sql/tests/cases/plan_to_sql.rs +++ b/datafusion/sql/tests/cases/plan_to_sql.rs @@ -17,6 +17,7 @@ use arrow::datatypes::{DataType, Field, Schema}; +use datafusion_common::tree_node::{Transformed, TransformedResult}; use datafusion_common::{ Column, DFSchema, DFSchemaRef, DataFusionError, Result, TableReference, assert_contains, @@ -111,6 +112,25 @@ fn roundtrip_expr(table: TableReference, sql: &str) -> Result<String> { Ok(ast.to_string()) } +fn remove_column_self_aliases(plan: LogicalPlan) -> Result<LogicalPlan> { + plan.transform_up_with_subqueries(|plan| { + plan.map_expressions(|expr| { + if let Expr::Alias(alias) = &expr + && alias.relation.is_none() + && alias.metadata.is_none() + && let Expr::Column(column) = alias.expr.as_ref() + && column.relation.is_none() + && column.name == alias.name + { + Ok(Transformed::yes(*alias.expr.clone())) + } else { + Ok(Transformed::no(expr)) + } + }) + }) + .data() +} + #[test] fn roundtrip_statement() -> Result<()> { let tests: Vec<&str> = vec![ @@ -254,7 +274,11 @@ fn roundtrip_statement() -> Result<()> { .sql_statement_to_plan(roundtrip_statement.clone()) .unwrap(); - assert_eq!(plan, plan_roundtrip); + // Explicit output names can add unqualified self-aliases without changing the plan's meaning. + assert_eq!( + remove_column_self_aliases(plan)?, + remove_column_self_aliases(plan_roundtrip)?, + ); } Ok(()) @@ -392,6 +416,37 @@ fn roundtrip_statement_with_dialect_4() -> Result<(), DataFusionError> { Ok(()) } +#[test] +fn unparse_preserves_derived_aggregate_output_name() -> Result<()> { + let schema = Schema::new(vec![Field::new("j1_id", DataType::Int32, false)]); + let aggregate = sum(col("j1.j1_id")); + let output = Expr::Column(Column::from_name(aggregate.schema_name().to_string())); + let plan = table_scan(Some("j1"), &schema, None)? + .aggregate(Vec::<Expr>::new(), vec![aggregate])? + .project(vec![output.clone().alias("visible"), output.clone()])? + .project(vec![output])? + .build()?; + + let sql = Unparser::new(&UnparserPostgreSqlDialect {}) + .plan_to_sql(&plan)? + .to_string(); + println!("UNPARSED_SQL={sql}"); + assert_snapshot!( + sql, + @r#"SELECT "sum(j1.j1_id)" FROM (SELECT sum("j1"."j1_id") AS "visible", sum("j1"."j1_id") AS "sum(j1.j1_id)" FROM "j1") AS "derived_projection""# + ); + + let sql = Unparser::new(&BigQueryDialect {}) + .plan_to_sql(&plan)? + .to_string(); + println!("BIGQUERY_SQL={sql}"); + assert_snapshot!( + sql, + @r#"SELECT `sum_40j1_46j1_id_41` FROM (SELECT sum(`j1`.`j1_id`) AS `visible`, sum(`j1`.`j1_id`) AS `sum_40j1_46j1_id_41` FROM `j1`)"# + ); + Ok(()) +} + #[test] fn roundtrip_rebases_derived_projection_references() -> Result<(), DataFusionError> { roundtrip_statement_with_dialect_helper!( --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
