You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "goutamadwant (via GitHub)" <gi...@apache.org> on 2026/06/17 16:13:33 UTC
[PR] Fix DuckDB unparse for optimized join projections [datafusion]
goutamadwant opened a new pull request, #23002:
URL: https://github.com/apache/datafusion/pull/23002
Title: Fix DuckDB unparse for optimized join projections
## Which issue does this PR close?
- Closes #22961.
## Rationale for this change
Optimized plans can introduce intermediate projection aliases from common subexpression elimination and can also leave join conditions referencing aliases from a flattened join input.
The SQL unparser was rebasing some unqualified optimizer aliases to a table alias, producing invalid references such as `"o"."__common_expr_1"`. It could also emit a derived projection around a join input while the outer join condition still referenced aliases inside that derived table, producing SQL that DuckDB rejects.
## What changes are included in this PR?
- Make `TableAliasRewriter` use `DFSchema` qualifier information instead of only Arrow field names, with explicit control over whether unqualified fields should be rebound to a table alias.
- Keep existing alias rebasing for table-scan filters and window-derived inputs.
- Avoid wrapping qualified pass-through join projections when they are used as the left input of another already projected join, so referenced aliases remain in scope.
- Add a regression test for the optimized DuckDB unparse path from the issue.
## Are these changes tested?
- `cargo fmt --all`
- `cargo test -p datafusion --test core_integration optimized_duckdb_unparse_preserves_derived_table_scope`
- `cargo test -p datafusion-sql --test sql_integration`
- `cargo test -p datafusion-sql unparser::rewrite`
- `cargo clippy -p datafusion-sql --test sql_integration -- -D warnings`
- `cargo clippy -p datafusion --test core_integration -- -D warnings`
## Are there any user-facing changes?
No public API changes. This fixes SQL generated by the unparser for optimized logical plans.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix DuckDB unparse for optimized join projections [datafusion]
Posted by "drewrip (via GitHub)" <gi...@apache.org>.
drewrip commented on PR #23002:
URL: https://github.com/apache/datafusion/pull/23002#issuecomment-4750822749
Thanks for the fix @goutamadwant ! This will be a big help.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix DuckDB unparse for optimized join projections [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #23002:
URL: https://github.com/apache/datafusion/pull/23002#issuecomment-4750300589
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion v54.0.0 (current)
Built [ 107.165s] (current)
Parsing datafusion v54.0.0 (current)
Parsed [ 0.036s] (current)
Building datafusion v54.0.0 (baseline)
Built [ 102.945s] (baseline)
Parsing datafusion v54.0.0 (baseline)
Parsed [ 0.035s] (baseline)
Checking datafusion v54.0.0 -> v54.0.0 (no change; assume patch)
Checked [ 0.617s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 212.305s] datafusion
Building datafusion-sql v54.0.0 (current)
Built [ 43.119s] (current)
Parsing datafusion-sql v54.0.0 (current)
Parsed [ 0.032s] (current)
Building datafusion-sql v54.0.0 (baseline)
Built [ 43.850s] (baseline)
Parsing datafusion-sql v54.0.0 (baseline)
Parsed [ 0.034s] (baseline)
Checking datafusion-sql v54.0.0 -> v54.0.0 (no change; assume patch)
Checked [ 0.221s] 223 checks: 222 pass, 1 fail, 0 warn, 30 skip
--- failure inherent_method_missing: pub method removed or renamed ---
Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.48.0/src/lints/inherent_method_missing.ron
Failed in:
SqlToRel::take_warnings, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/8220a571fb972ef2c38273c625de9715271a1a57/datafusion/sql/src/planner.rs:493
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 88.248s] datafusion-sql
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix DuckDB unparse for optimized join projections [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #23002:
URL: https://github.com/apache/datafusion/pull/23002#discussion_r3434809669
##########
datafusion/sql/src/unparser/plan.rs:
##########
Review Comment:
Nice catch on the derived-join-input case for the left side. I think the same invariant applies symmetrically to the right side as well.
Right now `unwrap_qualified_passthrough_join_projection` is only applied to `left_plan`. If the optimized plan contains a qualified passthrough `Projection(Join)` on the right input while `already_projected` is true, we'll still recurse into that projection, emit it as a derived table, and then `join_constraint_to_sql` will use `join.on` columns that refer to aliases defined inside the derived table.
That seems to reintroduce the invalid-scope SQL this PR is fixing, but for right-deep or nested-right join shapes. Could we apply `unwrap_qualified_passthrough_join_projection` to `right_plan` as well before building `right_relation` and add a regression test covering a nested join on the right side?
##########
datafusion/sql/src/unparser/rewrite.rs:
##########
@@ -524,12 +527,28 @@ impl TreeNodeRewriter for TableAliasRewriter<'_> {
fn f_down(&mut self, expr: Expr) -> Result<Transformed<Expr>> {
match expr {
Expr::Column(column) => {
- if let Ok(field) = self.table_schema.field_with_name(&column.name) {
- let new_column =
- Column::new(Some(self.alias_name.clone()), field.name().clone());
- Ok(Transformed::yes(Expr::Column(new_column)))
- } else {
- Ok(Transformed::no(Expr::Column(column)))
+ match self
+ .table_schema
+ .qualified_field_from_column(&column)
+ .or_else(|_| {
+ self.table_schema
+ .qualified_field_with_unqualified_name(&column.name)
+ }) {
+ Ok((Some(_), field)) => {
Review Comment:
Small cleanup suggestion: both rewrite arms construct the same replacement column.
It might read a bit more clearly if the match made the rewrite condition explicit, something along the lines of `Ok((qualifier, field)) if qualifier.is_some() || self.rewrite_unqualified`, and then performed the rewrite once. The remaining `Ok((None, _)) | Err(_)` arm could stay as the no-op path. That would remove the duplicated `Column::new(...)` construction.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix DuckDB unparse for optimized join projections [datafusion]
Posted by "goutamadwant (via GitHub)" <gi...@apache.org>.
goutamadwant commented on PR #23002:
URL: https://github.com/apache/datafusion/pull/23002#issuecomment-4775978962
Agree @drewrip . We can keep this PR scoped to the nested join alias-scope fix, and a second follow-up for the remaining CSE-enabled case might help.
Let me know. thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix DuckDB unparse for optimized join projections [datafusion]
Posted by "goutamadwant (via GitHub)" <gi...@apache.org>.
goutamadwant commented on PR #23002:
URL: https://github.com/apache/datafusion/pull/23002#issuecomment-4743105227
Thanks for the review @kosiew I pushed an update addressing both comments.
For the symmetric right-side case, the unparser now emits a qualified passthrough `Projection(Join)` as a parenthesized nested join relation instead of a derived projection. That keeps the inner join aliases visible to the outer join condition without reintroducing the invalid alias scope.
I also added a regression test for the right-side nested join case and simplified `TableAliasRewriter` so the rewrite condition is expressed once and the replacement column is constructed once.
Ran locally all the tests and they look fine. Let me know if you any suggestions. thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix DuckDB unparse for optimized join projections [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #23002:
URL: https://github.com/apache/datafusion/pull/23002#issuecomment-4757251104
Merging with `main` to get #23053
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix DuckDB unparse for optimized join projections [datafusion]
Posted by "drewrip (via GitHub)" <gi...@apache.org>.
drewrip commented on PR #23002:
URL: https://github.com/apache/datafusion/pull/23002#issuecomment-4772576496
@goutamadwant @kosiew It would be great to get this merged, but this won't fully resolve #22961 . The issue describes two different errors that appear, one when the CSE pass is enabled, and one when it is manually disabled. I tried the reproducer with this PR and it fixes the error that appears when the CSE pass is disabled. So, we can now generate correct SQL but you have to disable the CSE pass first. Since this PR is already ready to go, maybe we can solve the second part in a follow up?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix DuckDB unparse for optimized join projections [datafusion]
Posted by "goutamadwant (via GitHub)" <gi...@apache.org>.
goutamadwant commented on PR #23002:
URL: https://github.com/apache/datafusion/pull/23002#issuecomment-4752393284
@drewrip I checked the current CI failure. The semver check is passing now; the only red job is `cargo test hash collisions (amd64)`, failing in `datafusion/common/src/hash_utils.rs` under `hash_utils::tests::test_create_hashes_with_quality_hash_state`.
This PR only touches the SQL unparser/planner test path, so this looks unrelated to the DuckDB unparse change. I have refreshed my branch with main and might trigger the CI again. Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Fix DuckDB unparse for optimized join projections [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #23002:
URL: https://github.com/apache/datafusion/pull/23002
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org