Skip to content

Conversation

@jonahgao
Copy link
Member

Which issue does this PR close?

Closes #9161.

Rationale for this change

In issue #9161, the expression cast(a as int) in the select list generated a unqualified column named 't.a', which was then incorrectly referenced by the group by clause, leading to a schema error.

After merging PR #9228, the group by clause will now preferentially reference the column of the same name from the input, rather than the one in the select list, thereby resolving the issue.

I suspect there will still be similar issues, but we may need to find a new case to verify whether they truly exist.

What changes are included in this PR?

Add a test.

Are these changes tested?

Yes

Are there any user-facing changes?

No

@github-actions github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Feb 19, 2024
Copy link
Contributor

@comphead comphead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jonahgao I still don't get how this PR closes the issue 🤔

@jonahgao
Copy link
Member Author

Thanks @jonahgao I still don't get how this PR closes the issue 🤔

Previously, to parse column references in the 'GROUP BY' expression, we would sequentially search for the column names in the output schema of the select list and then in the input plan.

However, the cast expression generated an output column named "t.a", which has the same name as the column reference in the group by, thus resulting in an incorrect match, which should not happen.

After PR #9228, we first searched from the input plan and then obtained the correct match, which is the column a of table t.

I think there are still bugs concerning the naming of cast expressions and the planning of 'GROUP BY' clauses. But in this case, they cannot be reproduced. I need to find a new one.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jonahgao -- sorry for the delay in reviewing

I think this PR is nice that it shows a problem doesn't exist and ensures it won't start causing problems in the future

@alamb alamb merged commit d896ebe into apache:main Feb 28, 2024
@jonahgao
Copy link
Member Author

Thank you @jonahgao -- sorry for the delay in reviewing

I think this PR is nice that it shows a problem doesn't exist and ensures it won't start causing problems in the future

Thank you for the review! ❤️

@jonahgao jonahgao deleted the add_test branch February 28, 2024 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Schema error: No field named "t.a". Valid fields are t.a.

3 participants