You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "suibianwanwank (via GitHub)" <gi...@apache.org> on 2025/02/05 12:52:02 UTC
[PR] [CALCITE-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
suibianwanwank opened a new pull request, #4181:
URL: https://github.com/apache/calcite/pull/4181
This PR handles decorrelate since the query contains limit 1. There are two main types of conversions:
1. If sorted with no OFFSET and FETCH = 1, and only one collation field, rewrite the Sort as Aggregate using MIN/MAX function.
```
Example:
Sort(sort0=[$0], dir0=[ASC], fetch=[1])
input
Rewrite to:
Aggregate(group=(corVar), agg=[min($0))
```
2. For the more general case,rewrite the sort to be
```
Aggregate(group=(corVar.. , field..))
project(first_value(field) over (partition by corVar order by (sort collation)))
input
```
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4181:
URL: https://github.com/apache/calcite/pull/4181#issuecomment-2642760425
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4181) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4181&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4181&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4181&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [91.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4181&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4181&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4181)
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "suibianwanwank (via GitHub)" <gi...@apache.org>.
suibianwanwank commented on code in PR #4181:
URL: https://github.com/apache/calcite/pull/4181#discussion_r1947492911
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -795,6 +801,146 @@ private static void shiftMapping(Map<Integer, Integer> mapping, int startIndex,
return null;
}
+ private @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame frame) {
Review Comment:
I agree that we need a safe way to avoid errors. Usually the user uses the decorrelate by calling decorrelateQuery. So I think is it possible to add something like SqlToRelConverter#CONFIG (which can be combined with [CALCITE-6674]) to choose whether to enable it or not. WDYT
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #4181:
URL: https://github.com/apache/calcite/pull/4181#discussion_r1952427110
##########
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##########
@@ -8577,6 +8577,111 @@ private void checkSemiJoinRuleOnAntiJoin(RelOptRule rule) {
.check();
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6652">[CALCITE-6652]
+ * RelDecorrelator can't decorrelate query with limit 1</a>.
+ */
+ @Test void testDecorrelateProjectWithFetchOne() {
+ final String query = "SELECT name, "
+ + "(SELECT sal FROM emp where dept.deptno = emp.deptno order by sal limit 1) "
+ + "FROM dept";
+ sql(query).withRule(CoreRules.PROJECT_SUB_QUERY_TO_CORRELATE)
+ .withLateDecorrelate(true)
+ .check();
+ }
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6652">[CALCITE-6652]
+ * RelDecorrelator can't decorrelate query with limit 1</a>.
+ */
+ @Test void testDecorrelateProjectWithFetchOneDesc() {
+ final String query = "SELECT name, "
+ + "(SELECT emp.sal FROM emp WHERE dept.deptno = emp.deptno "
+ + "ORDER BY emp.sal desc nulls last LIMIT 1) "
+ + "FROM dept";
+ sql(query).withRule(CoreRules.PROJECT_SUB_QUERY_TO_CORRELATE)
+ .withLateDecorrelate(true)
+ .check();
+ }
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6652">[CALCITE-6652]
+ * RelDecorrelator can't decorrelate query with limit 1</a>.
+ */
+ @Test void testDecorrelateFilterWithFetchOne() {
+ final String query = "SELECT name FROM dept "
+ + "WHERE 10 > (SELECT emp.sal FROM emp where dept.deptno = emp.deptno "
+ + "ORDER BY emp.sal desc nulls last limit 1)";
+ sql(query).withRule(CoreRules.FILTER_SUB_QUERY_TO_CORRELATE)
+ .withLateDecorrelate(true)
+ .check();
+ }
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6652">[CALCITE-6652]
+ * RelDecorrelator can't decorrelate query with limit 1</a>.
+ */
+ @Test void testDecorrelateFilterWithFetchOneDesc() {
+ final String query = "SELECT name FROM dept "
Review Comment:
Ins't this test exactly the same as the previous one?
##########
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##########
@@ -8577,6 +8577,111 @@ private void checkSemiJoinRuleOnAntiJoin(RelOptRule rule) {
.check();
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6652">[CALCITE-6652]
+ * RelDecorrelator can't decorrelate query with limit 1</a>.
+ */
+ @Test void testDecorrelateProjectWithFetchOne() {
+ final String query = "SELECT name, "
+ + "(SELECT sal FROM emp where dept.deptno = emp.deptno order by sal limit 1) "
+ + "FROM dept";
+ sql(query).withRule(CoreRules.PROJECT_SUB_QUERY_TO_CORRELATE)
+ .withLateDecorrelate(true)
+ .check();
+ }
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6652">[CALCITE-6652]
+ * RelDecorrelator can't decorrelate query with limit 1</a>.
+ */
+ @Test void testDecorrelateProjectWithFetchOneDesc() {
+ final String query = "SELECT name, "
+ + "(SELECT emp.sal FROM emp WHERE dept.deptno = emp.deptno "
+ + "ORDER BY emp.sal desc nulls last LIMIT 1) "
+ + "FROM dept";
+ sql(query).withRule(CoreRules.PROJECT_SUB_QUERY_TO_CORRELATE)
+ .withLateDecorrelate(true)
+ .check();
+ }
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6652">[CALCITE-6652]
+ * RelDecorrelator can't decorrelate query with limit 1</a>.
+ */
+ @Test void testDecorrelateFilterWithFetchOne() {
+ final String query = "SELECT name FROM dept "
+ + "WHERE 10 > (SELECT emp.sal FROM emp where dept.deptno = emp.deptno "
+ + "ORDER BY emp.sal desc nulls last limit 1)";
+ sql(query).withRule(CoreRules.FILTER_SUB_QUERY_TO_CORRELATE)
+ .withLateDecorrelate(true)
+ .check();
+ }
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6652">[CALCITE-6652]
+ * RelDecorrelator can't decorrelate query with limit 1</a>.
+ */
+ @Test void testDecorrelateFilterWithFetchOneDesc() {
+ final String query = "SELECT name FROM dept "
Review Comment:
Isn't this test exactly the same as the previous 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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "suibianwanwank (via GitHub)" <gi...@apache.org>.
suibianwanwank commented on PR #4181:
URL: https://github.com/apache/calcite/pull/4181#issuecomment-2639659919
> If no one reviews it, I will start reviewing it this weekend, it may take some time.
Thank you. If you have any questions or need further clarification, feel free to ask.
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada merged PR #4181:
URL: https://github.com/apache/calcite/pull/4181
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #4181:
URL: https://github.com/apache/calcite/pull/4181#discussion_r1946175710
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -795,6 +801,122 @@ private static void shiftMapping(Map<Integer, Integer> mapping, int startIndex,
return null;
}
+ private @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame frame) {
+ final Map<Integer, Integer> mapOldToNewOutputs = new HashMap<>();
+ final NavigableMap<CorDef, Integer> corDefOutputs = new TreeMap<>();
+ //
+ // Rewrite logic:
+ //
+ // If sorted with no OFFSET and FETCH = 1, and only one collation field,
+ // rewrite the Sort as Aggregate using MIN/MAX function.
+ // Example:
+ // Sort(sort0=[$0], dir0=[ASC], fetch=[1])
+ // input
+ // Rewrite to:
+ // Aggregate(group=(corVar), agg=[min($0))
+ //
+ // Note: MIN/MAX is not strictly equivalent to LIMIT 1. When the input has 0 rows,
+ // MIN/MAX returns NULL, while LIMIT 1 returns 0 rows.
+ // However, in the decorrelate, we add correlated variables to the group list
+ // to ensure equivalence when Correlate is transformed to Join. When the group list
+ // is non-empty, MIN/MAX will also return 0 rows if input with 0 rows.(This behavior
+ // also leads to the issue described in [CALCITE-5743]) So in this case, the transformation
+ // is legal.
+ if (sort.getCollation().getFieldCollations().size() == 1
+ && sort.getRowType().getFieldCount() == 1
+ && !frame.corDefOutputs.isEmpty()) {
+ RelFieldCollation collation = Util.first(sort.getCollation().getFieldCollations());
+
+ SqlAggFunction aggFunction;
+ switch (collation.getDirection()) {
+ case ASCENDING:
+ case STRICTLY_ASCENDING:
+ aggFunction = SqlStdOperatorTable.MIN;
+ break;
+ case DESCENDING:
+ case STRICTLY_DESCENDING:
+ aggFunction = SqlStdOperatorTable.MAX;
+ break;
+ default:
+ return null;
+ }
+
+ final int newIdx = requireNonNull(frame.oldToNewOutputs.get(collation.getFieldIndex()));
+ RelBuilder.AggCall aggCall = relBuilder.push(frame.r)
Review Comment:
This was my exact same thought after a quick look at the PR... the proposed conversion is 100% equivalent in all cases, also when nulls are involved? What about the original Sort is nulls-first and the relevant field has null values?
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #4181:
URL: https://github.com/apache/calcite/pull/4181#discussion_r1948713704
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -795,6 +801,146 @@ private static void shiftMapping(Map<Integer, Integer> mapping, int startIndex,
return null;
}
+ private @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame frame) {
Review Comment:
It may be a cleaner solution, but IMO maybe a bit overkill for this particular purpose. I think protected methods would suffice to provide a way to "fallback". Let's see if there are other opinions on this subject.
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #4181:
URL: https://github.com/apache/calcite/pull/4181#discussion_r1946837795
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -795,6 +801,122 @@ private static void shiftMapping(Map<Integer, Integer> mapping, int startIndex,
return null;
}
+ private @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame frame) {
+ final Map<Integer, Integer> mapOldToNewOutputs = new HashMap<>();
+ final NavigableMap<CorDef, Integer> corDefOutputs = new TreeMap<>();
+ //
+ // Rewrite logic:
+ //
+ // If sorted with no OFFSET and FETCH = 1, and only one collation field,
+ // rewrite the Sort as Aggregate using MIN/MAX function.
+ // Example:
+ // Sort(sort0=[$0], dir0=[ASC], fetch=[1])
+ // input
+ // Rewrite to:
+ // Aggregate(group=(corVar), agg=[min($0))
+ //
+ // Note: MIN/MAX is not strictly equivalent to LIMIT 1. When the input has 0 rows,
+ // MIN/MAX returns NULL, while LIMIT 1 returns 0 rows.
+ // However, in the decorrelate, we add correlated variables to the group list
+ // to ensure equivalence when Correlate is transformed to Join. When the group list
+ // is non-empty, MIN/MAX will also return 0 rows if input with 0 rows.(This behavior
+ // also leads to the issue described in [CALCITE-5743]) So in this case, the transformation
+ // is legal.
+ if (sort.getCollation().getFieldCollations().size() == 1
+ && sort.getRowType().getFieldCount() == 1
+ && !frame.corDefOutputs.isEmpty()) {
+ RelFieldCollation collation = Util.first(sort.getCollation().getFieldCollations());
+
+ SqlAggFunction aggFunction;
+ switch (collation.getDirection()) {
+ case ASCENDING:
+ case STRICTLY_ASCENDING:
+ aggFunction = SqlStdOperatorTable.MIN;
+ break;
+ case DESCENDING:
+ case STRICTLY_DESCENDING:
+ aggFunction = SqlStdOperatorTable.MAX;
+ break;
+ default:
+ return null;
+ }
+
+ final int newIdx = requireNonNull(frame.oldToNewOutputs.get(collation.getFieldIndex()));
+ RelBuilder.AggCall aggCall = relBuilder.push(frame.r)
Review Comment:
Could we have some unit tests with nulls-first, to see the expected result being the conversion *not* being applied?
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #4181:
URL: https://github.com/apache/calcite/pull/4181#discussion_r1946844297
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -795,6 +801,146 @@ private static void shiftMapping(Map<Integer, Integer> mapping, int startIndex,
return null;
}
+ private @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame frame) {
Review Comment:
Maybe I'm a bit too paranoid, but just to be on the safe side, I'd propose the new methods (`decorrelateFetchOneSort` and `decorrelateSortAsAggregate`) to be `protected` instead of `private`; the reason is that if, for whatever reason, a downstream project wants to "switch off" this conversion, they could simply extend the RelDecorrletor with their own child class and override these methods to simply return `null`, right?
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -795,6 +801,146 @@ private static void shiftMapping(Map<Integer, Integer> mapping, int startIndex,
return null;
}
+ private @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame frame) {
Review Comment:
Maybe I'm a bit too paranoid, but just to be on the safe side, I'd propose the new methods (`decorrelateFetchOneSort` and `decorrelateSortAsAggregate`) to be `protected` instead of `private`; the reason is that if, for whatever reason, a downstream project wants to "switch off" this conversion, they could simply extend the RelDecorreletor with their own child class and override these methods to simply return `null`, right?
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4181:
URL: https://github.com/apache/calcite/pull/4181#issuecomment-2659013907
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4181) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4181&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4181&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4181&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [91.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4181&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4181&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4181)
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4181:
URL: https://github.com/apache/calcite/pull/4181#issuecomment-2642419645
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4181) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4181&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4181&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4181&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [90.7% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4181&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4181&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4181)
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "suibianwanwank (via GitHub)" <gi...@apache.org>.
suibianwanwank commented on code in PR #4181:
URL: https://github.com/apache/calcite/pull/4181#discussion_r1950153334
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -795,6 +801,122 @@ private static void shiftMapping(Map<Integer, Integer> mapping, int startIndex,
return null;
}
+ private @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame frame) {
+ final Map<Integer, Integer> mapOldToNewOutputs = new HashMap<>();
+ final NavigableMap<CorDef, Integer> corDefOutputs = new TreeMap<>();
+ //
+ // Rewrite logic:
+ //
+ // If sorted with no OFFSET and FETCH = 1, and only one collation field,
+ // rewrite the Sort as Aggregate using MIN/MAX function.
+ // Example:
+ // Sort(sort0=[$0], dir0=[ASC], fetch=[1])
+ // input
+ // Rewrite to:
+ // Aggregate(group=(corVar), agg=[min($0))
+ //
+ // Note: MIN/MAX is not strictly equivalent to LIMIT 1. When the input has 0 rows,
+ // MIN/MAX returns NULL, while LIMIT 1 returns 0 rows.
+ // However, in the decorrelate, we add correlated variables to the group list
+ // to ensure equivalence when Correlate is transformed to Join. When the group list
+ // is non-empty, MIN/MAX will also return 0 rows if input with 0 rows.(This behavior
+ // also leads to the issue described in [CALCITE-5743]) So in this case, the transformation
+ // is legal.
+ if (sort.getCollation().getFieldCollations().size() == 1
+ && sort.getRowType().getFieldCount() == 1
+ && !frame.corDefOutputs.isEmpty()) {
+ RelFieldCollation collation = Util.first(sort.getCollation().getFieldCollations());
+
+ SqlAggFunction aggFunction;
+ switch (collation.getDirection()) {
+ case ASCENDING:
+ case STRICTLY_ASCENDING:
+ aggFunction = SqlStdOperatorTable.MIN;
+ break;
+ case DESCENDING:
+ case STRICTLY_DESCENDING:
+ aggFunction = SqlStdOperatorTable.MAX;
+ break;
+ default:
+ return null;
+ }
+
+ final int newIdx = requireNonNull(frame.oldToNewOutputs.get(collation.getFieldIndex()));
+ RelBuilder.AggCall aggCall = relBuilder.push(frame.r)
Review Comment:
For MIN/MAX, null values are ignored in the databases I know (I didn't find a corresponding description in the sql standard). So I think both asc and desc should be null last, i.e.return null only if all values are null.
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "suibianwanwank (via GitHub)" <gi...@apache.org>.
suibianwanwank commented on PR #4181:
URL: https://github.com/apache/calcite/pull/4181#issuecomment-2642828446
Thanks for your feedback! I've updated the code and added null value test, thanks to review when you have time.
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "suibianwanwank (via GitHub)" <gi...@apache.org>.
suibianwanwank commented on code in PR #4181:
URL: https://github.com/apache/calcite/pull/4181#discussion_r1946209577
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -795,6 +801,122 @@ private static void shiftMapping(Map<Integer, Integer> mapping, int startIndex,
return null;
}
+ private @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame frame) {
+ final Map<Integer, Integer> mapOldToNewOutputs = new HashMap<>();
+ final NavigableMap<CorDef, Integer> corDefOutputs = new TreeMap<>();
+ //
+ // Rewrite logic:
+ //
+ // If sorted with no OFFSET and FETCH = 1, and only one collation field,
+ // rewrite the Sort as Aggregate using MIN/MAX function.
+ // Example:
+ // Sort(sort0=[$0], dir0=[ASC], fetch=[1])
+ // input
+ // Rewrite to:
+ // Aggregate(group=(corVar), agg=[min($0))
+ //
+ // Note: MIN/MAX is not strictly equivalent to LIMIT 1. When the input has 0 rows,
+ // MIN/MAX returns NULL, while LIMIT 1 returns 0 rows.
+ // However, in the decorrelate, we add correlated variables to the group list
+ // to ensure equivalence when Correlate is transformed to Join. When the group list
+ // is non-empty, MIN/MAX will also return 0 rows if input with 0 rows.(This behavior
+ // also leads to the issue described in [CALCITE-5743]) So in this case, the transformation
+ // is legal.
+ if (sort.getCollation().getFieldCollations().size() == 1
+ && sort.getRowType().getFieldCount() == 1
+ && !frame.corDefOutputs.isEmpty()) {
+ RelFieldCollation collation = Util.first(sort.getCollation().getFieldCollations());
+
+ SqlAggFunction aggFunction;
+ switch (collation.getDirection()) {
+ case ASCENDING:
+ case STRICTLY_ASCENDING:
+ aggFunction = SqlStdOperatorTable.MIN;
+ break;
+ case DESCENDING:
+ case STRICTLY_DESCENDING:
+ aggFunction = SqlStdOperatorTable.MAX;
+ break;
+ default:
+ return null;
+ }
+
+ final int newIdx = requireNonNull(frame.oldToNewOutputs.get(collation.getFieldIndex()));
+ RelBuilder.AggCall aggCall = relBuilder.push(frame.r)
Review Comment:
Yes, as mentioned in JIRA, I missed this. This conversion is only valid if NULL LAST.
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "suibianwanwank (via GitHub)" <gi...@apache.org>.
suibianwanwank commented on code in PR #4181:
URL: https://github.com/apache/calcite/pull/4181#discussion_r1952884246
##########
core/src/test/java/org/apache/calcite/test/RelOptRulesTest.java:
##########
@@ -8577,6 +8577,111 @@ private void checkSemiJoinRuleOnAntiJoin(RelOptRule rule) {
.check();
}
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6652">[CALCITE-6652]
+ * RelDecorrelator can't decorrelate query with limit 1</a>.
+ */
+ @Test void testDecorrelateProjectWithFetchOne() {
+ final String query = "SELECT name, "
+ + "(SELECT sal FROM emp where dept.deptno = emp.deptno order by sal limit 1) "
+ + "FROM dept";
+ sql(query).withRule(CoreRules.PROJECT_SUB_QUERY_TO_CORRELATE)
+ .withLateDecorrelate(true)
+ .check();
+ }
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6652">[CALCITE-6652]
+ * RelDecorrelator can't decorrelate query with limit 1</a>.
+ */
+ @Test void testDecorrelateProjectWithFetchOneDesc() {
+ final String query = "SELECT name, "
+ + "(SELECT emp.sal FROM emp WHERE dept.deptno = emp.deptno "
+ + "ORDER BY emp.sal desc nulls last LIMIT 1) "
+ + "FROM dept";
+ sql(query).withRule(CoreRules.PROJECT_SUB_QUERY_TO_CORRELATE)
+ .withLateDecorrelate(true)
+ .check();
+ }
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6652">[CALCITE-6652]
+ * RelDecorrelator can't decorrelate query with limit 1</a>.
+ */
+ @Test void testDecorrelateFilterWithFetchOne() {
+ final String query = "SELECT name FROM dept "
+ + "WHERE 10 > (SELECT emp.sal FROM emp where dept.deptno = emp.deptno "
+ + "ORDER BY emp.sal desc nulls last limit 1)";
+ sql(query).withRule(CoreRules.FILTER_SUB_QUERY_TO_CORRELATE)
+ .withLateDecorrelate(true)
+ .check();
+ }
+
+ /** Test case for
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-6652">[CALCITE-6652]
+ * RelDecorrelator can't decorrelate query with limit 1</a>.
+ */
+ @Test void testDecorrelateFilterWithFetchOneDesc() {
+ final String query = "SELECT name FROM dept "
Review Comment:
Thanks for catching that! I've fixed 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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4181:
URL: https://github.com/apache/calcite/pull/4181#issuecomment-2658808651
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4181) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4181&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4181&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4181&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [91.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4181&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4181&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4181)
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4181:
URL: https://github.com/apache/calcite/pull/4181#issuecomment-2636837235
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4181) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4181&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4181&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4181&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [89.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4181&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4181&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4181)
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "caicancai (via GitHub)" <gi...@apache.org>.
caicancai commented on PR #4181:
URL: https://github.com/apache/calcite/pull/4181#issuecomment-2638945516
If no one reviews it, I will start reviewing it this weekend, it may take some time.
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "suibianwanwank (via GitHub)" <gi...@apache.org>.
suibianwanwank commented on code in PR #4181:
URL: https://github.com/apache/calcite/pull/4181#discussion_r1946209577
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -795,6 +801,122 @@ private static void shiftMapping(Map<Integer, Integer> mapping, int startIndex,
return null;
}
+ private @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame frame) {
+ final Map<Integer, Integer> mapOldToNewOutputs = new HashMap<>();
+ final NavigableMap<CorDef, Integer> corDefOutputs = new TreeMap<>();
+ //
+ // Rewrite logic:
+ //
+ // If sorted with no OFFSET and FETCH = 1, and only one collation field,
+ // rewrite the Sort as Aggregate using MIN/MAX function.
+ // Example:
+ // Sort(sort0=[$0], dir0=[ASC], fetch=[1])
+ // input
+ // Rewrite to:
+ // Aggregate(group=(corVar), agg=[min($0))
+ //
+ // Note: MIN/MAX is not strictly equivalent to LIMIT 1. When the input has 0 rows,
+ // MIN/MAX returns NULL, while LIMIT 1 returns 0 rows.
+ // However, in the decorrelate, we add correlated variables to the group list
+ // to ensure equivalence when Correlate is transformed to Join. When the group list
+ // is non-empty, MIN/MAX will also return 0 rows if input with 0 rows.(This behavior
+ // also leads to the issue described in [CALCITE-5743]) So in this case, the transformation
+ // is legal.
+ if (sort.getCollation().getFieldCollations().size() == 1
+ && sort.getRowType().getFieldCount() == 1
+ && !frame.corDefOutputs.isEmpty()) {
+ RelFieldCollation collation = Util.first(sort.getCollation().getFieldCollations());
+
+ SqlAggFunction aggFunction;
+ switch (collation.getDirection()) {
+ case ASCENDING:
+ case STRICTLY_ASCENDING:
+ aggFunction = SqlStdOperatorTable.MIN;
+ break;
+ case DESCENDING:
+ case STRICTLY_DESCENDING:
+ aggFunction = SqlStdOperatorTable.MAX;
+ break;
+ default:
+ return null;
+ }
+
+ final int newIdx = requireNonNull(frame.oldToNewOutputs.get(collation.getFieldIndex()));
+ RelBuilder.AggCall aggCall = relBuilder.push(frame.r)
Review Comment:
Yes, as mentioned in JIRA, I missed this. This conversion is only valid if nulls-last.
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #4181:
URL: https://github.com/apache/calcite/pull/4181#issuecomment-2647766496
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4181) **Quality Gate passed**
Issues
 [2 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4181&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=4181&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=4181&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [91.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4181&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=4181&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=4181)
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4181:
URL: https://github.com/apache/calcite/pull/4181#discussion_r1949711895
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -795,6 +801,122 @@ private static void shiftMapping(Map<Integer, Integer> mapping, int startIndex,
return null;
}
+ private @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame frame) {
+ final Map<Integer, Integer> mapOldToNewOutputs = new HashMap<>();
+ final NavigableMap<CorDef, Integer> corDefOutputs = new TreeMap<>();
+ //
+ // Rewrite logic:
+ //
+ // If sorted with no OFFSET and FETCH = 1, and only one collation field,
+ // rewrite the Sort as Aggregate using MIN/MAX function.
+ // Example:
+ // Sort(sort0=[$0], dir0=[ASC], fetch=[1])
+ // input
+ // Rewrite to:
+ // Aggregate(group=(corVar), agg=[min($0))
+ //
+ // Note: MIN/MAX is not strictly equivalent to LIMIT 1. When the input has 0 rows,
+ // MIN/MAX returns NULL, while LIMIT 1 returns 0 rows.
+ // However, in the decorrelate, we add correlated variables to the group list
+ // to ensure equivalence when Correlate is transformed to Join. When the group list
+ // is non-empty, MIN/MAX will also return 0 rows if input with 0 rows.(This behavior
+ // also leads to the issue described in [CALCITE-5743]) So in this case, the transformation
+ // is legal.
+ if (sort.getCollation().getFieldCollations().size() == 1
+ && sort.getRowType().getFieldCount() == 1
+ && !frame.corDefOutputs.isEmpty()) {
+ RelFieldCollation collation = Util.first(sort.getCollation().getFieldCollations());
+
+ SqlAggFunction aggFunction;
+ switch (collation.getDirection()) {
+ case ASCENDING:
+ case STRICTLY_ASCENDING:
+ aggFunction = SqlStdOperatorTable.MIN;
+ break;
+ case DESCENDING:
+ case STRICTLY_DESCENDING:
+ aggFunction = SqlStdOperatorTable.MAX;
+ break;
+ default:
+ return null;
+ }
+
+ final int newIdx = requireNonNull(frame.oldToNewOutputs.get(collation.getFieldIndex()));
+ RelBuilder.AggCall aggCall = relBuilder.push(frame.r)
Review Comment:
I would think that MAX works for NULL LAST only, and MIN for NULL FIRST.
Am I wrong?
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "suibianwanwank (via GitHub)" <gi...@apache.org>.
suibianwanwank commented on code in PR #4181:
URL: https://github.com/apache/calcite/pull/4181#discussion_r1948929889
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -795,6 +801,146 @@ private static void shiftMapping(Map<Integer, Integer> mapping, int startIndex,
return null;
}
+ private @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame frame) {
Review Comment:
Got it, I've made the changes. Thanks for the 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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #4181:
URL: https://github.com/apache/calcite/pull/4181#discussion_r1945587325
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -795,6 +801,122 @@ private static void shiftMapping(Map<Integer, Integer> mapping, int startIndex,
return null;
}
+ private @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame frame) {
+ final Map<Integer, Integer> mapOldToNewOutputs = new HashMap<>();
+ final NavigableMap<CorDef, Integer> corDefOutputs = new TreeMap<>();
+ //
+ // Rewrite logic:
+ //
+ // If sorted with no OFFSET and FETCH = 1, and only one collation field,
+ // rewrite the Sort as Aggregate using MIN/MAX function.
+ // Example:
+ // Sort(sort0=[$0], dir0=[ASC], fetch=[1])
+ // input
+ // Rewrite to:
+ // Aggregate(group=(corVar), agg=[min($0))
+ //
+ // Note: MIN/MAX is not strictly equivalent to LIMIT 1. When the input has 0 rows,
+ // MIN/MAX returns NULL, while LIMIT 1 returns 0 rows.
+ // However, in the decorrelate, we add correlated variables to the group list
+ // to ensure equivalence when Correlate is transformed to Join. When the group list
+ // is non-empty, MIN/MAX will also return 0 rows if input with 0 rows.(This behavior
Review Comment:
"if the input has 0 rows."
I think that the references to the other calcite issue should not be in this comment - they will become obsolete if the issue is closed, and they don't really clarify what is going on here.
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -795,6 +801,122 @@ private static void shiftMapping(Map<Integer, Integer> mapping, int startIndex,
return null;
}
+ private @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame frame) {
+ final Map<Integer, Integer> mapOldToNewOutputs = new HashMap<>();
+ final NavigableMap<CorDef, Integer> corDefOutputs = new TreeMap<>();
+ //
+ // Rewrite logic:
+ //
+ // If sorted with no OFFSET and FETCH = 1, and only one collation field,
+ // rewrite the Sort as Aggregate using MIN/MAX function.
+ // Example:
+ // Sort(sort0=[$0], dir0=[ASC], fetch=[1])
+ // input
+ // Rewrite to:
+ // Aggregate(group=(corVar), agg=[min($0))
+ //
+ // Note: MIN/MAX is not strictly equivalent to LIMIT 1. When the input has 0 rows,
+ // MIN/MAX returns NULL, while LIMIT 1 returns 0 rows.
+ // However, in the decorrelate, we add correlated variables to the group list
+ // to ensure equivalence when Correlate is transformed to Join. When the group list
+ // is non-empty, MIN/MAX will also return 0 rows if input with 0 rows.(This behavior
+ // also leads to the issue described in [CALCITE-5743]) So in this case, the transformation
+ // is legal.
+ if (sort.getCollation().getFieldCollations().size() == 1
+ && sort.getRowType().getFieldCount() == 1
+ && !frame.corDefOutputs.isEmpty()) {
+ RelFieldCollation collation = Util.first(sort.getCollation().getFieldCollations());
+
+ SqlAggFunction aggFunction;
+ switch (collation.getDirection()) {
+ case ASCENDING:
+ case STRICTLY_ASCENDING:
+ aggFunction = SqlStdOperatorTable.MIN;
+ break;
+ case DESCENDING:
+ case STRICTLY_DESCENDING:
+ aggFunction = SqlStdOperatorTable.MAX;
+ break;
+ default:
+ return null;
+ }
+
+ final int newIdx = requireNonNull(frame.oldToNewOutputs.get(collation.getFieldIndex()));
+ RelBuilder.AggCall aggCall = relBuilder.push(frame.r)
Review Comment:
I am wondering: sorting has collation, but min/max do not.
For example, sorting can specify what happens to nulls.
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -795,6 +801,122 @@ private static void shiftMapping(Map<Integer, Integer> mapping, int startIndex,
return null;
}
+ private @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame frame) {
+ final Map<Integer, Integer> mapOldToNewOutputs = new HashMap<>();
+ final NavigableMap<CorDef, Integer> corDefOutputs = new TreeMap<>();
+ //
+ // Rewrite logic:
+ //
+ // If sorted with no OFFSET and FETCH = 1, and only one collation field,
Review Comment:
the check for fetch == 1 is not here (so you can mention that it's enforced by the caller).
Moreover, for symmetry, should this comment be within the if branch?
--
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-6652] RelDecorrelator can't decorrelate query with limit 1 [calcite]
Posted by "rubenada (via GitHub)" <gi...@apache.org>.
rubenada commented on code in PR #4181:
URL: https://github.com/apache/calcite/pull/4181#discussion_r1946175710
##########
core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java:
##########
@@ -795,6 +801,122 @@ private static void shiftMapping(Map<Integer, Integer> mapping, int startIndex,
return null;
}
+ private @Nullable Frame decorrelateFetchOneSort(Sort sort, final Frame frame) {
+ final Map<Integer, Integer> mapOldToNewOutputs = new HashMap<>();
+ final NavigableMap<CorDef, Integer> corDefOutputs = new TreeMap<>();
+ //
+ // Rewrite logic:
+ //
+ // If sorted with no OFFSET and FETCH = 1, and only one collation field,
+ // rewrite the Sort as Aggregate using MIN/MAX function.
+ // Example:
+ // Sort(sort0=[$0], dir0=[ASC], fetch=[1])
+ // input
+ // Rewrite to:
+ // Aggregate(group=(corVar), agg=[min($0))
+ //
+ // Note: MIN/MAX is not strictly equivalent to LIMIT 1. When the input has 0 rows,
+ // MIN/MAX returns NULL, while LIMIT 1 returns 0 rows.
+ // However, in the decorrelate, we add correlated variables to the group list
+ // to ensure equivalence when Correlate is transformed to Join. When the group list
+ // is non-empty, MIN/MAX will also return 0 rows if input with 0 rows.(This behavior
+ // also leads to the issue described in [CALCITE-5743]) So in this case, the transformation
+ // is legal.
+ if (sort.getCollation().getFieldCollations().size() == 1
+ && sort.getRowType().getFieldCount() == 1
+ && !frame.corDefOutputs.isEmpty()) {
+ RelFieldCollation collation = Util.first(sort.getCollation().getFieldCollations());
+
+ SqlAggFunction aggFunction;
+ switch (collation.getDirection()) {
+ case ASCENDING:
+ case STRICTLY_ASCENDING:
+ aggFunction = SqlStdOperatorTable.MIN;
+ break;
+ case DESCENDING:
+ case STRICTLY_DESCENDING:
+ aggFunction = SqlStdOperatorTable.MAX;
+ break;
+ default:
+ return null;
+ }
+
+ final int newIdx = requireNonNull(frame.oldToNewOutputs.get(collation.getFieldIndex()));
+ RelBuilder.AggCall aggCall = relBuilder.push(frame.r)
Review Comment:
This was my exact same thought after a quick look at the PR... is the proposed conversion 100% equivalent in all cases, also when nulls are involved? What about when the original Sort is nulls-first and the relevant field has null values?
--
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