fix: Ensure columns are casted to the correct names with Unions #20146
Conversation
github-actions
bot
added
logical-expr
| Projection: test.col_int32 AS id | ||
| TableScan: test projection=[col_int32] | ||
| Projection: CAST(CAST(nodes.id AS Int64) + Int64(1) AS Int32) AS id | ||
| Projection: CAST(CAST(nodes.id AS Int64) + Int64(1) AS Int32) |
There was a problem hiding this comment.
These changes are ok, the alias had been introduced by #19019 but it was not needed.
There was a problem hiding this comment.
it isn't needed because the outer query doesn't use id?
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thanks @nuno-faria -- this looks good to me. Given all the tests pass I think this one is good to go, though it is strange to me 🤔 I wonder if this is something specific to unions
| Ok(expr.cast_to(new_type, src_schema)?.alias(name)) | ||
| match expr { | ||
| // maintain the original name when casting a column | ||
| Expr::Column(ref column) => { |
There was a problem hiding this comment.
can we add some context about why we maintain the original name only for columns?
| Projection: test.col_int32 AS id | ||
| TableScan: test projection=[col_int32] | ||
| Projection: CAST(CAST(nodes.id AS Int64) + Int64(1) AS Int32) AS id | ||
| Projection: CAST(CAST(nodes.id AS Int64) + Int64(1) AS Int32) |
There was a problem hiding this comment.
it isn't needed because the outer query doesn't use id?
The outer query uses |
|
Thanks again @nuno-faria |
## Which issue does this PR close? - Follow on to #20146 from @nuno-faria ## Rationale for this change While reviewing the change with Codex, I noticed that the actual output names and their uniqueness were not verified, which I think would help the coverage ## What changes are included in this PR? Extend the test ## Are these changes tested? Yes it is only tests ## Are there any user-facing changes? No
3 participants
Which issue does this PR close?
Rationale for this change
When aliasing a cast, use the original name to prevent duplicate column names errors.
What changes are included in this PR?
coerce_exprs_for_schemato use the correct name.Are these changes tested?
Yes.
Are there any user-facing changes?
No.