Skip to content

fix: preserve computed names in derived SQL projections - #25521

Open
1fanwang wants to merge 2 commits into
apache:mainfrom
1fanwang:1fannnw/preserve-derived-expression-names
Open

1fanwang wants to merge 2 commits into
apache:mainfrom
1fanwang:1fannnw/preserve-derived-expression-names

Conversation

@1fanwang

@1fanwang 1fanwang commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Related to #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 4abdeef; 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:

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
Raw logs
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,)]

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 <1fannnw@gmail.com>
@github-actions github-actions Bot added the sql SQL Planner label Sep 20, 2026
@codecov-commenter

codecov-commenter commented Sep 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.42%. Comparing base (925d7f8) to head (91ca248).
⚠️ Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/sql/src/unparser/plan.rs 83.33% 0 Missing and 3 partials ⚠️
datafusion/sql/src/unparser/ast.rs 96.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #25521      +/-   ##
==========================================
+ Coverage   82.38%   82.42%   +0.03%     
==========================================
  Files        1138     1138              
  Lines      434309   435443    +1134     
  Branches   434309   435443    +1134     
==========================================
+ Hits       357803   358893    +1090     
+ Misses      54875    54845      -30     
- Partials    21631    21705      +74     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kumarUjjawal kumarUjjawal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @1fanwang for picking this issue. I left one comment please take a look.

alias: Option<ast::TableAlias>,
lateral: bool,
) -> Result<()> {
let preserve_names = matches!(plan, LogicalPlan::Projection(_))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Output-name preservation should not depend on the derived table having a table alias. BigQueryDialect does not require one, so derive_with_dialect_alias passes None and this condition remains false. The inner computed expression is then anonymous while the outer projection references its encoded logical name, producing invalid GoogleSQL. Could we preserve names whenever no table-column alias list already provides them, and add a BigQuery or alias-less regression test?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 91ca248.

Signed-off-by: 1fanwang <1fannnw@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sql SQL Planner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants