You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "nielspardon (via GitHub)" <gi...@apache.org> on 2025/07/28 15:00:49 UTC
[PR] [CALCITE-7112] support correlation variables in HAVING clause in RelToSqlConverter [calcite]
nielspardon opened a new pull request, #4481:
URL: https://github.com/apache/calcite/pull/4481
Turns out the fix is pretty straightforward: Use `SqlImplementor.BaseContext` instead of `SqlImplementor.Context` in `SqlImplementor.Result.builder()`.
--
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-7112] Support correlation variables in HAVING clause in RelToSqlConverter [calcite]
Posted by "nielspardon (via GitHub)" <gi...@apache.org>.
nielspardon commented on code in PR #4481:
URL: https://github.com/apache/calcite/pull/4481#discussion_r2237948301
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1897,7 +1897,7 @@ private Builder builder(RelNode rel, Set<Clause> clauses) {
if (!selectList.equals(SqlNodeList.SINGLETON_STAR)) {
final boolean aliasRef = expectedClauses.contains(Clause.HAVING)
&& dialect.getConformance().isHavingAlias();
- newContext = new Context(dialect, selectList.size()) {
+ newContext = new BaseContext(dialect, selectList.size()) {
Review Comment:
`Context` is also an abstract class. what is happening here is that the previous author decided to instantiate a new instance of `Context` by implementing the abstract methods on the spot as an anonymous inner class. which is the syntax you are seeing here: `new Context(dialect, selectList.size()) {}` (note the curly braces)
--
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-7112] Support correlation variables in HAVING clause in RelToSqlConverter [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4481:
URL: https://github.com/apache/calcite/pull/4481#discussion_r2240484024
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -2337,6 +2285,69 @@ Result withExpectedClauses(boolean ignoreClauses,
: new Result(node, clauses, neededAlias, neededType, aliases, anon,
ignoreClauses, ImmutableSet.copyOf(expectedClauses), expectedRel);
}
+
+ /**
+ * A context that uses a select list.
Review Comment:
Frankly, this JavaDoc does not really help much.
But having the class separate it's still an improvement.
--
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-7112] Support correlation variables in HAVING clause in RelToSqlConverter [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4481:
URL: https://github.com/apache/calcite/pull/4481#discussion_r2240532916
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -2337,6 +2285,69 @@ Result withExpectedClauses(boolean ignoreClauses,
: new Result(node, clauses, neededAlias, neededType, aliases, anon,
ignoreClauses, ImmutableSet.copyOf(expectedClauses), expectedRel);
}
+
+ /**
+ * A context that uses a select list.
Review Comment:
Yes, I have approved.
My guess is that a description like "the local SELECT statement is used as a context" would be a good approximation.
But that not add much either.
So checkstyle wants JavaDoc even on private classes? Too bad.
--
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-7112] Correlation variable in HAVING clause causes UnsupportedOperationException in RelToSql conversion [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4481:
URL: https://github.com/apache/calcite/pull/4481#issuecomment-3134164046
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4481) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4481&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4481&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4481&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4481&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4481&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4481)
--
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-7112] Support correlation variables in HAVING clause in RelToSqlConverter [calcite]
Posted by "nielspardon (via GitHub)" <gi...@apache.org>.
nielspardon commented on PR #4481:
URL: https://github.com/apache/calcite/pull/4481#issuecomment-3133490409
> BTW: I think you can also squash the commits so we can merge
Thanks. Done.
--
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-7112] Support correlation variables in HAVING clause in RelToSqlConverter [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4481:
URL: https://github.com/apache/calcite/pull/4481#discussion_r2237951754
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1897,7 +1897,7 @@ private Builder builder(RelNode rel, Set<Clause> clauses) {
if (!selectList.equals(SqlNodeList.SINGLETON_STAR)) {
final boolean aliasRef = expectedClauses.contains(Clause.HAVING)
&& dialect.getConformance().isHavingAlias();
- newContext = new Context(dialect, selectList.size()) {
+ newContext = new BaseContext(dialect, selectList.size()) {
Review Comment:
I think this class is big enough to be pulled outside in a static named class.
(The original author should have done that, and documented what this class is supposed to do)
--
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-7112] Correlation variable in HAVING clause causes UnsupportedOperationException in RelToSql conversion [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #4481:
URL: https://github.com/apache/calcite/pull/4481
--
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-7112] Support correlation variables in HAVING clause in RelToSqlConverter [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4481:
URL: https://github.com/apache/calcite/pull/4481#discussion_r2237768195
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1897,7 +1897,7 @@ private Builder builder(RelNode rel, Set<Clause> clauses) {
if (!selectList.equals(SqlNodeList.SINGLETON_STAR)) {
final boolean aliasRef = expectedClauses.contains(Clause.HAVING)
&& dialect.getConformance().isHavingAlias();
- newContext = new Context(dialect, selectList.size()) {
+ newContext = new BaseContext(dialect, selectList.size()) {
Review Comment:
how is this even possible, BaseContext is abstract?
--
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-7112] Support correlation variables in HAVING clause in RelToSqlConverter [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4481:
URL: https://github.com/apache/calcite/pull/4481#issuecomment-3127810770
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4481) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4481&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4481&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4481&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4481&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4481&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4481)
--
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-7112] Support correlation variables in HAVING clause in RelToSqlConverter [calcite]
Posted by "nielspardon (via GitHub)" <gi...@apache.org>.
nielspardon commented on code in PR #4481:
URL: https://github.com/apache/calcite/pull/4481#discussion_r2240527824
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -2337,6 +2285,69 @@ Result withExpectedClauses(boolean ignoreClauses,
: new Result(node, clauses, neededAlias, neededType, aliases, anon,
ignoreClauses, ImmutableSet.copyOf(expectedClauses), expectedRel);
}
+
+ /**
+ * A context that uses a select list.
Review Comment:
I had to put a javadoc to satisfy checkstyle. Would love to put something more meaningful here if you have a suggestion. Since I'm not the original author I'm not sure how to best describe the purpose of this class.
--
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-7112] Support correlation variables in HAVING clause in RelToSqlConverter [calcite]
Posted by "nielspardon (via GitHub)" <gi...@apache.org>.
nielspardon commented on code in PR #4481:
URL: https://github.com/apache/calcite/pull/4481#discussion_r2237948301
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1897,7 +1897,7 @@ private Builder builder(RelNode rel, Set<Clause> clauses) {
if (!selectList.equals(SqlNodeList.SINGLETON_STAR)) {
final boolean aliasRef = expectedClauses.contains(Clause.HAVING)
&& dialect.getConformance().isHavingAlias();
- newContext = new Context(dialect, selectList.size()) {
+ newContext = new BaseContext(dialect, selectList.size()) {
Review Comment:
`Context` is also an abstract class. what is happening here is that the previous author decided to instantiate a new instance of `Context` by implementing the abstract methods on the spot which is the syntax you are seeing here: `new Context(dialect, selectList.size()) {}` (note the curly braces)
--
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-7112] Support correlation variables in HAVING clause in RelToSqlConverter [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4481:
URL: https://github.com/apache/calcite/pull/4481#issuecomment-3133559972
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4481) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4481&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4481&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4481&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4481&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4481&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4481)
--
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-7112] Support correlation variables in HAVING clause in RelToSqlConverter [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4481:
URL: https://github.com/apache/calcite/pull/4481#issuecomment-3133466750
BTW: I think you can also squash the commits so we can merge
--
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-7112] Support correlation variables in HAVING clause in RelToSqlConverter [calcite]
Posted by "nielspardon (via GitHub)" <gi...@apache.org>.
nielspardon commented on code in PR #4481:
URL: https://github.com/apache/calcite/pull/4481#discussion_r2238508052
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1897,7 +1897,7 @@ private Builder builder(RelNode rel, Set<Clause> clauses) {
if (!selectList.equals(SqlNodeList.SINGLETON_STAR)) {
final boolean aliasRef = expectedClauses.contains(Clause.HAVING)
&& dialect.getConformance().isHavingAlias();
- newContext = new Context(dialect, selectList.size()) {
+ newContext = new BaseContext(dialect, selectList.size()) {
Review Comment:
sure, I can extract it. would you by any chance have a suggestion for a class name?
--
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-7112] fix UnsupportedOperationException with correlation variable in HAVING clause [calcite]
Posted by "nielspardon (via GitHub)" <gi...@apache.org>.
nielspardon commented on PR #4481:
URL: https://github.com/apache/calcite/pull/4481#issuecomment-3134070156
> BTW: I am afraid that "Support" is not acceptable. You may need to change the comment for the test too. Should have caught this sooner.
no problem. I changed the PR title, Jira issue title and commit message to: `fix UnsupportedOperationException with correlation variable in HAVING clause`
I hope that is acceptable. let me know if I should change it to something else.
--
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-7112] Support correlation variables in HAVING clause in RelToSqlConverter [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4481:
URL: https://github.com/apache/calcite/pull/4481#issuecomment-3134039733
BTW: I am afraid that "Support" is not acceptable.
You may need to change the comment for the test too.
Should have caught this sooner.
--
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-7112] Support correlation variables in HAVING clause in RelToSqlConverter [calcite]
Posted by "nielspardon (via GitHub)" <gi...@apache.org>.
nielspardon commented on code in PR #4481:
URL: https://github.com/apache/calcite/pull/4481#discussion_r2238792028
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1897,7 +1897,7 @@ private Builder builder(RelNode rel, Set<Clause> clauses) {
if (!selectList.equals(SqlNodeList.SINGLETON_STAR)) {
final boolean aliasRef = expectedClauses.contains(Clause.HAVING)
&& dialect.getConformance().isHavingAlias();
- newContext = new Context(dialect, selectList.size()) {
+ newContext = new BaseContext(dialect, selectList.size()) {
Review Comment:
I extracted the anonymous class into a nested class. Let me know if you are good with the name or if you have a better suggestion.
--
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-7112] Support correlation variables in HAVING clause in RelToSqlConverter [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4481:
URL: https://github.com/apache/calcite/pull/4481#issuecomment-3134037870
There is one more problem: your commit message and PR title must match the JIRA issue; can you please bring then in sync?
--
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-7112] Correlation variable in HAVING clause causes UnsupportedOperationException in RelToSql conversion [calcite]
Posted by "nielspardon (via GitHub)" <gi...@apache.org>.
nielspardon commented on PR #4481:
URL: https://github.com/apache/calcite/pull/4481#issuecomment-3134090678
updated everything to what you suggested in Jira
--
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