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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
