Repository navigation
fix: preserve computed names in derived SQL projections - #25521
kumarUjjawal merged 2 commits into
Conversation
Signed-off-by: 1fanwang <[email protected]>
Codecov Report❌ Patch coverage is
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. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
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(_)) |
There was a problem hiding this comment.
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?
Signed-off-by: 1fanwang <[email protected]>
|
Thank you @1fanwang |
## Which issue does this PR close? Related to apache#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 apache@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: ```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]>
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_41without 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.
python3 - "$log"belowno such column: sum_40j1_46j1_id_41python3 - "$log"below[(3,)]Run from this checkout:
Raw logs
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.