Skip to content
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

[CALCITE-4512] GROUP BY expression with argument name same with SELECT field and alias causes validation error #3929

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hannerwang
Copy link

What changes were proposed in this pull request?

Stop identifiers in a SQL call from being treated as aliases and expanded into select expressions.
For example:
select replace(name, 'a', 'b') as name from users group by replace(name, 'a', 'b')
The name in GROUP BY clause should not be replaced by replace(name, 'a', 'b'), it will be resolved as users.name,
i.e.
select replace(users.name, 'a', 'b') as name from users group by replace(users.name, 'a', 'b')

Why are the changes needed?

Make the sql semantics unchanged when treating identifiers within Group By clause as aliases.
current calcite behavior:
select replace(**name**, 'a', 'b') as **name** from users group by replace(**name**, 'a', 'b')
will be expanded into
select replace(**name**, 'a', 'b') as **name** from users group by replace(**replace(name, 'a', 'b')**, 'a', 'b')
the semantics has been changed.

Does this PR introduce any user-facing change?

No for existing users

Copy link

sonarcloud bot commented Aug 22, 2024

case GROUPING_SETS:
case ROLLUP:
case CUBE:
if (root instanceof SqlBasicCall) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should SqlBasicCall be replaced with SqlCall here. e.g. SqlCase does not inherit SqlBasicCall.

case ROLLUP:
case CUBE:
if (root instanceof SqlBasicCall) {
List<SqlNode> operandList = ((SqlBasicCall) root).getOperandList();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@mihaibudiu
Copy link
Contributor

@hannerwang I don't know if you have seen the comments in the JIRA.
This PR is very close to being done.
Do you think you can address the comments in time for the next release of Calcite?

I reproduce the comments here:

Which DB are we trying to emulate the behavior of? If so, I would like to see a Quidem test that returns the same in Calcite and that DB, and would return the wrong result if the bug were not fixed. Perhaps something like select mod(empno, 3) as empno from emp group by mod(empno, 2) and make sure that we get the right number of rows.

Also I would like to see a SqlValidatorTest. Something like select date_add('2024-01-01', empno) as empno from emp group by year(empno), where there will be a type error (int vs date) if the wrong column is referenced.

In short, more tests - that are broken before the fix, and pass after the fix, and are demonstrably the same behavior as a given DB - will give me the confidence to approve this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants