[CALCITE-6652] RelDecorrelator can't decorrelate query with limit 1 #4181

suibianwanwank · 2025-02-05T12:52:00Z

This PR handles decorrelate since the query contains limit 1. There are two main types of conversions:

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))

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

caicancai · 2025-02-06T06:25:11Z

If no one reviews it, I will start reviewing it this weekend, it may take some time.

suibianwanwank · 2025-02-06T12:13:57Z

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.

mihaibudiu

I will try to review some more later, after I understand the relationship between order by and min/max.
Perhaps it's enough to check that the orderby does null handling in the expected way.

mihaibudiu · 2025-02-06T22:55:40Z

core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java

+    // 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


"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.

mihaibudiu · 2025-02-06T23:16:06Z

core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java

+    //
+    // Rewrite logic:
+    //
+    // If sorted with no OFFSET and FETCH = 1, and only one collation field,


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?

mihaibudiu · 2025-02-06T23:22:55Z

core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java

+      }
+
+      final int newIdx = requireNonNull(frame.oldToNewOutputs.get(collation.getFieldIndex()));
+      RelBuilder.AggCall aggCall = relBuilder.push(frame.r)


I am wondering: sorting has collation, but min/max do not.
For example, sorting can specify what happens to nulls.

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?

Yes, as mentioned in JIRA, I missed this. This conversion is only valid if nulls-last.

Could we have some unit tests with nulls-first, to see the expected result being the conversion not being applied?

I would think that MAX works for NULL LAST only, and MIN for NULL FIRST.
Am I wrong?

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.

suibianwanwank · 2025-02-07T12:49:55Z

@mihaibudiu @rubenada Thanks for your feedback! I've updated the code and added null value test, thanks to review when you have time.

rubenada · 2025-02-07T16:45:41Z

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) {


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?

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

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.

Got it, I've made the changes. Thanks for the suggestion.

sonarqubecloud · 2025-02-10T11:53:02Z

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
91.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

mihaibudiu

I am assuming that the quidem test results have been checked with another database as well.

mihaibudiu reviewed Feb 6, 2025

View reviewed changes

suibianwanwank force-pushed the calcite-6652 branch from a7d9b79 to 6982d0b Compare February 7, 2025 09:10

[CALCITE-6652] RelDecorrelator can't decorrelate query with limit 1

58d47f8

suibianwanwank force-pushed the calcite-6652 branch from 6982d0b to 58d47f8 Compare February 7, 2025 11:49

suibianwanwank requested review from mihaibudiu and rubenada February 7, 2025 12:44

rubenada reviewed Feb 7, 2025

View reviewed changes

[CALCITE-6652] Update method visibility

be78d9e

mihaibudiu approved these changes Feb 11, 2025

View reviewed changes

mihaibudiu added the LGTM-will-merge-soon Overall PR looks OK. Only minor things left. label Feb 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-6652] RelDecorrelator can't decorrelate query with limit 1 #4181

[CALCITE-6652] RelDecorrelator can't decorrelate query with limit 1 #4181

suibianwanwank commented Feb 5, 2025

caicancai commented Feb 6, 2025

suibianwanwank commented Feb 6, 2025

mihaibudiu left a comment

mihaibudiu Feb 6, 2025

mihaibudiu Feb 6, 2025

mihaibudiu Feb 6, 2025

rubenada Feb 7, 2025 •

edited

Loading

suibianwanwank Feb 7, 2025 •

edited

Loading

rubenada Feb 7, 2025

mihaibudiu Feb 10, 2025

suibianwanwank Feb 11, 2025

suibianwanwank commented Feb 7, 2025 •

edited

Loading

rubenada Feb 7, 2025 •

edited

Loading

suibianwanwank Feb 8, 2025

rubenada Feb 10, 2025

suibianwanwank Feb 10, 2025

sonarqubecloud bot commented Feb 10, 2025

mihaibudiu left a comment

[CALCITE-6652] RelDecorrelator can't decorrelate query with limit 1 #4181

Are you sure you want to change the base?

[CALCITE-6652] RelDecorrelator can't decorrelate query with limit 1 #4181

Conversation

suibianwanwank commented Feb 5, 2025

caicancai commented Feb 6, 2025

suibianwanwank commented Feb 6, 2025

mihaibudiu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenada Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

suibianwanwank Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suibianwanwank commented Feb 7, 2025 • edited Loading

rubenada Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Feb 10, 2025

Quality Gate passed

mihaibudiu left a comment

Choose a reason for hiding this comment

rubenada Feb 7, 2025 •

edited

Loading

suibianwanwank Feb 7, 2025 •

edited

Loading

suibianwanwank commented Feb 7, 2025 •

edited

Loading

rubenada Feb 7, 2025 •

edited

Loading