You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "mihaibudiu (via GitHub)" <gi...@apache.org> on 2025/06/16 00:42:25 UTC
[PR] [CALCITE-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
mihaibudiu opened a new pull request, #4417:
URL: https://github.com/apache/calcite/pull/4417
…alidator rejects legal queries
I have tried to validate these behaviors using MySQL, which is similar in this respect with our LENIENT conformance.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4417:
URL: https://github.com/apache/calcite/pull/4417#issuecomment-2993206070
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4417) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4417&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4417&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4417&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [81.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4417&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4417&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4417)
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4417:
URL: https://github.com/apache/calcite/pull/4417#issuecomment-2982331814
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4417) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4417&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4417&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4417&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [83.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4417&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4417&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4417)
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2149015796
##########
core/src/test/resources/sql/misc.iq:
##########
@@ -2460,24 +2460,288 @@ FROM "hr"."emps";
!ok
-!use post
+!use scott
# [CALCITE-4512] GROUP BY expression with argument name same with SELECT field and alias causes validation error
SELECT floor(empno/2) as empno
-FROM emps
+FROM emp
+GROUP BY floor(empno/2);
++-------+
+| EMPNO |
++-------+
+| 3684 |
+| 3749 |
+| 3760 |
+| 3783 |
+| 3827 |
+| 3849 |
+| 3891 |
+| 3894 |
+| 3919 |
+| 3922 |
+| 3938 |
+| 3950 |
+| 3951 |
+| 3967 |
++-------+
+(14 rows)
+
+!ok
+
+!use scott-lenient
+
+# Result should be the same as above
+# Validated using MySQL
+SELECT floor(empno/2) as empno
Review Comment:
yes, the sql is the same, but the conformance is different. that's the whole point: a query that is valid in all conformances should always produce the exact same result
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4417:
URL: https://github.com/apache/calcite/pull/4417#issuecomment-2974848767
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4417) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4417&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4417&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4417&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [80.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4417&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4417&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4417)
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2149012298
##########
core/src/test/resources/sql/misc.iq:
##########
@@ -2460,24 +2460,288 @@ FROM "hr"."emps";
!ok
-!use post
+!use scott
# [CALCITE-4512] GROUP BY expression with argument name same with SELECT field and alias causes validation error
SELECT floor(empno/2) as empno
-FROM emps
+FROM emp
+GROUP BY floor(empno/2);
++-------+
+| EMPNO |
++-------+
+| 3684 |
+| 3749 |
+| 3760 |
+| 3783 |
+| 3827 |
+| 3849 |
+| 3891 |
+| 3894 |
+| 3919 |
+| 3922 |
+| 3938 |
+| 3950 |
+| 3951 |
+| 3967 |
++-------+
+(14 rows)
+
+!ok
+
+!use scott-lenient
+
+# Result should be the same as above
+# Validated using MySQL
+SELECT floor(empno/2) as empno
Review Comment:
Is this same as the line 2466?
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2157687894
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6589,12 +6610,14 @@ public boolean isBangEqualAllowed() {
sql("select deptno + empno as d, deptno + empno + mgr from emp"
+ " group by d,mgr")
.withConformance(lenient).ok();
- // When alias is equal to one or more columns in the query then giving
- // priority to alias. But Postgres may throw ambiguous column error or give
- // priority to column name.
+ // Alias is not used since there is a single column in the SELECT already matching
sql("select count(*) from (\n"
- + " select ename AS deptno FROM emp GROUP BY deptno) t")
- .withConformance(lenient).ok();
+ + " select ^ename^ AS deptno FROM emp GROUP BY deptno) t")
+ .withConformance(strict)
+ .fails("Expression 'ENAME' is not being grouped")
+ .withConformance(lenient)
Review Comment:
@xiedeyantu I would like to better understand your concerns. This stuff is very subtle, and it would be nice to get it right.
There are 3 features touched in this PR:
1. alias resolution in GROUP BY
2. alias resolution in HAVING
3. alias resolution in SELECT (lateral column aliasing) (I should have not changed this at all in this PR, but read further)
As far as I know, the current implementation behaves like MySQL with respect to alias resolution in 1. GROUP BY and 2. HAVING. It also behaves like Spark with respect to alias resolution in 3. SELECT. MySQL does not seem to have 3, lateral column aliasing.
This implementation may be compatible with Spark with respect to 1. GROUP BY and 2. HAVING too, but I don't have an easy way to check. Neither of these systems has a spec of the exact behavior.
For MySQL I have validated all tests in aliasing.iq; they give the same results. I don't know if the tests cover all combinations possible; I tried to cover all combinations of features 1 & 2.
Unfortunately these three features are not independent, and they interact in subtle ways. I don't have a good way to test whether our current implementation is "sound", since MySQL supports only 1 & 2 of the features.
Do you find the comments in the code incorrect or unclear?
There is one major principle that should be obeyed by any implementation: the semantics of any legal query in "standard" SQL (without these features) does not change after these features are implemented. The bug I am fixing with this PR is exactly in this space: legal queries became illegal, and some legal queries produced different results.
How illegal queries in standard SQL behave - there you have choices. Some will remain illegal, some will become legal. I tried to emulate MySQL as far as I could test, since I expect it is not unsound.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2149019488
##########
core/src/test/resources/sql/misc.iq:
##########
@@ -2460,24 +2460,288 @@ FROM "hr"."emps";
!ok
-!use post
+!use scott
# [CALCITE-4512] GROUP BY expression with argument name same with SELECT field and alias causes validation error
SELECT floor(empno/2) as empno
-FROM emps
+FROM emp
+GROUP BY floor(empno/2);
++-------+
+| EMPNO |
++-------+
+| 3684 |
+| 3749 |
+| 3760 |
+| 3783 |
+| 3827 |
+| 3849 |
+| 3891 |
+| 3894 |
+| 3919 |
+| 3922 |
+| 3938 |
+| 3950 |
+| 3951 |
+| 3967 |
++-------+
+(14 rows)
+
+!ok
+
+!use scott-lenient
+
+# Result should be the same as above
+# Validated using MySQL
+SELECT floor(empno/2) as empno
Review Comment:
You are right, I missed the `!use scott-lenient`.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2153607239
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -7236,6 +7236,30 @@ public SqlNode go(SqlNode root) {
() -> this + " returned null for " + root);
}
+ /** True if the exception ex indicates that name lookup has failed. */
+ public static boolean isNotFoundException(Exception ex) {
+ if (!(ex instanceof CalciteContextException)) {
+ return false;
+ }
+ String message = ex.getMessage();
+ if (message != null && !message.contains("not found")) {
Review Comment:
turns out that StringUtils.contains does not work, since it is not annotated with `@Nullable`, and thus is causes the CheckedFramework to reject the program.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2148932464
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -7236,6 +7236,30 @@ public SqlNode go(SqlNode root) {
() -> this + " returned null for " + root);
}
+ /** True if the exception ex indicates that name lookup has failed. */
+ public static boolean isNotFoundException(Exception ex) {
+ if (!(ex instanceof CalciteContextException)) {
+ return false;
+ }
+ String message = ex.getMessage();
+ if (message != null && !message.contains("not found")) {
Review Comment:
A little hard code, here seems can use StringUtils.contains(message, "not found") to instead.
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -7236,6 +7236,30 @@ public SqlNode go(SqlNode root) {
() -> this + " returned null for " + root);
}
+ /** True if the exception ex indicates that name lookup has failed. */
+ public static boolean isNotFoundException(Exception ex) {
+ if (!(ex instanceof CalciteContextException)) {
+ return false;
+ }
+ String message = ex.getMessage();
+ if (message != null && !message.contains("not found")) {
+ return false;
+ }
+ return true;
+ }
+
+ /** True if the exception ex indicates that a column is ambiguous. */
+ public static boolean isAmbiguousException(Exception ex) {
+ if (!(ex instanceof CalciteContextException)) {
+ return false;
+ }
+ String message = ex.getMessage();
+ if (message != null && !message.contains("is ambiguous")) {
Review Comment:
Same as above.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2156240861
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6589,12 +6610,14 @@ public boolean isBangEqualAllowed() {
sql("select deptno + empno as d, deptno + empno + mgr from emp"
+ " group by d,mgr")
.withConformance(lenient).ok();
- // When alias is equal to one or more columns in the query then giving
- // priority to alias. But Postgres may throw ambiguous column error or give
- // priority to column name.
+ // Alias is not used since there is a single column in the SELECT already matching
sql("select count(*) from (\n"
- + " select ename AS deptno FROM emp GROUP BY deptno) t")
- .withConformance(lenient).ok();
+ + " select ^ename^ AS deptno FROM emp GROUP BY deptno) t")
+ .withConformance(strict)
+ .fails("Expression 'ENAME' is not being grouped")
+ .withConformance(lenient)
Review Comment:
I think I understand you, so should we not emphasize the consistency with any database behavior? This is the behavior of calcite? This is just my personal suggestion, no need to make any changes.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2159486606
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6589,12 +6610,14 @@ public boolean isBangEqualAllowed() {
sql("select deptno + empno as d, deptno + empno + mgr from emp"
+ " group by d,mgr")
.withConformance(lenient).ok();
- // When alias is equal to one or more columns in the query then giving
- // priority to alias. But Postgres may throw ambiguous column error or give
- // priority to column name.
+ // Alias is not used since there is a single column in the SELECT already matching
sql("select count(*) from (\n"
- + " select ename AS deptno FROM emp GROUP BY deptno) t")
- .withConformance(lenient).ok();
+ + " select ^ename^ AS deptno FROM emp GROUP BY deptno) t")
+ .withConformance(strict)
+ .fails("Expression 'ENAME' is not being grouped")
+ .withConformance(lenient)
Review Comment:
The comment should say "if there is no column in the FROM with the name..."
I will fix it.
Would this make it clear?
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2149013085
##########
core/src/test/resources/sql/misc.iq:
##########
@@ -2460,24 +2460,288 @@ FROM "hr"."emps";
!ok
-!use post
+!use scott
# [CALCITE-4512] GROUP BY expression with argument name same with SELECT field and alias causes validation error
SELECT floor(empno/2) as empno
-FROM emps
+FROM emp
+GROUP BY floor(empno/2);
++-------+
+| EMPNO |
++-------+
+| 3684 |
+| 3749 |
+| 3760 |
+| 3783 |
+| 3827 |
+| 3849 |
+| 3891 |
+| 3894 |
+| 3919 |
+| 3922 |
+| 3938 |
+| 3950 |
+| 3951 |
+| 3967 |
++-------+
+(14 rows)
+
+!ok
+
+!use scott-lenient
+
+# Result should be the same as above
+# Validated using MySQL
+SELECT floor(empno/2) as empno
Review Comment:
yes, that's what the comment says
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2149511988
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6806,12 +6829,13 @@ public boolean isBangEqualAllowed() {
sql("select ^e.empno^ from emp as e group by 1 having e.empno > 10")
.withConformance(strict).fails("Expression 'E.EMPNO' is not being grouped")
.withConformance(lenient).ok();
- // When alias is equal to one or more columns in the query then giving
- // priority to alias, but PostgreSQL throws ambiguous column error or gives
- // priority to column name.
+ // When alias matches multiple columns in the query, alias has priority.
+ sql("select ename AS deptno FROM emp, dept GROUP BY ^deptno^ HAVING deptno = 2")
+ .withConformance(strict).fails("Column 'DEPTNO' is ambiguous")
+ .withConformance(lenient).ok();
sql("select count(empno) as deptno from emp having ^deptno^ > 10")
.withConformance(strict).fails("Expression 'DEPTNO' is not being grouped")
- .withConformance(lenient).ok();
+ .withConformance(lenient).fails("Expression 'DEPTNO' is not being grouped");
Review Comment:
I tried spark, mysql, and pgsql, and currently only pgsql fails.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4417:
URL: https://github.com/apache/calcite/pull/4417#issuecomment-2982284004
Hopefully I have fixed the handling of HAVING aliases too; all the tests I wrote match the behavior of MySQL.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2155894692
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -7236,6 +7236,30 @@ public SqlNode go(SqlNode root) {
() -> this + " returned null for " + root);
}
+ /** True if the exception ex indicates that name lookup has failed. */
+ public static boolean isNotFoundException(Exception ex) {
+ if (!(ex instanceof CalciteContextException)) {
+ return false;
+ }
+ String message = ex.getMessage();
+ if (message != null && !message.contains("not found")) {
Review Comment:
OK，keep it as current version is better.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2148935487
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -7560,13 +7583,29 @@ static class ExtendedExpander extends Expander {
}
final SelectScope selectScope = validator.getRawSelectScopeNonNull(select);
- final boolean replaceAliases = clause.shouldReplaceAliases(validator.config);
- if (!replaceAliases || (clause == Clause.GROUP_BY && !aliasOrdinalExpandSet.contains(id))) {
- SqlNode node = expandCommonColumn(select, id, selectScope, validator);
- if (node != id) {
- return node;
+ try {
+ // First try a standard expansion
+ if (clause == Clause.GROUP_BY || clause == Clause.HAVING) {
+ SqlNode node = expandCommonColumn(select, id, selectScope, validator);
+ if (node != id) {
+ return node;
+ }
+ return super.visit(id);
}
- return super.visit(id);
+ } catch (Exception ex) {
+ // This behavior is from MySQL:
+ // - if there is no column in the SELECT with the name used in the GROUP BY or HAVING,
+ // then look for a column alias in the SELECT
+ // - if there are multiple columns in the SELECT with the name used in the GROUP BY or
+ // HAVING, then also look for a column alias in the SELECT
+ if (!Expander.isNotFoundException(ex) && !Expander.isAmbiguousException(ex)) {
+ throw ex;
+ }
+ final boolean replaceAliases = clause.shouldReplaceAliases(validator.config);
+ if (!replaceAliases) {
+ throw ex;
+ }
+ // Fall through and try to replace alias
Review Comment:
Sure this comment is in the correct position?
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -7683,7 +7720,7 @@ private void addIdentifierOrdinal2ExpandSet(@UnknownInitialization ExtendedExpan
for (SqlNode node : rowOperandList) {
addIdentifierOrdinal2ExpandSet(node);
}
- } else if (sqlNode.getKind() == SqlKind.IDENTIFIER || sqlNode.getKind() == SqlKind.LITERAL) {
+ } else if (sqlNode.getKind() == SqlKind.LITERAL) {
Review Comment:
+1
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2148935824
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -7683,7 +7720,7 @@ private void addIdentifierOrdinal2ExpandSet(@UnknownInitialization ExtendedExpan
for (SqlNode node : rowOperandList) {
addIdentifierOrdinal2ExpandSet(node);
}
- } else if (sqlNode.getKind() == SqlKind.IDENTIFIER || sqlNode.getKind() == SqlKind.LITERAL) {
+ } else if (sqlNode.getKind() == SqlKind.LITERAL) {
Review Comment:
+1
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #4417:
URL: https://github.com/apache/calcite/pull/4417#issuecomment-2993198944
I think squash commits are OK now. Wait 24 hours and merge if there are no other comments.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2156207543
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6589,12 +6610,14 @@ public boolean isBangEqualAllowed() {
sql("select deptno + empno as d, deptno + empno + mgr from emp"
+ " group by d,mgr")
.withConformance(lenient).ok();
- // When alias is equal to one or more columns in the query then giving
- // priority to alias. But Postgres may throw ambiguous column error or give
- // priority to column name.
+ // Alias is not used since there is a single column in the SELECT already matching
sql("select count(*) from (\n"
- + " select ename AS deptno FROM emp GROUP BY deptno) t")
- .withConformance(lenient).ok();
+ + " select ^ename^ AS deptno FROM emp GROUP BY deptno) t")
+ .withConformance(strict)
+ .fails("Expression 'ENAME' is not being grouped")
+ .withConformance(lenient)
Review Comment:
sorry, didn't look carefully, the comment is not in the test, and it's fine.
This test fails because there is exactly one column named DEPTNO in the collection produced by FROM, and thus the alias is not substituted in the GROUP BY; the GROUP BY will be done on ENAME and not DEPTNO.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2156224578
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6589,12 +6610,14 @@ public boolean isBangEqualAllowed() {
sql("select deptno + empno as d, deptno + empno + mgr from emp"
+ " group by d,mgr")
.withConformance(lenient).ok();
- // When alias is equal to one or more columns in the query then giving
- // priority to alias. But Postgres may throw ambiguous column error or give
- // priority to column name.
+ // Alias is not used since there is a single column in the SELECT already matching
sql("select count(*) from (\n"
- + " select ename AS deptno FROM emp GROUP BY deptno) t")
- .withConformance(lenient).ok();
+ + " select ^ename^ AS deptno FROM emp GROUP BY deptno) t")
+ .withConformance(strict)
+ .fails("Expression 'ENAME' is not being grouped")
+ .withConformance(lenient)
Review Comment:
I also didn't say "completely like MySQL", because I don't think MySQL has lateral column aliasing, which is a separate feature just for SELECT. Here we deal with alias expansion for GROUP BY, HAVING, and SELECT, and each of them has different rules.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4417:
URL: https://github.com/apache/calcite/pull/4417#issuecomment-2993698416
I will try to fix and merge first [CALCITE-7060] and move the tests I added here back to core from babel.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2155811235
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6589,12 +6610,14 @@ public boolean isBangEqualAllowed() {
sql("select deptno + empno as d, deptno + empno + mgr from emp"
+ " group by d,mgr")
.withConformance(lenient).ok();
- // When alias is equal to one or more columns in the query then giving
- // priority to alias. But Postgres may throw ambiguous column error or give
- // priority to column name.
+ // Alias is not used since there is a single column in the SELECT already matching
sql("select count(*) from (\n"
- + " select ename AS deptno FROM emp GROUP BY deptno) t")
- .withConformance(lenient).ok();
+ + " select ^ename^ AS deptno FROM emp GROUP BY deptno) t")
+ .withConformance(strict)
+ .fails("Expression 'ENAME' is not being grouped")
+ .withConformance(lenient)
Review Comment:
this test fails on MySQL with a similar error
Perhaps the comment is not helping; I will delete it.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "xuzifu666 (via GitHub)" <gi...@apache.org>.
xuzifu666 commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2148941193
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -7236,6 +7236,30 @@ public SqlNode go(SqlNode root) {
() -> this + " returned null for " + root);
}
+ /** True if the exception ex indicates that name lookup has failed. */
+ public static boolean isNotFoundException(Exception ex) {
+ if (!(ex instanceof CalciteContextException)) {
+ return false;
+ }
+ String message = ex.getMessage();
+ if (message != null && !message.contains("not found")) {
Review Comment:
This judgment is somewhat repetitive, and the effect of StringUtils.contains should be the same as these judgments.
this is StringUtils.contains related codes:
```
public static boolean contains(String str, String searchStr) {
if (str == null || searchStr == null) {
return false;
}
return str.indexOf(searchStr) >= 0;
}
```
It's just a suggestion, no problem.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4417:
URL: https://github.com/apache/calcite/pull/4417#issuecomment-2982708061
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4417) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4417&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4417&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4417&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [81.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4417&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4417&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4417)
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #4417:
URL: https://github.com/apache/calcite/pull/4417
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2156226622
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6589,12 +6610,14 @@ public boolean isBangEqualAllowed() {
sql("select deptno + empno as d, deptno + empno + mgr from emp"
+ " group by d,mgr")
.withConformance(lenient).ok();
- // When alias is equal to one or more columns in the query then giving
- // priority to alias. But Postgres may throw ambiguous column error or give
- // priority to column name.
+ // Alias is not used since there is a single column in the SELECT already matching
sql("select count(*) from (\n"
- + " select ename AS deptno FROM emp GROUP BY deptno) t")
- .withConformance(lenient).ok();
+ + " select ^ename^ AS deptno FROM emp GROUP BY deptno) t")
+ .withConformance(strict)
+ .fails("Expression 'ENAME' is not being grouped")
+ .withConformance(lenient)
Review Comment:
All this shows again that programming language semantics is a very subtle thing, which has to be considered carefully. I think these rules are quite complicated, and I would prefer not to deal with them, but since Spark and MySQL have such features, and users ask for them, we have to make an effort. Neither of these systems has a very sharp specification that we can rely on.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2155841737
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6589,12 +6610,14 @@ public boolean isBangEqualAllowed() {
sql("select deptno + empno as d, deptno + empno + mgr from emp"
+ " group by d,mgr")
.withConformance(lenient).ok();
- // When alias is equal to one or more columns in the query then giving
- // priority to alias. But Postgres may throw ambiguous column error or give
- // priority to column name.
+ // Alias is not used since there is a single column in the SELECT already matching
sql("select count(*) from (\n"
- + " select ename AS deptno FROM emp GROUP BY deptno) t")
- .withConformance(lenient).ok();
+ + " select ^ename^ AS deptno FROM emp GROUP BY deptno) t")
+ .withConformance(strict)
+ .fails("Expression 'ENAME' is not being grouped")
+ .withConformance(lenient)
Review Comment:
This PR is not really about omitting GROUP BY, it is about the meaning of aliases in GROUP BY.
This comment should probably be somewhere in the code and not on the test case.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2155844113
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6589,12 +6610,14 @@ public boolean isBangEqualAllowed() {
sql("select deptno + empno as d, deptno + empno + mgr from emp"
+ " group by d,mgr")
.withConformance(lenient).ok();
- // When alias is equal to one or more columns in the query then giving
- // priority to alias. But Postgres may throw ambiguous column error or give
- // priority to column name.
+ // Alias is not used since there is a single column in the SELECT already matching
sql("select count(*) from (\n"
- + " select ename AS deptno FROM emp GROUP BY deptno) t")
- .withConformance(lenient).ok();
+ + " select ^ename^ AS deptno FROM emp GROUP BY deptno) t")
+ .withConformance(strict)
+ .fails("Expression 'ENAME' is not being grouped")
+ .withConformance(lenient)
Review Comment:
Yes, it should be in the code. I just saw that this test case mentioned the comment in the code.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2159489058
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6589,12 +6610,14 @@ public boolean isBangEqualAllowed() {
sql("select deptno + empno as d, deptno + empno + mgr from emp"
+ " group by d,mgr")
.withConformance(lenient).ok();
- // When alias is equal to one or more columns in the query then giving
- // priority to alias. But Postgres may throw ambiguous column error or give
- // priority to column name.
+ // Alias is not used since there is a single column in the SELECT already matching
sql("select count(*) from (\n"
- + " select ename AS deptno FROM emp GROUP BY deptno) t")
- .withConformance(lenient).ok();
+ + " select ^ename^ AS deptno FROM emp GROUP BY deptno) t")
+ .withConformance(strict)
+ .fails("Expression 'ENAME' is not being grouped")
+ .withConformance(lenient)
Review Comment:
In this case there is exactly one column in the FROM with the same name, so neither case applies
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4417:
URL: https://github.com/apache/calcite/pull/4417#issuecomment-2993193680
Thanks!
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2148937729
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -7236,6 +7236,30 @@ public SqlNode go(SqlNode root) {
() -> this + " returned null for " + root);
}
+ /** True if the exception ex indicates that name lookup has failed. */
+ public static boolean isNotFoundException(Exception ex) {
+ if (!(ex instanceof CalciteContextException)) {
+ return false;
+ }
+ String message = ex.getMessage();
+ if (message != null && !message.contains("not found")) {
Review Comment:
Frankly, I am not sure why StringUtil is better
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4417:
URL: https://github.com/apache/calcite/pull/4417#issuecomment-2993856953
I will rebase this on main and move the aliasing tests to CoreQuidemTests, which is easy to do since we merged [CALCITE-7060]. I will squash the commits too in the same move, in preparation for merging.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4417:
URL: https://github.com/apache/calcite/pull/4417#issuecomment-2993871664
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4417) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4417&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4417&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4417&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [81.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4417&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4417&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4417)
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2153430735
##########
core/src/test/resources/sql/misc.iq:
##########
@@ -2460,24 +2460,6 @@ FROM "hr"."emps";
!ok
-!use post
Review Comment:
This test was not doing the right thing, since conformance was wrong; I replaced it with a different test, which is now in aliasing.iq
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2155839399
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6589,12 +6610,14 @@ public boolean isBangEqualAllowed() {
sql("select deptno + empno as d, deptno + empno + mgr from emp"
+ " group by d,mgr")
.withConformance(lenient).ok();
- // When alias is equal to one or more columns in the query then giving
- // priority to alias. But Postgres may throw ambiguous column error or give
- // priority to column name.
+ // Alias is not used since there is a single column in the SELECT already matching
sql("select count(*) from (\n"
- + " select ename AS deptno FROM emp GROUP BY deptno) t")
- .withConformance(lenient).ok();
+ + " select ^ename^ AS deptno FROM emp GROUP BY deptno) t")
+ .withConformance(strict)
+ .fails("Expression 'ENAME' is not being grouped")
+ .withConformance(lenient)
Review Comment:
If I set SET SESSION sql_mode = '' in MySQL, then this sql can be passed. But MySQL defaults to sql_mode = only_full_group_by. Do we need to consider this? Or should we keep the correct description and correct the incomplete description? I think this description is very helpful.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2148887073
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -8218,14 +8255,10 @@ private enum Clause {
boolean shouldReplaceAliases(Config config) {
switch (this) {
case GROUP_BY:
- return config.conformance().isGroupByAlias()
- || (config.conformance().isSelectAlias()
- != SqlConformance.SelectAliasLookup.UNSUPPORTED);
+ return config.conformance().isGroupByAlias();
Review Comment:
Previously lateral aliases in SELECT were always implying GROUP BY and HAVING aliases.
This is a change which I had introduced, but I think there is no reason these should not be separated.
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -7683,7 +7720,7 @@ private void addIdentifierOrdinal2ExpandSet(@UnknownInitialization ExtendedExpan
for (SqlNode node : rowOperandList) {
addIdentifierOrdinal2ExpandSet(node);
}
- } else if (sqlNode.getKind() == SqlKind.IDENTIFIER || sqlNode.getKind() == SqlKind.LITERAL) {
+ } else if (sqlNode.getKind() == SqlKind.LITERAL) {
Review Comment:
IDENTIFIER was not useful, since the aliasOrdinal set should only contain ordinals.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2149014808
##########
core/src/test/resources/sql/misc.iq:
##########
@@ -2460,24 +2460,288 @@ FROM "hr"."emps";
!ok
-!use post
+!use scott
# [CALCITE-4512] GROUP BY expression with argument name same with SELECT field and alias causes validation error
SELECT floor(empno/2) as empno
-FROM emps
+FROM emp
+GROUP BY floor(empno/2);
++-------+
+| EMPNO |
++-------+
+| 3684 |
+| 3749 |
+| 3760 |
+| 3783 |
+| 3827 |
+| 3849 |
+| 3891 |
+| 3894 |
+| 3919 |
+| 3922 |
+| 3938 |
+| 3950 |
+| 3951 |
+| 3967 |
++-------+
+(14 rows)
+
+!ok
+
+!use scott-lenient
+
+# Result should be the same as above
+# Validated using MySQL
+SELECT floor(empno/2) as empno
Review Comment:
I see that the sql is exactly the same, is it a duplicate?
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2148938088
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java:
##########
@@ -7560,13 +7583,29 @@ static class ExtendedExpander extends Expander {
}
final SelectScope selectScope = validator.getRawSelectScopeNonNull(select);
- final boolean replaceAliases = clause.shouldReplaceAliases(validator.config);
- if (!replaceAliases || (clause == Clause.GROUP_BY && !aliasOrdinalExpandSet.contains(id))) {
- SqlNode node = expandCommonColumn(select, id, selectScope, validator);
- if (node != id) {
- return node;
+ try {
+ // First try a standard expansion
+ if (clause == Clause.GROUP_BY || clause == Clause.HAVING) {
+ SqlNode node = expandCommonColumn(select, id, selectScope, validator);
+ if (node != id) {
+ return node;
+ }
+ return super.visit(id);
}
- return super.visit(id);
+ } catch (Exception ex) {
+ // This behavior is from MySQL:
+ // - if there is no column in the SELECT with the name used in the GROUP BY or HAVING,
+ // then look for a column alias in the SELECT
+ // - if there are multiple columns in the SELECT with the name used in the GROUP BY or
+ // HAVING, then also look for a column alias in the SELECT
+ if (!Expander.isNotFoundException(ex) && !Expander.isAmbiguousException(ex)) {
+ throw ex;
+ }
+ final boolean replaceAliases = clause.shouldReplaceAliases(validator.config);
+ if (!replaceAliases) {
+ throw ex;
+ }
+ // Fall through and try to replace alias
Review Comment:
yes, I want to convey the fact that we ignore the exception and continue processing
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "suibianwanwank (via GitHub)" <gi...@apache.org>.
suibianwanwank commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2149247203
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6806,12 +6829,13 @@ public boolean isBangEqualAllowed() {
sql("select ^e.empno^ from emp as e group by 1 having e.empno > 10")
.withConformance(strict).fails("Expression 'E.EMPNO' is not being grouped")
.withConformance(lenient).ok();
- // When alias is equal to one or more columns in the query then giving
- // priority to alias, but PostgreSQL throws ambiguous column error or gives
- // priority to column name.
+ // When alias matches multiple columns in the query, alias has priority.
+ sql("select ename AS deptno FROM emp, dept GROUP BY ^deptno^ HAVING deptno = 2")
+ .withConformance(strict).fails("Column 'DEPTNO' is ambiguous")
+ .withConformance(lenient).ok();
sql("select count(empno) as deptno from emp having ^deptno^ > 10")
.withConformance(strict).fails("Expression 'DEPTNO' is not being grouped")
- .withConformance(lenient).ok();
+ .withConformance(lenient).fails("Expression 'DEPTNO' is not being grouped");
Review Comment:
Other tests look good, Why does this one fail in lenient?
It runs successfully in SparkSQL. Maybe deptno is unambiguous in this context?
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2159782016
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6589,12 +6610,14 @@ public boolean isBangEqualAllowed() {
sql("select deptno + empno as d, deptno + empno + mgr from emp"
+ " group by d,mgr")
.withConformance(lenient).ok();
- // When alias is equal to one or more columns in the query then giving
- // priority to alias. But Postgres may throw ambiguous column error or give
- // priority to column name.
+ // Alias is not used since there is a single column in the SELECT already matching
sql("select count(*) from (\n"
- + " select ename AS deptno FROM emp GROUP BY deptno) t")
- .withConformance(lenient).ok();
+ + " select ^ename^ AS deptno FROM emp GROUP BY deptno) t")
+ .withConformance(strict)
+ .fails("Expression 'ENAME' is not being grouped")
+ .withConformance(lenient)
Review Comment:
I have pushed a commit to change the comment
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2152778599
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6806,12 +6829,13 @@ public boolean isBangEqualAllowed() {
sql("select ^e.empno^ from emp as e group by 1 having e.empno > 10")
.withConformance(strict).fails("Expression 'E.EMPNO' is not being grouped")
.withConformance(lenient).ok();
- // When alias is equal to one or more columns in the query then giving
- // priority to alias, but PostgreSQL throws ambiguous column error or gives
- // priority to column name.
+ // When alias matches multiple columns in the query, alias has priority.
+ sql("select ename AS deptno FROM emp, dept GROUP BY ^deptno^ HAVING deptno = 2")
+ .withConformance(strict).fails("Column 'DEPTNO' is ambiguous")
+ .withConformance(lenient).ok();
sql("select count(empno) as deptno from emp having ^deptno^ > 10")
.withConformance(strict).fails("Expression 'DEPTNO' is not being grouped")
- .withConformance(lenient).ok();
+ .withConformance(lenient).fails("Expression 'DEPTNO' is not being grouped");
Review Comment:
it looks like my handling of HAVING is not compatible with MySQL, I will try to improve it.
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2155807386
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6589,12 +6610,14 @@ public boolean isBangEqualAllowed() {
sql("select deptno + empno as d, deptno + empno + mgr from emp"
+ " group by d,mgr")
.withConformance(lenient).ok();
- // When alias is equal to one or more columns in the query then giving
- // priority to alias. But Postgres may throw ambiguous column error or give
- // priority to column name.
+ // Alias is not used since there is a single column in the SELECT already matching
sql("select count(*) from (\n"
- + " select ename AS deptno FROM emp GROUP BY deptno) t")
- .withConformance(lenient).ok();
+ + " select ^ename^ AS deptno FROM emp GROUP BY deptno) t")
+ .withConformance(strict)
+ .fails("Expression 'ENAME' is not being grouped")
+ .withConformance(lenient)
Review Comment:
> // This behavior is from MySQL:
> // - if there is no column in the SELECT with the name used in the GROUP BY or HAVING,
> // then look for a column alias in the SELECT
> // - if there are multiple columns in the SELECT with the name used in the GROUP BY or
> // HAVING, then also look for a column alias in the SELECT
Given the description above, should this test using lenient pass? Does the original description of this case seem correct?
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4417:
URL: https://github.com/apache/calcite/pull/4417#issuecomment-2986177636
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4417) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4417&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4417&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4417&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [81.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4417&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4417&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4417)
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2156218256
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6589,12 +6610,14 @@ public boolean isBangEqualAllowed() {
sql("select deptno + empno as d, deptno + empno + mgr from emp"
+ " group by d,mgr")
.withConformance(lenient).ok();
- // When alias is equal to one or more columns in the query then giving
- // priority to alias. But Postgres may throw ambiguous column error or give
- // priority to column name.
+ // Alias is not used since there is a single column in the SELECT already matching
sql("select count(*) from (\n"
- + " select ename AS deptno FROM emp GROUP BY deptno) t")
- .withConformance(lenient).ok();
+ + " select ^ename^ AS deptno FROM emp GROUP BY deptno) t")
+ .withConformance(strict)
+ .fails("Expression 'ENAME' is not being grouped")
+ .withConformance(lenient)
Review Comment:
I am a little confused. The code comments say that it is implemented completely according to MySQL's behavior, and there are two explanations, but the test case does not seem to meet these two explanations?
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4417:
URL: https://github.com/apache/calcite/pull/4417#issuecomment-2986080068
This is now ready for review; most of the code is in tests actually, and most tests are relatively simple queries.
(Maybe there should be more...)
--
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-7052] When conformance specifies isGroupbyAlias = true the v… [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4417:
URL: https://github.com/apache/calcite/pull/4417#discussion_r2157871990
##########
core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java:
##########
@@ -6589,12 +6610,14 @@ public boolean isBangEqualAllowed() {
sql("select deptno + empno as d, deptno + empno + mgr from emp"
+ " group by d,mgr")
.withConformance(lenient).ok();
- // When alias is equal to one or more columns in the query then giving
- // priority to alias. But Postgres may throw ambiguous column error or give
- // priority to column name.
+ // Alias is not used since there is a single column in the SELECT already matching
sql("select count(*) from (\n"
- + " select ename AS deptno FROM emp GROUP BY deptno) t")
- .withConformance(lenient).ok();
+ + " select ^ename^ AS deptno FROM emp GROUP BY deptno) t")
+ .withConformance(strict)
+ .fails("Expression 'ENAME' is not being grouped")
+ .withConformance(lenient)
Review Comment:
Thank you very much for your detailed reply. I've tested this example:
```
CREATE TABLE emps (
empid int,
deptno int,
name varchar(255),
salary int,
commission int
);
INSERT INTO emps VALUES (100, 10, "Bill", 10000, 1000);
INSERT INTO emps VALUES (110, 10, "Theodore", 11500, 250);
INSERT INTO emps VALUES (150, 10, "Sebastian", 7000, null);
INSERT INTO emps VALUES (200, 20, "Eric", 8000, 500);
-- first query --
select count(*) from (select name AS deptno FROM emps GROUP BY deptno) t;
-- second query --
select name AS deptno FROM emps GROUP BY deptno;
```
Both queries return the same error in MySQL:
```
ERROR 1055 (42000): Expression #1 of SELECT list is not in GROUP BY clause and contains nonaggregated column 'test.emps.name' which is not functionally dependent on columns in GROUP BY clause; this is incompatible with sql_mode=only_full_group_by
```
From my observation, MySQL's `only_full_group_by` SQL mode checks both:
1. Original column names in the SELECT clause for `deptno`
2. Column aliases in the SELECT clause for `deptno`
When setting `sql_mode=''`, MySQL returns what it considers correct results:
```
select name AS deptno FROM emps GROUP BY deptno;
+--------+
| deptno |
+--------+
| Bill |
| Eric |
+--------+
```
In this case, it recognizes the original column name `deptno`, not the alias `deptno`.
I believe we can ignore the `sql_mode=''` special case since MySQL defaults to `sql_mode=only_full_group_by`. Based on this, I found a code comment about GROUP BY handling:
```
// This behavior is from MySQL:
// - if there is no column in the SELECT with the name used in the GROUP BY
// then look for a column alias in the SELECT
// - if there are multiple columns in the SELECT with the name used in the GROUP BY,
// then also look for a column alias in the SELECT
```
I understand the first point, as demonstrated by this example:
```
select name AS deptno1 FROM emps GROUP BY deptno1;
+-----------+
| deptno1 |
+-----------+
| Bill |
| Theodore |
| Sebastian |
| Eric |
+-----------+
```
The second point is what confuses me. I thought it would work like this example:
```
set sql_mode=only_full_group_by;
select name AS deptno FROM emps GROUP BY deptno;
```
But MySQL doesn't look for the alias - it just errors out. This doesn't match the comment.
These tests were conducted on MySQL 8.0.23.
Perhaps I've misunderstood something - I'd appreciate any corrections to my understanding. Thank you!
--
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