You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2023/01/20 06:56:35 UTC
[GitHub] [calcite] libenchao opened a new pull request, #3038: [CALCITE-5483] ProjectAggregateMergeRule throws exception if literal is non-numeric
libenchao opened a new pull request, #3038:
URL: https://github.com/apache/calcite/pull/3038
This PR is based on https://github.com/apache/calcite/pull/3031 from @birschick-bq
--
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 #3038: [CALCITE-5483] ProjectAggregateMergeRule throws exception if literal is non-numeric
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on PR #3038:
URL: https://github.com/apache/calcite/pull/3038#issuecomment-1398008354
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3038)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3038&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3038&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3038&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3038&metric=new_coverage&view=list) [75.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3038&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3038&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3038&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 #3038: [CALCITE-5483] ProjectAggregateMergeRule throws exception if literal is non-numeric
Posted by sonarcloud.
sonarcloud[bot] commented on PR #3038:
URL: https://github.com/apache/calcite/pull/3038#issuecomment-1404520263
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3038)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3038&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3038&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3038&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3038&metric=new_coverage&view=list) [83.3% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3038&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3038&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3038&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 #3038: [CALCITE-5483] ProjectAggregateMergeRule throws exception if literal is non-numeric
Posted by sonarcloud.
sonarcloud[bot] commented on PR #3038:
URL: https://github.com/apache/calcite/pull/3038#issuecomment-1404519065
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3038)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3038&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3038&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3038&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3038&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3038&metric=new_coverage&view=list) [83.3% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3038&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3038&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3038&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 closed pull request #3038: [CALCITE-5483] ProjectAggregateMergeRule throws exception if literal is non-numeric
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao closed pull request #3038: [CALCITE-5483] ProjectAggregateMergeRule throws exception if literal is non-numeric
URL: https://github.com/apache/calcite/pull/3038
--
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] birschick-bq commented on a diff in pull request #3038: [CALCITE-5483] ProjectAggregateMergeRule throws exception if literal is non-numeric
Posted by "birschick-bq (via GitHub)" <gi...@apache.org>.
birschick-bq commented on code in PR #3038:
URL: https://github.com/apache/calcite/pull/3038#discussion_r1082986380
##########
core/src/main/java/org/apache/calcite/rel/rules/ProjectAggregateMergeRule.java:
##########
@@ -99,25 +100,27 @@ && kindCount(project.getProjects(), SqlKind.CASE) == 0) {
&& operands.get(1).getKind() == SqlKind.CAST
&& ((RexCall) operands.get(1)).operands.get(0).getKind()
== SqlKind.INPUT_REF
- && operands.get(2).getKind() == SqlKind.LITERAL) {
+ && operands.get(2).getKind() == SqlKind.LITERAL
+ && operands.get(2).getType().getFamily() == SqlTypeFamily.NUMERIC) {
Review Comment:
Optionally, you could move this test (`operands.get(2).getType().getFamily() == SqlTypeFamily.NUMERIC`) to just before the call to `literal.getValueAs(BigDecimal.class)` - it might have better 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
[GitHub] [calcite] libenchao commented on pull request #3038: [CALCITE-5483] ProjectAggregateMergeRule throws exception if literal is non-numeric
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao commented on PR #3038:
URL: https://github.com/apache/calcite/pull/3038#issuecomment-1404635280
@birschick-bq Thanks for your review.
I'll merge this after '1.33.0' is out if there is no more objections anymore. And I'll add @birschick-bq as the co-author since this PR is mostly based on his https://github.com/apache/calcite/pull/3031.
--
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 #3038: [CALCITE-5483] ProjectAggregateMergeRule throws exception if literal is non-numeric
Posted by "libenchao (via GitHub)" <gi...@apache.org>.
libenchao commented on code in PR #3038:
URL: https://github.com/apache/calcite/pull/3038#discussion_r1087394193
##########
core/src/main/java/org/apache/calcite/rel/rules/ProjectAggregateMergeRule.java:
##########
@@ -99,25 +100,27 @@ && kindCount(project.getProjects(), SqlKind.CASE) == 0) {
&& operands.get(1).getKind() == SqlKind.CAST
&& ((RexCall) operands.get(1)).operands.get(0).getKind()
== SqlKind.INPUT_REF
- && operands.get(2).getKind() == SqlKind.LITERAL) {
+ && operands.get(2).getKind() == SqlKind.LITERAL
+ && operands.get(2).getType().getFamily() == SqlTypeFamily.NUMERIC) {
Review Comment:
I was meant to remove this check originally since we can assume the operand is numeric when the aggregator is `SUM`. Sorry that this was still in my git staging.
--
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