You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "xiedeyantu (via GitHub)" <gi...@apache.org> on 2025/04/15 10:36:34 UTC
[PR] [CALCITE-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
xiedeyantu opened a new pull request, #4308:
URL: https://github.com/apache/calcite/pull/4308
Please see: [CALCITE-6955](https://issues.apache.org/jira/browse/CALCITE-6955)
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #4308:
URL: https://github.com/apache/calcite/pull/4308#issuecomment-2807744824
> Frankly, this would be very amenable to having an end-to-end test showing incorrect results before. This could be done easily after we merge #4301
I totally agree with you. If some real data may be exposed.
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4308:
URL: https://github.com/apache/calcite/pull/4308#discussion_r2045676118
##########
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##########
@@ -406,6 +406,9 @@ public interface UnionEmptyPruneRuleConfig extends PruneEmptyRule.Config {
+ RelOptUtil.toString(union);
if (nonEmptyInputs == 0) {
relBuilder.push(union).empty();
+ } else if (nonEmptyInputs == 1 && !union.all) {
Review Comment:
If intersect contains two children, one is values(1) and the other is values(), then the result will be an empty set. This is another logic, and the original implementation is as expected.
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu closed pull request #4308: [CALCITE-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly
URL: https://github.com/apache/calcite/pull/4308
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4308:
URL: https://github.com/apache/calcite/pull/4308#discussion_r2048034559
##########
core/src/test/resources/sql/set-op.iq:
##########
@@ -258,4 +258,32 @@ UNION
(2 rows)
!ok
+
+# [CALCITE-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly
+select * from (values (10, 1), (30, 3), (30, 3)) as t (x, y)
+union
+select * from (values (20, 2)) as t (x, y) where x > 30;
++----+---+
+| X | Y |
++----+---+
+| 10 | 1 |
+| 30 | 3 |
++----+---+
+(2 rows)
+
Review Comment:
The corresponding plan is already present in RelOptRulesTest.xml. Would you like me to add an additional one here as well.
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4308:
URL: https://github.com/apache/calcite/pull/4308#issuecomment-2807738429
Frankly, this would be very amenable to having an end-to-end test showing incorrect results before.
This could be done easily after we merge https://github.com/apache/calcite/pull/4301
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4308:
URL: https://github.com/apache/calcite/pull/4308#discussion_r2048055583
##########
core/src/test/resources/sql/set-op.iq:
##########
@@ -258,4 +258,32 @@ UNION
(2 rows)
!ok
+
+# [CALCITE-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly
+select * from (values (10, 1), (30, 3), (30, 3)) as t (x, y)
+union
+select * from (values (20, 2)) as t (x, y) where x > 30;
++----+---+
+| X | Y |
++----+---+
+| 10 | 1 |
+| 30 | 3 |
++----+---+
+(2 rows)
+
Review Comment:
@NobiGo 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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #4308:
URL: https://github.com/apache/calcite/pull/4308#discussion_r2048038107
##########
core/src/test/resources/sql/set-op.iq:
##########
@@ -258,4 +258,32 @@ UNION
(2 rows)
!ok
+
+# [CALCITE-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly
+select * from (values (10, 1), (30, 3), (30, 3)) as t (x, y)
+union
+select * from (values (20, 2)) as t (x, y) where x > 30;
++----+---+
+| X | Y |
++----+---+
+| 10 | 1 |
+| 30 | 3 |
++----+---+
+(2 rows)
+
Review Comment:
If this plan doesn't take effect, this SQL statement should still be able to execute successfully.
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4308:
URL: https://github.com/apache/calcite/pull/4308#discussion_r2048041740
##########
core/src/test/resources/sql/set-op.iq:
##########
@@ -258,4 +258,32 @@ UNION
(2 rows)
!ok
+
+# [CALCITE-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly
+select * from (values (10, 1), (30, 3), (30, 3)) as t (x, y)
+union
+select * from (values (20, 2)) as t (x, y) where x > 30;
++----+---+
+| X | Y |
++----+---+
+| 10 | 1 |
+| 30 | 3 |
++----+---+
+(2 rows)
+
Review Comment:
Oops! You're completely right. I'll add it soon.
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo merged PR #4308:
URL: https://github.com/apache/calcite/pull/4308
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4308:
URL: https://github.com/apache/calcite/pull/4308#issuecomment-2804639111
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4308) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4308&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4308&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4308&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [78.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4308&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4308&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4308)
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4308:
URL: https://github.com/apache/calcite/pull/4308#discussion_r2045687062
##########
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##########
@@ -406,6 +406,9 @@ public interface UnionEmptyPruneRuleConfig extends PruneEmptyRule.Config {
+ RelOptUtil.toString(union);
if (nonEmptyInputs == 0) {
relBuilder.push(union).empty();
+ } else if (nonEmptyInputs == 1 && !union.all) {
Review Comment:
Yes, this rule requires matching an empty value.
```
UnionEmptyPruneRuleConfig DEFAULT = ImmutableUnionEmptyPruneRuleConfig.of()
.withOperandSupplier(b0 ->
b0.operand(Union.class).unorderedInputs(b1 ->
b1.operand(Values.class)
.predicate(Values::isEmpty).noInputs()))
.withDescription("Union");
```
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #4308:
URL: https://github.com/apache/calcite/pull/4308#issuecomment-2808236177
@NobiGo I've added the `.iq` test cases.
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4308:
URL: https://github.com/apache/calcite/pull/4308#issuecomment-2808269879
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4308) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4308&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4308&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4308&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [78.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4308&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4308&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4308)
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4308:
URL: https://github.com/apache/calcite/pull/4308#discussion_r2045680490
##########
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##########
@@ -406,6 +406,9 @@ public interface UnionEmptyPruneRuleConfig extends PruneEmptyRule.Config {
+ RelOptUtil.toString(union);
if (nonEmptyInputs == 0) {
relBuilder.push(union).empty();
+ } else if (nonEmptyInputs == 1 && !union.all) {
Review Comment:
that is only true if the number of empty inputs is > 0
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4308:
URL: https://github.com/apache/calcite/pull/4308#discussion_r2045122578
##########
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##########
@@ -406,6 +406,9 @@ public interface UnionEmptyPruneRuleConfig extends PruneEmptyRule.Config {
+ RelOptUtil.toString(union);
if (nonEmptyInputs == 0) {
relBuilder.push(union).empty();
+ } else if (nonEmptyInputs == 1 && !union.all) {
Review Comment:
how about intersect?
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4308:
URL: https://github.com/apache/calcite/pull/4308#issuecomment-2811562008
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4308) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4308&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4308&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4308&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [78.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4308&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4308&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4308)
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #4308:
URL: https://github.com/apache/calcite/pull/4308#issuecomment-2811544021
> @xiedeyantu Please squash the commit info.
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #4308:
URL: https://github.com/apache/calcite/pull/4308#discussion_r2048031133
##########
core/src/test/resources/sql/set-op.iq:
##########
@@ -258,4 +258,32 @@ UNION
(2 rows)
!ok
+
+# [CALCITE-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly
+select * from (values (10, 1), (30, 3), (30, 3)) as t (x, y)
+union
+select * from (values (20, 2)) as t (x, y) where x > 30;
++----+---+
+| X | Y |
++----+---+
+| 10 | 1 |
+| 30 | 3 |
++----+---+
+(2 rows)
+
Review Comment:
We need to verify whether the optimizations related to this PR have taken effect through `!plan`.
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4308:
URL: https://github.com/apache/calcite/pull/4308#issuecomment-2811519617
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4308) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4308&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4308&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4308&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [78.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4308&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4308&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4308)
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4308:
URL: https://github.com/apache/calcite/pull/4308#issuecomment-2808279198
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4308) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4308&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4308&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4308&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [78.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4308&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4308&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4308)
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #4308:
URL: https://github.com/apache/calcite/pull/4308#issuecomment-2810401685
Waiting for @NobiGo to confirm that he's satisfied
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #4308:
URL: https://github.com/apache/calcite/pull/4308#discussion_r2045791580
##########
core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml:
##########
@@ -3768,6 +3796,22 @@ LogicalIntersect(all=[false])
<Resource name="planAfter">
<![CDATA[
LogicalValues(tuples=[[]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testEmptyIntersect2">
Review Comment:
This test case does not have a corresponding one in Java.
##########
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##########
@@ -406,6 +406,9 @@ public interface UnionEmptyPruneRuleConfig extends PruneEmptyRule.Config {
+ RelOptUtil.toString(union);
if (nonEmptyInputs == 0) {
relBuilder.push(union).empty();
+ } else if (nonEmptyInputs == 1 && !union.all) {
Review Comment:
> If intersect contains two children, one is values(1) and the other is values(), then the result will be an empty set. This is another logic, and the original implementation is as expected.
Do we have test cases covering this?
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on code in PR #4308:
URL: https://github.com/apache/calcite/pull/4308#discussion_r2048037576
##########
core/src/test/resources/sql/set-op.iq:
##########
@@ -258,4 +258,32 @@ UNION
(2 rows)
!ok
+
+# [CALCITE-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly
+select * from (values (10, 1), (30, 3), (30, 3)) as t (x, y)
+union
+select * from (values (20, 2)) as t (x, y) where x > 30;
++----+---+
+| X | Y |
++----+---+
+| 10 | 1 |
+| 30 | 3 |
++----+---+
+(2 rows)
+
Review Comment:
Yes, this is different. In RelOptRulesTest, we have actively added the plan. For the iq file, we are using the default optimization plan.
--
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4308:
URL: https://github.com/apache/calcite/pull/4308#discussion_r2046024250
##########
core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java:
##########
@@ -406,6 +406,9 @@ public interface UnionEmptyPruneRuleConfig extends PruneEmptyRule.Config {
+ RelOptUtil.toString(union);
if (nonEmptyInputs == 0) {
relBuilder.push(union).empty();
+ } else if (nonEmptyInputs == 1 && !union.all) {
Review Comment:
testEmptyIntersect can cover this 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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4308:
URL: https://github.com/apache/calcite/pull/4308#discussion_r2046022034
##########
core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml:
##########
@@ -3768,6 +3796,22 @@ LogicalIntersect(all=[false])
<Resource name="planAfter">
<![CDATA[
LogicalValues(tuples=[[]])
+]]>
+ </Resource>
+ </TestCase>
+ <TestCase name="testEmptyIntersect2">
Review Comment:
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-6955] PruneEmptyRules does not handle the all attribute of SetOp correctly [calcite]
Posted by "NobiGo (via GitHub)" <gi...@apache.org>.
NobiGo commented on PR #4308:
URL: https://github.com/apache/calcite/pull/4308#issuecomment-2811528897
@xiedeyantu Please squash the commit info.
--
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