You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "JiajunBernoulli (via GitHub)" <gi...@apache.org> on 2023/01/29 09:50:05 UTC
[GitHub] [calcite] JiajunBernoulli opened a new pull request, #3052: [CALCITE-5506] RelToSqlConverter get error result because RelFieldTri…
JiajunBernoulli opened a new pull request, #3052:
URL: https://github.com/apache/calcite/pull/3052
…mmer lost aggregate function
--
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
[GitHub] [calcite] libenchao commented on a diff in pull request #3052: [CALCITE-5506] RelToSqlConverter should retain the aggregation logic
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao commented on code in PR #3052:
URL: https://github.com/apache/calcite/pull/3052#discussion_r1183809617
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1831,6 +1831,21 @@ && hasSortByOrdinal()) {
return true;
}
+ if (rel instanceof Project) {
+ Project project = (Project) rel;
+ RelNode input = project.getInput();
+ // Cannot merge because "select 1 from t"
+ // is different from "select 1 from (select count(1) from t)"
+ final boolean hasInputRef = project.getProjects()
+ .stream()
+ .anyMatch(rex -> RexUtil.containsInputRef(rex));
+ final boolean hasAggregate =
+ input instanceof Aggregate && input.getRowType().getFieldCount() > 0;
Review Comment:
But the aggregate in your test actually has 1 column, will it still pass without this change?
--
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
[GitHub] [calcite] JiajunBernoulli commented on a diff in pull request #3052: [CALCITE-5506] RelToSqlConverter should retain the aggregation logic
Posted by "JiajunBernoulli (via GitHub)" <gi...@apache.org>.
JiajunBernoulli commented on code in PR #3052:
URL: https://github.com/apache/calcite/pull/3052#discussion_r1183467850
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1831,6 +1831,21 @@ && hasSortByOrdinal()) {
return true;
}
+ if (rel instanceof Project) {
+ Project project = (Project) rel;
+ RelNode input = project.getInput();
+ // Cannot merge because "select 1 from t"
+ // is different from "select 1 from (select count(1) from t)"
+ final boolean hasInputRef = project.getProjects()
+ .stream()
+ .anyMatch(rex -> RexUtil.containsInputRef(rex));
+ final boolean hasAggregate =
+ input instanceof Aggregate && input.getRowType().getFieldCount() > 0;
Review Comment:
Yes, here is the RelNode which has 0 field.
--
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
[GitHub] [calcite] libenchao commented on a diff in pull request #3052: [CALCITE-5506] RelToSqlConverter should retain the aggregation logic
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao commented on code in PR #3052:
URL: https://github.com/apache/calcite/pull/3052#discussion_r1183442969
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1831,6 +1831,21 @@ && hasSortByOrdinal()) {
return true;
}
+ if (rel instanceof Project) {
+ Project project = (Project) rel;
+ RelNode input = project.getInput();
+ // Cannot merge because "select 1 from t"
+ // is different from "select 1 from (select count(1) from t)"
+ final boolean hasInputRef = project.getProjects()
+ .stream()
+ .anyMatch(rex -> RexUtil.containsInputRef(rex));
+ final boolean hasAggregate =
+ input instanceof Aggregate && input.getRowType().getFieldCount() > 0;
Review Comment:
Do you need to add `input.getRowType().getFieldCount() > 0`? (I even doubt that we will produce `RelNode` which has `0` fields)
--
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
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3052: [CALCITE-5506] RelToSqlConverter get error result because RelFieldTri…
Posted by sonarcloud.
sonarcloud[bot] commented on PR #3052:
URL: https://github.com/apache/calcite/pull/3052#issuecomment-1407618359
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3052)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3052&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3052&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3052&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3052&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3052&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3052&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3052&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [calcite] libenchao commented on a diff in pull request #3052: [CALCITE-5506] RelToSqlConverter should retain the aggregation logic
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao commented on code in PR #3052:
URL: https://github.com/apache/calcite/pull/3052#discussion_r1184848806
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1831,6 +1831,21 @@ && hasSortByOrdinal()) {
return true;
}
+ if (rel instanceof Project) {
+ Project project = (Project) rel;
+ RelNode input = project.getInput();
+ // Cannot merge because "select 1 from t"
+ // is different from "select 1 from (select count(1) from t)"
+ final boolean hasInputRef = project.getProjects()
+ .stream()
+ .anyMatch(rex -> RexUtil.containsInputRef(rex));
+ final boolean hasAggregate =
+ input instanceof Aggregate && input.getRowType().getFieldCount() > 0;
Review Comment:
Sorry that I misunderstood the attention of this condition. Now I think I get your point, and it looks correct (although it may be a little coarse grained, it's ok to me because we have `hasInputRef` to narrow down the impact)
--
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
[GitHub] [calcite] libenchao merged pull request #3052: [CALCITE-5506] RelToSqlConverter should retain the aggregation logic
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao merged PR #3052:
URL: https://github.com/apache/calcite/pull/3052
--
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
[GitHub] [calcite] JiajunBernoulli commented on a diff in pull request #3052: [CALCITE-5506] RelToSqlConverter should retain the aggregation logic
Posted by "JiajunBernoulli (via GitHub)" <gi...@apache.org>.
JiajunBernoulli commented on code in PR #3052:
URL: https://github.com/apache/calcite/pull/3052#discussion_r1184769528
##########
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java:
##########
@@ -1831,6 +1831,21 @@ && hasSortByOrdinal()) {
return true;
}
+ if (rel instanceof Project) {
+ Project project = (Project) rel;
+ RelNode input = project.getInput();
+ // Cannot merge because "select 1 from t"
+ // is different from "select 1 from (select count(1) from t)"
+ final boolean hasInputRef = project.getProjects()
+ .stream()
+ .anyMatch(rex -> RexUtil.containsInputRef(rex));
+ final boolean hasAggregate =
+ input instanceof Aggregate && input.getRowType().getFieldCount() > 0;
Review Comment:
No, it failed.
```
java.lang.AssertionError:
Expected: is "SELECT 42 AS \"C\"\nFROM \"foodmart\".\"product\"\nGROUP BY ()"
but: was "SELECT 42 AS \"C\"\nFROM (SELECT\nFROM \"foodmart\".\"product\"\nGROUP BY ()) AS \"t\""
```
--
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
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3052: [CALCITE-5506] RelToSqlConverter should retain the aggregation logic
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3052:
URL: https://github.com/apache/calcite/pull/3052#issuecomment-1532436443
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3052)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3052&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3052&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3052&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3052&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3052&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3052&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3052&metric=new_duplicated_lines_density&view=list)
--
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
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3052: [CALCITE-5506] RelToSqlConverter should retain the aggregation logic
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3052:
URL: https://github.com/apache/calcite/pull/3052#issuecomment-1532437140
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3052)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3052&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3052&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3052&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3052&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3052&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3052&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3052&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3052&metric=new_duplicated_lines_density&view=list)
--
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