-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(optimizer): optimize_projections rule bug #12184
Conversation
The plan looks like aliasing to the same name after
I wonder could we even remove the alias here, since the name is the same |
Removing the Alias will cause a schema change. 🤔 |
That is why I think the root cause might be the incorrect schema we have.
The expected schema should be something like.
a is aliased to "test.a". The column should be Upd:
This is the schema for the query without alias
This is the schema with alias
|
Yes. Possibly because the
|
due to my subjective recency bias, #1468 seems related. |
Not pretty sure, I think they are unrelated 🤔 I think the issue here is that we lost the qualifier for alias in subquery case |
It looks like we need to save the qualifiers in the |
Should we get Column's relation for Alias in Expr::Alias(Alias { expr, name, .. }) => {
let relation = match expr.as_ref() {
Expr::Column(c) => c.relation.to_owned(),
_ => None
};
let (data_type, nullable) = self.data_type_and_nullable(input_schema)?;
Ok((
relation,
Field::new(name, data_type, nullable)
.with_metadata(self.metadata(input_schema)?)
.into(),
))
} |
I think we should directly use the relation from the Alias itself, as that is the purpose of an Alias expression. After an expression is rewritten, by adding an Alias we can ensure that the result of |
thanks @jonahgao @jayzhan211 @alamb |
Which issue does this PR close?
Closes #12183
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?