You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "NobiGo (via GitHub)" <gi...@apache.org> on 2025/02/17 14:18:20 UTC
[PR] [CALCITE-6834] In query that applies COALESCE to nullable SUM, Enumer… [calcite]
NobiGo opened a new pull request, #4201:
URL: https://github.com/apache/calcite/pull/4201
…ableProjectToCalcRule throws AssertionError
--
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-6834] In query that applies COALESCE to nullable SUM, Enumer… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4201:
URL: https://github.com/apache/calcite/pull/4201#discussion_r2010764094
##########
core/src/main/java/org/apache/calcite/rel/rules/ProjectAggregateMergeRule.java:
##########
@@ -154,9 +156,22 @@ && kindCount(project.getProjects(), SqlKind.CASE) == 0) {
builder.push(aggregate.getInput());
builder.aggregate(
builder.groupKey(aggregate.getGroupSet(), aggregate.groupSets), aggCallList);
- builder.project(
- RexPermuteInputsShuttle.of(mapping).visitList(projects2),
- project.getRowType().getFieldNames());
+ // If the output type of Aggregate does not match the input type required by original Project,
+ // CAST needs to be added
+ List<RexNode> rexInputRefs = RexPermuteInputsShuttle.of(mapping).visitList(projects2);
Review Comment:
I imagine the project already contains the cast.
But, in general, the project could have a more complex expression of the aggregate.
Shouldn't the entire expression be preserved?
--
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-6834] In query that applies COALESCE to nullable SUM, Enumer… [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #4201:
URL: https://github.com/apache/calcite/pull/4201#discussion_r1962183633
##########
core/src/test/resources/sql/agg.iq:
##########
@@ -3815,4 +3815,51 @@ select distinct sum(deptno + '1') as deptsum from dept order by 1;
!ok
+# [CALCITE-2192] In query that applies COALESCE to nullable SUM, EnumerableProjectToCalcRule throws AssertionError
Review Comment:
not CALCITE-2192
--
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-6834] In query that applies COALESCE to nullable SUM, Enumer… [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo merged PR #4201:
URL: https://github.com/apache/calcite/pull/4201
--
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-6834] In query that applies COALESCE to nullable SUM, Enumer… [calcite]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4201:
URL: https://github.com/apache/calcite/pull/4201#issuecomment-2746802831
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.
--
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-6834] In query that applies COALESCE to nullable SUM, Enumer… [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #4201:
URL: https://github.com/apache/calcite/pull/4201#discussion_r1962179743
##########
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##########
@@ -6925,6 +6925,13 @@ private HepProgram getTransitiveProgram() {
.check();
}
+ @Test void testProjectAggregateMergeSum01() {
Review Comment:
Add a hyperlink to jira case. It will help explain what problem the test case is solving.
--
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-6834] In query that applies COALESCE to nullable SUM, Enumer… [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #4201:
URL: https://github.com/apache/calcite/pull/4201#discussion_r1962178573
##########
core/src/main/java/org/apache/calcite/rel/rules/ProjectAggregateMergeRule.java:
##########
@@ -154,8 +156,22 @@ && kindCount(project.getProjects(), SqlKind.CASE) == 0) {
builder.push(aggregate.getInput());
builder.aggregate(
builder.groupKey(aggregate.getGroupSet(), aggregate.groupSets), aggCallList);
- builder.project(
- RexPermuteInputsShuttle.of(mapping).visitList(projects2));
+ // If the output type of Aggregate does not match the input type required by original Project,
+ // CAST needs to be added
+ List<RexNode> rexInputRefs = RexPermuteInputsShuttle.of(mapping).visitList(projects2);
+ final ImmutableList.Builder<RexNode> selectList = ImmutableList.builder();
+ final RexShuttle rexShuttle = new RexShuttle() {
+ @Override public RexNode visitInputRef(RexInputRef inputRef) {
+ return builder.getRexBuilder().ensureType(
+ inputRef.getType(),
+ builder.fields().get(inputRef.getIndex()),
+ true);
+ }
+ };
+ for (RexNode rexInputRef : rexInputRefs) {
+ selectList.add(rexInputRef.accept(rexShuttle));
+ }
+ builder.project(selectList.build());
Review Comment:
I think you can achieve the same effect with one line of code: use `RelBuilder.cast` to convert the output row type of `aggregate` to what `project` expects it to be. If `cast` adds a `CAST` call then `project` will flatten the expression on top of 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-6834] In query that applies COALESCE to nullable SUM, Enumer… [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4201:
URL: https://github.com/apache/calcite/pull/4201#issuecomment-2667528106
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4201) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4201&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4201&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4201&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4201&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4201&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4201)
--
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-6834] In query that applies COALESCE to nullable SUM, Enumer… [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #4201:
URL: https://github.com/apache/calcite/pull/4201#discussion_r2013894178
##########
core/src/main/java/org/apache/calcite/rel/rules/ProjectAggregateMergeRule.java:
##########
@@ -154,9 +156,22 @@ && kindCount(project.getProjects(), SqlKind.CASE) == 0) {
builder.push(aggregate.getInput());
builder.aggregate(
builder.groupKey(aggregate.getGroupSet(), aggregate.groupSets), aggCallList);
- builder.project(
- RexPermuteInputsShuttle.of(mapping).visitList(projects2),
- project.getRowType().getFieldNames());
+ // If the output type of Aggregate does not match the input type required by original Project,
+ // CAST needs to be added
+ List<RexNode> rexInputRefs = RexPermuteInputsShuttle.of(mapping).visitList(projects2);
Review Comment:
I re-examined the root cause of this ISSUE. It lies in the fact that after adding SUM0(), the referenced type is indeed CAST type, which should be the return type of SUM0(). After performing type conversion in the final Project, it becomes much simpler. Therefore, I made corresponding changes. Please review again.
--
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-6834] In query that applies COALESCE to nullable SUM, Enumer… [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #4201:
URL: https://github.com/apache/calcite/pull/4201#discussion_r2011560596
##########
core/src/main/java/org/apache/calcite/rel/rules/ProjectAggregateMergeRule.java:
##########
@@ -154,9 +156,22 @@ && kindCount(project.getProjects(), SqlKind.CASE) == 0) {
builder.push(aggregate.getInput());
builder.aggregate(
builder.groupKey(aggregate.getGroupSet(), aggregate.groupSets), aggCallList);
- builder.project(
- RexPermuteInputsShuttle.of(mapping).visitList(projects2),
- project.getRowType().getFieldNames());
+ // If the output type of Aggregate does not match the input type required by original Project,
+ // CAST needs to be added
+ List<RexNode> rexInputRefs = RexPermuteInputsShuttle.of(mapping).visitList(projects2);
Review Comment:
Here, we convert `CASE(IS NOT NULL($0), CAST($0):INTEGER NOT NULL, 0)` into `SUM0(x)` when `$0` is `SUM(x)`.
I attempted to directly define the return type of `SUM0(x)` as `INTEGER` when constructing it to be consistent with the output type of CASE. However, this was checked in the Aggregate's create method. When the parameter type is TINYINT, the result of SUM0(x) should also be TINYINT other throws exception. Therefore, this approach doesn't work.
When the returned type of the constructed SUM0(x) is TINYINT, not in line with the original plan. Therefore, in the PR, I converted the final type of `SUM0(x)` to be consistent with `CAST`.
Why don't I directly use `CAST($0):INTEGER NOT NULL` as the parameter for constructing the Aggregate? From this AggregateCall#create API, the parameter is an ordinals, so we need to construct a Project containing `CAST($0):INTEGER NOT NULL`. This approach will make the problem more complicated.
--
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-6834] In query that applies COALESCE to nullable SUM, Enumer… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4201:
URL: https://github.com/apache/calcite/pull/4201#discussion_r2012523843
##########
core/src/main/java/org/apache/calcite/rel/rules/ProjectAggregateMergeRule.java:
##########
@@ -154,9 +156,22 @@ && kindCount(project.getProjects(), SqlKind.CASE) == 0) {
builder.push(aggregate.getInput());
builder.aggregate(
builder.groupKey(aggregate.getGroupSet(), aggregate.groupSets), aggCallList);
- builder.project(
- RexPermuteInputsShuttle.of(mapping).visitList(projects2),
- project.getRowType().getFieldNames());
+ // If the output type of Aggregate does not match the input type required by original Project,
+ // CAST needs to be added
+ List<RexNode> rexInputRefs = RexPermuteInputsShuttle.of(mapping).visitList(projects2);
Review Comment:
you should not depend on a particular type checking strategy for aggregates, that can be changed as part of the type system.
--
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-6834] In query that applies COALESCE to nullable SUM, Enumer… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4201:
URL: https://github.com/apache/calcite/pull/4201#discussion_r2012526900
##########
core/src/main/java/org/apache/calcite/rel/rules/ProjectAggregateMergeRule.java:
##########
@@ -154,9 +156,22 @@ && kindCount(project.getProjects(), SqlKind.CASE) == 0) {
builder.push(aggregate.getInput());
builder.aggregate(
builder.groupKey(aggregate.getGroupSet(), aggregate.groupSets), aggCallList);
- builder.project(
- RexPermuteInputsShuttle.of(mapping).visitList(projects2),
- project.getRowType().getFieldNames());
+ // If the output type of Aggregate does not match the input type required by original Project,
+ // CAST needs to be added
+ List<RexNode> rexInputRefs = RexPermuteInputsShuttle.of(mapping).visitList(projects2);
Review Comment:
in other words, what you have done is the only thing reasonable
--
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-6834] In query that applies COALESCE to nullable SUM, Enumer… [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on PR #4201:
URL: https://github.com/apache/calcite/pull/4201#issuecomment-2747769032
@julianhyde @mihaibudiu Could you please review this PR? Then we can move forward with 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-6834] In query that applies COALESCE to nullable SUM, Enumer… [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #4201:
URL: https://github.com/apache/calcite/pull/4201#discussion_r1965351734
##########
core/src/main/java/org/apache/calcite/rel/rules/ProjectAggregateMergeRule.java:
##########
@@ -154,8 +156,22 @@ && kindCount(project.getProjects(), SqlKind.CASE) == 0) {
builder.push(aggregate.getInput());
builder.aggregate(
builder.groupKey(aggregate.getGroupSet(), aggregate.groupSets), aggCallList);
- builder.project(
- RexPermuteInputsShuttle.of(mapping).visitList(projects2));
+ // If the output type of Aggregate does not match the input type required by original Project,
+ // CAST needs to be added
+ List<RexNode> rexInputRefs = RexPermuteInputsShuttle.of(mapping).visitList(projects2);
+ final ImmutableList.Builder<RexNode> selectList = ImmutableList.builder();
+ final RexShuttle rexShuttle = new RexShuttle() {
+ @Override public RexNode visitInputRef(RexInputRef inputRef) {
+ return builder.getRexBuilder().ensureType(
+ inputRef.getType(),
+ builder.fields().get(inputRef.getIndex()),
+ true);
+ }
+ };
+ for (RexNode rexInputRef : rexInputRefs) {
+ selectList.add(rexInputRef.accept(rexShuttle));
+ }
+ builder.project(selectList.build());
Review Comment:
What you are referring to here should be `RelBuilder.convert`, which also requires us to build a new RowType based on Mapping, so I have not made any changes here.
--
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-6834] In query that applies COALESCE to nullable SUM, Enumer… [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4201:
URL: https://github.com/apache/calcite/pull/4201#discussion_r2012533936
##########
core/src/main/java/org/apache/calcite/rel/rules/ProjectAggregateMergeRule.java:
##########
@@ -154,9 +156,22 @@ && kindCount(project.getProjects(), SqlKind.CASE) == 0) {
builder.push(aggregate.getInput());
builder.aggregate(
builder.groupKey(aggregate.getGroupSet(), aggregate.groupSets), aggCallList);
- builder.project(
- RexPermuteInputsShuttle.of(mapping).visitList(projects2),
- project.getRowType().getFieldNames());
+ // If the output type of Aggregate does not match the input type required by original Project,
+ // CAST needs to be added
+ List<RexNode> rexInputRefs = RexPermuteInputsShuttle.of(mapping).visitList(projects2);
Review Comment:
I guess my question was whether at this point the selectList only contains RexInputRef objects.
--
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-6834] In query that applies COALESCE to nullable SUM, Enumer… [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #4201:
URL: https://github.com/apache/calcite/pull/4201#discussion_r2013242996
##########
core/src/main/java/org/apache/calcite/rel/rules/ProjectAggregateMergeRule.java:
##########
@@ -154,9 +156,22 @@ && kindCount(project.getProjects(), SqlKind.CASE) == 0) {
builder.push(aggregate.getInput());
builder.aggregate(
builder.groupKey(aggregate.getGroupSet(), aggregate.groupSets), aggCallList);
- builder.project(
- RexPermuteInputsShuttle.of(mapping).visitList(projects2),
- project.getRowType().getFieldNames());
+ // If the output type of Aggregate does not match the input type required by original Project,
+ // CAST needs to be added
+ List<RexNode> rexInputRefs = RexPermuteInputsShuttle.of(mapping).visitList(projects2);
Review Comment:
This is uncertain. Maybe this naming is a bit confusing. Here, we only focus on changing RexInputRef.
--
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