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 2026/04/11 15:23:14 UTC
[PR] [CALCITE-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
xiedeyantu opened a new pull request, #4878:
URL: https://github.com/apache/calcite/pull/4878
<!--
Thanks for sending a pull request! Here are some critical tips for you:
1. READ THE GUIDE FIRST: https://calcite.apache.org/develop/#contributing
*For significant contributions, please discuss on the dev mailing list or Jira BEFORE coding.*
2. JIRA IS USUALLY MANDATORY: Ensure you have created an issue on the Calcite Jira:
https://issues.apache.org/jira/projects/CALCITE/issues
*Check existing issues first to avoid duplicates.*
*Note: A Jira is NOT required for typos and cosmetic changes (i.e., changes that are neither bugs nor features).*
3. TIMING: Strongly recommended to create the Jira BEFORE you start writing code (e.g., a day or so before posting a PR).
This gives others a chance to weigh in on your specification.
4. CRITICAL CONSISTENCY RULE
The following three items MUST match exactly in wording and meaning:
(A) The Jira Issue Title
(B) This Pull Request Title
(C) Your Git Commit Message
Format: [CALCITE-XXXX] <Description>
Example: [CALCITE-0000] Add IF NOT EXISTS clause to CREATE TABLE
Guidelines for a good Title:
- Illustrate using SQL keywords (in ALL-CAPS) rather than Java method names.
- Focus on the specification and user experience, not the implementation.
- Make it clear whether it is a bug or a feature.
- Use words like "should" to indicate desired behavior vs. current behavior (e.g., "Validator should not close model file").
5. REPRODUCTION: If fixing a bug, please provide a concise SQL example or test case to reproduce the issue for a faster review.
6. TESTING: Ensure `./gradlew build` passes and appropriate tests are added/updated.
-->
## Jira Link
[CALCITE-7463](https://issues.apache.org/jira/browse/CALCITE-7463)
## Changes Proposed
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR.
-->
The UnionToFilterRule produces incorrect results when applied to inputs that contain LIMIT.
Specifically, the rule incorrectly collapses:
```
(SELECT mgr, comm FROM emp LIMIT 2)
UNION
(SELECT mgr, comm FROM emp LIMIT 2)
```
into:
```
SELECT DISTINCT mgr, comm FROM emp LIMIT 2
```
This transformation is not semantically equivalent.
Reproduction
```
(SELECT mgr, comm FROM emp LIMIT 2)
UNION
(SELECT mgr, comm FROM emp LIMIT 2)
```
Plan Before
```
LogicalUnion(all=[false])
LogicalSort(fetch=[2])
LogicalProject(MGR=[$3], COMM=[$6])
LogicalTableScan(table=[[CATALOG, SALES, EMP]])
LogicalSort(fetch=[2])
LogicalProject(MGR=[$3], COMM=[$6])
LogicalTableScan(table=[[CATALOG, SALES, EMP]])
```
Plan After (Incorrect)
```
LogicalAggregate(group=[{0, 1}])
LogicalSort(fetch=[2])
LogicalProject(MGR=[$3], COMM=[$6])
LogicalTableScan(table=[[CATALOG, SALES, EMP]])
```
Expected Behavior
The transformation should NOT be applied when any input of UNION contains LogicalSort(That contains ORDER BY, LIMIT, OFFSET).
--
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-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4878:
URL: https://github.com/apache/calcite/pull/4878#discussion_r3068226821
##########
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##########
@@ -11435,6 +11435,33 @@ private void checkLoptOptimizeJoinRule(LoptOptimizeJoinRule rule) {
.check();
}
+ /** Test case of
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-7463">[CALCITE-7463]
+ * UnionToFilterRule incorrectly rewrites UNION with LIMIT</a>. */
+ @Test void testUnionToFilterRuleWithLimit() {
Review Comment:
Great suggestion! 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-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4878:
URL: https://github.com/apache/calcite/pull/4878#discussion_r3069179368
##########
core/src/main/java/org/apache/calcite/rel/rules/SetOpToFilterRule.java:
##########
@@ -205,13 +207,35 @@ private static RelBuilder buildSetOp(RelBuilder builder, int count, RelNode setO
// Skip non-deterministic conditions or those containing subqueries
return Pair.of(input, null);
}
- return Pair.of(filter.getInput().stripped(), filter.getCondition());
+ final RelNode source = filter.getInput().stripped();
+ if (containsSortInProjectFilterChain(source)) {
+ return Pair.of(input, null);
+ }
+ return Pair.of(source, filter.getCondition());
+ }
+ if (containsSortInProjectFilterChain(input)) {
+ return Pair.of(input, null);
}
// For non-filter inputs, use TRUE literal as default condition.
return Pair.of(input.stripped(),
input.getCluster().getRexBuilder().makeLiteral(true));
}
+ private static boolean containsSortInProjectFilterChain(RelNode input) {
+ RelNode current = input.stripped();
+ while (true) {
+ if (current instanceof Sort) {
+ return true;
Review Comment:
Thanks! It's a great idea! I agree with you.Since `SqlToRelConverter#convertOrder` removes the ORDER BY clause, I used RelBuilder to construct the test case.
##########
core/src/main/java/org/apache/calcite/rel/rules/SetOpToFilterRule.java:
##########
@@ -205,13 +207,35 @@ private static RelBuilder buildSetOp(RelBuilder builder, int count, RelNode setO
// Skip non-deterministic conditions or those containing subqueries
return Pair.of(input, null);
}
- return Pair.of(filter.getInput().stripped(), filter.getCondition());
+ final RelNode source = filter.getInput().stripped();
+ if (containsSortInProjectFilterChain(source)) {
+ return Pair.of(input, null);
+ }
+ return Pair.of(source, filter.getCondition());
+ }
+ if (containsSortInProjectFilterChain(input)) {
+ return Pair.of(input, null);
}
// For non-filter inputs, use TRUE literal as default condition.
return Pair.of(input.stripped(),
input.getCluster().getRexBuilder().makeLiteral(true));
}
+ private static boolean containsSortInProjectFilterChain(RelNode input) {
+ RelNode current = input.stripped();
+ while (true) {
+ if (current instanceof Sort) {
+ return true;
Review Comment:
Thanks! It's a great idea! I agree with you. Since `SqlToRelConverter#convertOrder` removes the ORDER BY clause, I used RelBuilder to construct the test 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-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #4878:
URL: https://github.com/apache/calcite/pull/4878#issuecomment-4229695240
> The fix is correct: returning `(input, null)` when a `Sort` is detected in the input chain prevents the rule from incorrectly collapsing UNION branches that have LIMIT/OFFSET semantics. The helper method is clean and the traversal logic handles arbitrary nesting of Project/Filter above a Sort. Two minor gaps: the method name omits Filter from its description, and there is no test for `UNION ALL` with LIMIT.
@utafrali I'm curious how you managed to review this PR in less than 5 minutes.
--
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-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #4878:
URL: https://github.com/apache/calcite/pull/4878#discussion_r3075127521
##########
core/src/main/java/org/apache/calcite/rel/rules/SetOpToFilterRule.java:
##########
@@ -143,7 +153,14 @@ private static void match(RelOptRuleCall call) {
for (int i = 1; i < inputs.size(); i++) {
final RelNode input = inputs.get(i).stripped();
- final Pair<RelNode, @Nullable RexNode> pair = extractSourceAndCond(input);
+ boolean isSorted = false;
+ final List<RelCollation> inputCollations = mq.collations(input);
Review Comment:
As suggested let's bring the discussion in Jira, I will try to summarize there my idea, let's see if it makes sense for everyone, thanks for the code draft @xiedeyantu!
--
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-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4878:
URL: https://github.com/apache/calcite/pull/4878#issuecomment-4447621523
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-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on PR #4878:
URL: https://github.com/apache/calcite/pull/4878#issuecomment-4232891727
> So there is no doubt:
>
> -1
>
> Three of us (me and Steven in Jira, Alessandro in the PR comments) have expressed doubts that this is even a bug.
>
> Let's not merge any fix until we decide what, if anything, needs to be fixed.
@julianhyde Of course, I won't merge it before receiving approval from the committer. The conversation in the JIRA hasn't progressed for several days, so I assumed everyone agreed with this conservative approach. I would also like to thank Julian for the timely reminder. Perhaps I misunderstood. However, I agree with the solution described by Alessandro. This PR can be put on hold for now. Shall we move the discussion back to the 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
Re: [PR] [CALCITE-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on PR #4878:
URL: https://github.com/apache/calcite/pull/4878#issuecomment-4232821149
So there is no doubt:
-1
Three of us (me and Steven in Jira, Alessandro in the PR comments) have expressed doubts that this is even a bug.
Let's not merge any fix until we decide what, if anything, needs to be fixed.
--
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-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4878:
URL: https://github.com/apache/calcite/pull/4878#discussion_r3069409056
##########
core/src/main/java/org/apache/calcite/rel/rules/SetOpToFilterRule.java:
##########
@@ -205,13 +207,35 @@ private static RelBuilder buildSetOp(RelBuilder builder, int count, RelNode setO
// Skip non-deterministic conditions or those containing subqueries
return Pair.of(input, null);
}
- return Pair.of(filter.getInput().stripped(), filter.getCondition());
+ final RelNode source = filter.getInput().stripped();
+ if (containsSortInProjectFilterChain(source)) {
+ return Pair.of(input, null);
+ }
+ return Pair.of(source, filter.getCondition());
+ }
+ if (containsSortInProjectFilterChain(input)) {
+ return Pair.of(input, null);
}
// For non-filter inputs, use TRUE literal as default condition.
return Pair.of(input.stripped(),
input.getCluster().getRexBuilder().makeLiteral(true));
}
+ private static boolean containsSortInProjectFilterChain(RelNode input) {
+ RelNode current = input.stripped();
+ while (true) {
+ if (current instanceof Sort) {
+ return true;
Review Comment:
Of course, I agree with your suggestion; this likely involves the transfer of physical properties, and I will proceed to investigate that area next. I would personally be very grateful to receive any further suggestions. Thank you!
--
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-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
Posted by "utafrali (via GitHub)" <gi...@apache.org>.
utafrali commented on PR #4878:
URL: https://github.com/apache/calcite/pull/4878#issuecomment-4229715900
> > The fix is correct: returning `(input, null)` when a `Sort` is detected in the input chain prevents the rule from incorrectly collapsing UNION branches that have LIMIT/OFFSET semantics. The helper method is clean and the traversal logic handles arbitrary nesting of Project/Filter above a Sort. Two minor gaps: the method name omits Filter from its description, and there is no test for `UNION ALL` with LIMIT.
>
>
>
> @utafrali I'm curious how you managed to review this PR in less than 5 minutes.
I was already following this case closely, so I had the full context in mind. The changes were straightforward, so the review didn’t take long.
--
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-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #4878:
URL: https://github.com/apache/calcite/pull/4878#discussion_r3068460388
##########
core/src/main/java/org/apache/calcite/rel/rules/SetOpToFilterRule.java:
##########
@@ -205,13 +207,35 @@ private static RelBuilder buildSetOp(RelBuilder builder, int count, RelNode setO
// Skip non-deterministic conditions or those containing subqueries
return Pair.of(input, null);
}
- return Pair.of(filter.getInput().stripped(), filter.getCondition());
+ final RelNode source = filter.getInput().stripped();
+ if (containsSortInProjectFilterChain(source)) {
+ return Pair.of(input, null);
+ }
+ return Pair.of(source, filter.getCondition());
+ }
+ if (containsSortInProjectFilterChain(input)) {
+ return Pair.of(input, null);
}
// For non-filter inputs, use TRUE literal as default condition.
return Pair.of(input.stripped(),
input.getCluster().getRexBuilder().makeLiteral(true));
}
+ private static boolean containsSortInProjectFilterChain(RelNode input) {
+ RelNode current = input.stripped();
+ while (true) {
+ if (current instanceof Sort) {
+ return true;
Review Comment:
Shouldn't we also check that there is actually a LIMIT, as you can have a Sort without LIMIT and that rewrite I think is correct
--
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-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4878:
URL: https://github.com/apache/calcite/pull/4878#discussion_r3069554148
##########
core/src/main/java/org/apache/calcite/rel/rules/SetOpToFilterRule.java:
##########
@@ -143,7 +153,14 @@ private static void match(RelOptRuleCall call) {
for (int i = 1; i < inputs.size(); i++) {
final RelNode input = inputs.get(i).stripped();
- final Pair<RelNode, @Nullable RexNode> pair = extractSourceAndCond(input);
+ boolean isSorted = false;
+ final List<RelCollation> inputCollations = mq.collations(input);
Review Comment:
@asolimando I use `mq` to check if the input is sorted. If the input is sorted, a sub-plan that has both limit/offset and sort is safe to be rewritten. Please check if this logic matches your 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-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
Posted by "xiedeyantu (via GitHub)" <gi...@apache.org>.
xiedeyantu commented on code in PR #4878:
URL: https://github.com/apache/calcite/pull/4878#discussion_r3068219488
##########
core/src/main/java/org/apache/calcite/rel/rules/SetOpToFilterRule.java:
##########
@@ -205,13 +207,35 @@ private static RelBuilder buildSetOp(RelBuilder builder, int count, RelNode setO
// Skip non-deterministic conditions or those containing subqueries
return Pair.of(input, null);
}
- return Pair.of(filter.getInput().stripped(), filter.getCondition());
+ final RelNode source = filter.getInput().stripped();
+ if (containsSortInProjectFilterChain(source)) {
+ return Pair.of(input, null);
+ }
+ return Pair.of(source, filter.getCondition());
+ }
+ if (containsSortInProjectFilterChain(input)) {
+ return Pair.of(input, null);
}
// For non-filter inputs, use TRUE literal as default condition.
return Pair.of(input.stripped(),
input.getCluster().getRexBuilder().makeLiteral(true));
}
+ private static boolean containsSortInProjectFilterChain(RelNode input) {
Review Comment:
Thank you for your comments, but I don't think the name you suggested is as fitting as the current one.
--
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-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4878:
URL: https://github.com/apache/calcite/pull/4878#issuecomment-4231166176
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4878) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4878&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4878&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4878&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [96.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4878&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4878&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4878)
--
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-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4878:
URL: https://github.com/apache/calcite/pull/4878#issuecomment-4229702463
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4878) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4878&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4878&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4878&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4878&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4878&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4878)
--
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-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
Posted by "asolimando (via GitHub)" <gi...@apache.org>.
asolimando commented on code in PR #4878:
URL: https://github.com/apache/calcite/pull/4878#discussion_r3069390566
##########
core/src/main/java/org/apache/calcite/rel/rules/SetOpToFilterRule.java:
##########
@@ -205,13 +207,35 @@ private static RelBuilder buildSetOp(RelBuilder builder, int count, RelNode setO
// Skip non-deterministic conditions or those containing subqueries
return Pair.of(input, null);
}
- return Pair.of(filter.getInput().stripped(), filter.getCondition());
+ final RelNode source = filter.getInput().stripped();
+ if (containsSortInProjectFilterChain(source)) {
+ return Pair.of(input, null);
+ }
+ return Pair.of(source, filter.getCondition());
+ }
+ if (containsSortInProjectFilterChain(input)) {
+ return Pair.of(input, null);
}
// For non-filter inputs, use TRUE literal as default condition.
return Pair.of(input.stripped(),
input.getCluster().getRexBuilder().makeLiteral(true));
}
+ private static boolean containsSortInProjectFilterChain(RelNode input) {
+ RelNode current = input.stripped();
+ while (true) {
+ if (current instanceof Sort) {
+ return true;
Review Comment:
I wonder if this couldn't/shouldn't be generalized via a new metadata characterizing what makes this rewrite unsafe (and probably makes potentially other filter pushdowns unsound?
After reading the comments in Jira I am still trying to wrap my head around the problem and I think it's a more general pattern, but I couldn't spend much time thinking it through so I am just sharing my intuition hoping it helps.
I understand the LIMIT 2 unions up to a LIMIT 4 conceptually but it's an "at most 4" and non deterministic as discussed.
So what characterizes here a property of the union operands that makes the rewrite unsafe?
It's a sort+limit it seems, maybe this can become a metadata? Does this apply to other types of pushdowns like filter-sort transpose? Can they all be rewritten in terms of this new metadata?
I don't mean to block this effort until we figure this out. But I think we might have a more general fix.
--
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-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
Posted by "utafrali (via GitHub)" <gi...@apache.org>.
utafrali commented on code in PR #4878:
URL: https://github.com/apache/calcite/pull/4878#discussion_r3068208639
##########
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##########
@@ -11435,6 +11435,33 @@ private void checkLoptOptimizeJoinRule(LoptOptimizeJoinRule rule) {
.check();
}
+ /** Test case of
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-7463">[CALCITE-7463]
+ * UnionToFilterRule incorrectly rewrites UNION with LIMIT</a>. */
+ @Test void testUnionToFilterRuleWithLimit() {
Review Comment:
The two new tests cover `UNION` (distinct) with `LIMIT`, but `UNION ALL` with `LIMIT` is also a broken case and is not tested. The rule applies to both set-op types, so a third test covering `UNION ALL` would close that gap:
```java
@Test void testUnionAllToFilterRuleWithLimit() {
final String sql = "(SELECT mgr, comm FROM emp LIMIT 2)\n"
+ "UNION ALL\n"
+ "(SELECT mgr, comm FROM emp LIMIT 2)\n";
sql(sql)
.withRule(CoreRules.UNION_FILTER_TO_FILTER)
.checkUnchanged();
}
```
##########
core/src/main/java/org/apache/calcite/rel/rules/SetOpToFilterRule.java:
##########
@@ -205,13 +207,35 @@ private static RelBuilder buildSetOp(RelBuilder builder, int count, RelNode setO
// Skip non-deterministic conditions or those containing subqueries
return Pair.of(input, null);
}
- return Pair.of(filter.getInput().stripped(), filter.getCondition());
+ final RelNode source = filter.getInput().stripped();
+ if (containsSortInProjectFilterChain(source)) {
+ return Pair.of(input, null);
+ }
+ return Pair.of(source, filter.getCondition());
+ }
+ if (containsSortInProjectFilterChain(input)) {
+ return Pair.of(input, null);
}
// For non-filter inputs, use TRUE literal as default condition.
return Pair.of(input.stripped(),
input.getCluster().getRexBuilder().makeLiteral(true));
}
+ private static boolean containsSortInProjectFilterChain(RelNode input) {
Review Comment:
The method name `containsSortInProjectFilterChain` is slightly misleading because the while loop also traverses `Filter` nodes (not just `Project`). A name like `containsSortBelowProjectFilterChain` or simply `hasSortInChain` would more accurately describe what it does. Minor, but method names are part of the long-term maintenance surface.
--
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-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4878:
URL: https://github.com/apache/calcite/pull/4878#issuecomment-4229741043
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4878) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4878&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4878&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4878&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [100.0% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4878&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4878&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4878)
--
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-7463] UnionToFilterRule incorrectly rewrites UNION with LIMIT [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4878:
URL: https://github.com/apache/calcite/pull/4878#issuecomment-4231707920
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4878) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4878&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4878&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4878&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [89.5% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4878&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4878&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4878)
--
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