You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "hannerwang (via GitHub)" <gi...@apache.org> on 2024/08/22 12:13:20 UTC
[PR] [CALCITE-4512] GROUP BY expression with argument name same with SELECT field and alias causes validation error [calcite]
hannerwang opened a new pull request, #3929:
URL: https://github.com/apache/calcite/pull/3929
### **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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4512] GROUP BY expression with argument name same with SELECT field and alias causes validation error [calcite]
Posted by "suibianwanwank (via GitHub)" <gi...@apache.org>.
suibianwanwank commented on code in PR #3929:
URL: https://github.com/apache/calcite/pull/3929#discussion_r1727272417
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -7378,6 +7368,37 @@ static class ExtendedExpander extends Expander {
return super.visit(literal);
}
+ /**
+ * Add all possible expandable 'group by' expression to set, which is
+ * used to check whether expr could be expanded as alias or ordinal.
+ */
+ @RequiresNonNull({"root"})
+ private void addExpandableExpressions(@UnknownInitialization ExtendedExpander this) {
+ switch (root.getKind()) {
+ case IDENTIFIER:
+ case LITERAL:
+ aliasOrdinalExpandSet.add(root);
+ break;
+ case GROUPING_SETS:
+ case ROLLUP:
+ case CUBE:
+ if (root instanceof SqlBasicCall) {
+ List<SqlNode> operandList = ((SqlBasicCall) root).getOperandList();
Review Comment:
Same as above.
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -7378,6 +7368,37 @@ static class ExtendedExpander extends Expander {
return super.visit(literal);
}
+ /**
+ * Add all possible expandable 'group by' expression to set, which is
+ * used to check whether expr could be expanded as alias or ordinal.
+ */
+ @RequiresNonNull({"root"})
+ private void addExpandableExpressions(@UnknownInitialization ExtendedExpander this) {
+ switch (root.getKind()) {
+ case IDENTIFIER:
+ case LITERAL:
+ aliasOrdinalExpandSet.add(root);
+ break;
+ case GROUPING_SETS:
+ case ROLLUP:
+ case CUBE:
+ if (root instanceof SqlBasicCall) {
Review Comment:
Should SqlBasicCall be replaced with SqlCall here. e.g. SqlCase does not inherit SqlBasicCall.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4512] GROUP BY expression with argument name same with SELECT field and alias causes validation error [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #3929:
URL: https://github.com/apache/calcite/pull/3929
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4512] GROUP BY expression with argument name same with SELECT field and alias causes validation error [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #3929:
URL: https://github.com/apache/calcite/pull/3929#issuecomment-2364906992
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-4512] GROUP BY expression with argument name same with SELECT field and alias causes validation error [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3929:
URL: https://github.com/apache/calcite/pull/3929#issuecomment-2304598510
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3929) **Quality Gate passed**
Issues
 [4 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3929&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3929&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3929&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [97.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3929&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3929&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3929)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org