fix: Consider column names' case when aliasing tables#22917
Conversation
github-actions
Bot
added
sqllogictest
github-actions
Bot
added
sql
There was a problem hiding this comment.
@nuno-faria
Thanks for working on this. I think there is still one blocking case around quoted identifiers with separators.
| LogicalPlanBuilder::from(plan) | ||
| .project(fields.iter().zip(idents).map(|(field, ident)| { | ||
| col(field.name()).alias(self.ident_normalizer.normalize(ident)) | ||
| Expr::Column(Column::from_qualified_name_ignore_case(field.name())) |
There was a problem hiding this comment.
apply_expr_alias still rebuilds a column reference by reparsing field.name() as SQL text. That helps with the simple case-sensitive name case, but quoted identifiers can legally contain separators like ..
For example, CREATE TABLE t ("A.B" int); SELECT * FROM (SELECT * FROM t) t_(x); is still resolved as a qualified column A.B instead of the existing field named A.B, so aliasing still errors.
I think the important invariant here is positional: each existing output field should be aliased while preserving its already resolved schema identity. Could we build Expr::Column from plan.schema().columns() or qualified_field instead of parsing the display name string? One way to do that would be to collect the schema columns before moving plan, then zip those with idents.
There was a problem hiding this comment.
Thanks @kosiew, it's better to alias the existing columns directly. I also updated the test to include a column with a dot.
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
@nuno-faria
Thanks for the iteration.
Looks 👍 to me
## Which issue does this PR close? Follow up to apache#22917, I overlooked these: - Add SLT regression coverage for quoted dotted names / column-list aliasing. - remove redundant .clone() after columns() since it already returns Vec<Column>. ## What changes are included in this PR? This PR adds SLT regression coverage for quoted dotted column names used with table column-list aliasing, including qualified and unqualified references. It also removes a redundant `.clone()` call after `plan.schema().columns()`. ## Are these changes tested? Yes. Tests are added in: `datafusion/sqllogictest/test_files/alias.slt` ## Are there any user-facing changes? No user-facing changes. ## LLM-generated code disclosure This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.
2 participants
Which issue does this PR close?
Rationale for this change
Avoid errors when aliasing tables/subqueries with case sensitive column names.
What changes are included in this PR?
apply_expr_aliasso it aliases the original columns directly.Are these changes tested?
Yes.
Are there any user-facing changes?
No.