[CALCITE-6942] Support decorrelated for sub-queries with LIMIT 1 and OFFSET #4633

iwanttobepowerful · 2025-11-14T12:55:11Z

github-actions · 2025-12-15T03:55:53Z

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 [email protected] list. Thank you for your contributions.

iwanttobepowerful · 2025-12-16T09:30:16Z

core/src/test/resources/sql/sub-query.iq


+!ok
+
+select * from emp


https://onecompiler.com/postgresql/447qy3drf

core/src/test/java/org/apache/calcite/sql2rel/RelDecorrelatorTest.java

mihaibudiu · 2025-12-16T17:37:33Z

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

-    Frame aggFrame = decorrelateSortAsAggregate(sort, frame);
-    if (aggFrame != null) {
-      return aggFrame;
+    if (sort.offset == null


the name of this function is no longer appropriate, but it is protected, so I am not sure you can change it.
you may need to create a new one.

maybe need change to a better name

mihaibudiu · 2025-12-16T17:38:13Z

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

-      return aggFrame;
+    if (sort.offset == null
+        && sort.fetch != null
+        && RexLiteral.intValue(sort.fetch) == 0) {


is 0 even allowed?
Also, intValue seems unsafe; there have been some bug fixes about this in the past.
BigInt is probably safest.

The purpose of this if check is to skip the subsequent optimization process for limit 0.

BigInt is probably safest.

ok

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

mihaibudiu

I have approved, but I still have a couple of comments.

mihaibudiu · 2025-12-18T00:00:53Z

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

-      return null;
+      if (rel.offset == null
+          && rel.fetch != null
+          && RexLiteral.longValue(rel.fetch) == 0) {


long is probably safe -- I don't expect more than 2^64 elements in a collection very soon
bigInt would have been safest. See https://issues.apache.org/jira/browse/CALCITE-7181 and the associated PR.

Okay, I will submit the modifications later. Thank you for your review.
At that time, I referred to

calcite/core/src/main/java/org/apache/calcite/rel/rules/PruneEmptyRules.java

Line 503 in dd1cd86

&& RexLiteral.longValue(sort.fetch) == 0;

@mihaibudiu @xiedeyantu

I don‘t think it's ok, please refer to https://issues.apache.org/jira/browse/CALCITE-7181. @julianhyde provided a very clear explanation.

It won't be much harder to use BigInteger here, so why not build the perfect solution?

mihaibudiu · 2025-12-18T00:05:07Z

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

-    //      - group by corVar1, covVar2, field1, field2
-    //      - any_value(fields1), any_value(fields2) group by corVar1, covVar2
-    //    Here we use the first.
+  protected @Nullable Frame decorrelateSortWithRowNumber(Sort sort, final Frame frame) {


My comment was that protected functions are part of the public API, so they cannot be modified (unless you change the major version). Perhaps the function should have been private?

I think it's safer to leave the function unchanged and add a new function.

Does the function have no other callers except this one? In that case you may need to add a description of a breaking change to the release notes: "Function RelDecorrelator.decorrelateFetchOneSort is no longer called by the decorrelator; instead consider using ..."

This function should have been designed as private in the first place. It only has one caller.
@mihaibudiu @suibianwanwank

You can bite the bullet and change the function as private, and add this in the list of breaking changes in _docs/history.md. It has to be part of the same commit. The way I see this, it will be a breaking change if you preserve the original function and if you don't, so why not make it private now?

OK, thanks
bfde556

Making this protected makes it easier for users to turn off the rewrite when they don’t want the plan to be transformed into row_number or min/max. That said, this might be over-engineering, and I’m fine either way.

If there is a legit reason to keep it protected, let's do it.
Then renaming it is still a breaking change, so let's document that.

iwanttobepowerful · 2025-12-22T03:27:30Z

Can I squash these commits to facilitate merging?
@mihaibudiu @suibianwanwank

mihaibudiu

If the other reviewers have no comments I propose to merge this.

mihaibudiu · 2025-12-23T22:11:10Z

Sorry, but the branch has developed a conflict, please fix and rebase so we can merge

…OFFSET

sonarqubecloud · 2025-12-23T22:59:49Z

Quality Gate passed

Issues
0 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

iwanttobepowerful · 2025-12-24T03:43:39Z

Sorry, but the branch has developed a conflict, please fix and rebase so we can merge

Done

github-actions bot added the stale label Dec 15, 2025

iwanttobepowerful force-pushed the zwhtest branch from 3cf367f to a463a62 Compare December 16, 2025 05:12

iwanttobepowerful marked this pull request as ready for review December 16, 2025 05:13

iwanttobepowerful force-pushed the zwhtest branch 6 times, most recently from 46e621a to ce662e7 Compare December 16, 2025 09:07

iwanttobepowerful changed the title ~~draft fix~~ [CALCITE-6942] Support decorrelated for sub-queries with LIMIT 1 and OFFSET Dec 16, 2025

iwanttobepowerful closed this Dec 16, 2025

iwanttobepowerful reopened this Dec 16, 2025

iwanttobepowerful force-pushed the zwhtest branch from ce662e7 to 2fca43a Compare December 16, 2025 09:23

iwanttobepowerful commented Dec 16, 2025

View reviewed changes

core/src/test/resources/sql/sub-query.iq

!ok

select * from emp

Copy link

Contributor Author

iwanttobepowerful Dec 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://onecompiler.com/postgresql/447qy3drf

iwanttobepowerful force-pushed the zwhtest branch 2 times, most recently from 5159fd0 to 5ec06f2 Compare December 16, 2025 11:15

mihaibudiu requested changes Dec 16, 2025

View reviewed changes

github-actions bot removed the stale label Dec 17, 2025

iwanttobepowerful force-pushed the zwhtest branch from 3ea095e to c314d24 Compare December 17, 2025 04:01

mihaibudiu approved these changes Dec 18, 2025

View reviewed changes

iwanttobepowerful force-pushed the zwhtest branch 3 times, most recently from bfde556 to b7dcb67 Compare December 19, 2025 08:04

mihaibudiu approved these changes Dec 22, 2025

View reviewed changes

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

iwanttobepowerful force-pushed the zwhtest branch from b7dcb67 to 8168d98 Compare December 23, 2025 02:23

[CALCITE-6942] Support decorrelated for sub-queries with LIMIT 1 and …

28068af

…OFFSET

iwanttobepowerful force-pushed the zwhtest branch from 8168d98 to 28068af Compare December 23, 2025 22:34

mihaibudiu merged commit 1e92bbe into apache:main Dec 24, 2025
21 checks passed

[CALCITE-6942] Support decorrelated for sub-queries with LIMIT 1 and OFFSET #4633

[CALCITE-6942] Support decorrelated for sub-queries with LIMIT 1 and OFFSET #4633

Uh oh!

Conversation

iwanttobepowerful commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Dec 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iwanttobepowerful Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mihaibudiu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iwanttobepowerful Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iwanttobepowerful commented Dec 22, 2025

Uh oh!

mihaibudiu left a comment

Choose a reason for hiding this comment

Uh oh!

mihaibudiu commented Dec 23, 2025

Uh oh!

sonarqubecloud bot commented Dec 23, 2025

Quality Gate passed

Uh oh!

iwanttobepowerful commented Dec 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

iwanttobepowerful commented Nov 14, 2025 •

edited

Loading

iwanttobepowerful Dec 17, 2025 •

edited

Loading

iwanttobepowerful Dec 18, 2025 •

edited

Loading