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.
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.