[CALCITE-7266] Optimize the "well-known count bug" correction

[rubenada]

See CALCITE-7266

[suibianwanwank]

I believe there's way for optimization here, but merely considering LEFT and COUNT doesn't seem sufficient in my view.

Test in sub-query.iq:

SELECT deptno, (SELECT CASE WHEN SUM(sal) > 10 then 'VIP' else 'Regular' END expr
                   FROM emp e
                   WHERE d.deptno = e.deptno) a
FROM dept d;

!ok

Currently, this query will be decorrelated by hepPlanner. Suppose we remove HepPlanner.

--- a/core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java
+++ b/core/src/main/java/org/apache/calcite/sql2rel/RelDecorrelator.java
@@ -247,9 +247,7 @@ public static RelNode decorrelateQuery(RelNode rootRel,
         new RelDecorrelator(corelMap,
             cluster.getPlanner().getContext(), relBuilder);

-    RelNode newRootRel = decorrelationRules == null
-        ? decorrelator.removeCorrelationViaRule(rootRel)
-        : decorrelator.removeCorrelationViaRule(rootRel, decorrelationRules);
+    RelNode newRootRel = rootRel;

     if (SQL2REL_LOGGER.isDebugEnabled()) {
       SQL2REL_LOGGER.debug(
@@ -324,7 +322,7 @@ protected RelNode decorrelate(RelNode root) {
     HepPlanner planner = createPlanner(program);

     planner.setRoot(root);
-    root = planner.findBestExp();
+//    root = planner.findBestExp();
     if (SQL2REL_LOGGER.isDebugEnabled()) {
       SQL2REL_LOGGER.debug("Plan before extracting correlated computations:\n"
           + RelOptUtil.toString(root));

After this PR, the decorrelator will return incorrect results.

DEPTNO | A
--------+---------
      10 | VIP
      20 | VIP
      30 | VIP
      40 | NULL

[rubenada]

Thanks for taking a look @suibianwanwank . Maybe I'm doing something wrong, but I'm getting the same results for your sample query with: a) RelDecorrelator disabled, b) RelDecorrelator enabled with current code, c) RelDecorrelator with this PR code:

+--------+---------+
| DEPTNO | A       |
+--------+---------+
|     10 | VIP     |
|     20 | VIP     |
|     30 | VIP     |
|     40 | Regular |
+--------+---------+

PS: I have pushed the test, just to double-check

[suibianwanwank]

As mentioned above, this happens because the RBO decorrelates such patterns in advance. However, I believe the RelDecorrelator framework itself should ensure correctness, rather than relying on rules to pre-handle certain bad cases. After all, pattern-based approaches in decorrelation are inherently limited in what they can cover.

[rubenada]

Ok, I understand now what you mean @suibianwanwank , there's indeed a regression with the proposed patch (observable only if we deactivate the decorrelation via rules step).
Thanks for the feedback @iwanttobepowerful .
It's clear that this idea need some rework.... WIP

BTW It seems there was a similar PR on Spark apache/spark#43341 👀 UPDATE: after a closer look, it seems that PR on Spark was to avoid calling the extra join in case of several Aggregates on the same subtree, whereas the current PR idea is looking at the possibility of avoiding it in case of LeftCorrelate.

[rubenada]

@iwanttobepowerful I haven't looked in detail, but it seems that Spark uses a slightly different approach to deal with the count bug (at least in some cases), with the usage of this "alwaysTrue" value. Notice that some of the manipulations done by Spark might be done (more or less) in Calcite not by the RelDecorrelator itself, but by certain auxiliary rules called via HepPlanner inside the RelDecorrelator, so in Calcite this process is intermingled among rule transformations + the pure decorrelate algorithm itself (which might be not ideal, as stated by @suibianwanwank above).

I'm not entirely sure, but I have the impression that the "LEFT" approach might be valid if the Aggregate result is not further manipulated (as in the counter-example proposed by @suibianwanwank ), i.e. we could avoid the rewrite if the Correlate is LEFT and the Aggregate is directly its right child (this seems to fix the counter-example).
I've just pushed a new commit with this idea.

[suibianwanwank]

It seems like it could be optimized this way🤔 in decorrelateRel(Join):

final Frame rightFrame = getInvoke(oldRight, true, rel, parentPropagatesNullValues);

//to:

final Frame rightFrame = getInvoke(oldRight, true, rel, true);

[rubenada]

You mean on decorrelateRel(Correlate) ?
That actually seems to do the trick in a much cleaner way. It maintains the plans adjusted in the PR, does not fail on the counter-example that you proposed on my initial commit, and it also works as expected on my downstream project's tests if I apply it.
I've pushed this change, cleaning up the previous modifications.

[rubenada]

I could also add the "counter-example" as a unit test in RelDecorrelatorTest, but it would require some minor adjustments in RelDecorrelator to allow running the decorrelation algorithm without any type of rule prior. It's manageable, adds more flexibility (and can be done in a way to keep things backward-compatible)

[suibianwanwank]

 LGTM, An additional thought is whether an inner join would also work.

[rubenada]

Test pushed

[rubenada]

@suibianwanwank @iwanttobepowerful are there other remarks for this change? Shall I squash commits to prepare the merge?

[suibianwanwank]

LGTM!

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud