[CALCITE-4686] SubQueryRemoveRule.matchJoin should correctly rewrite all sub-queries #2385

jamesstarr · 2021-03-31T22:42:47Z

Reworking SubQueryRemoveRule.matchJoin to:

Validate join type before rewriting
Correctly shift filter and then rewrite join condition on right side of the join
Rewrite all subqueries in ON conditions

Testing and refactoring:

Moved Shifter from RelBuilder to RelOptUtil, so the logic can be reused by SubQueryRemoveRule.matchJoin.
Enabled RelOptRulesTest.testExpandJoinIn and RelOptRulesTest.testExpandJoinInComposite.
Adding RelOptRulesTest. testJoinOnMultipleCorrelatedQueries

Moving Shifter to from RelBuilder to RelOptUtil. Treating all variables from the left side as correlated variables then apply them subqueries to thes right side.

zabetak · 2021-05-25T15:54:02Z

Hey @jamesstarr , thanks for working on this. Going quickly over the changeset I get the impression that the PR tries to achieve multiple things. Can you highlight/outline the goals of this PR either here or under CALCITE-1045?

jamesstarr · 2021-05-25T19:16:57Z

@zabetak, This PR is focused on handling left joins with correlated queries in ON clauses.

Joins with correlated queries are rewritten too as join with the right side contain filter node and correlate node to handle the join's filter in SubQueryRemovelRule. The filter is checked in a loop for correlation ID to handle multiple subqueries. Right and full outer joins are not rewritten because they would logically be incorrect. Shifter, a util class in RelBuilder, was moved to RelOptUtil so the existing logic could be used in SubQueryRemoveRule. RelBuilder also now only considers a join correlated if an ID is provided and actually referenced is found on the right side to the ID.

jamesstarr · 2021-06-21T20:20:59Z

@zabetak, do you have any further feed back?

core/src/main/java/org/apache/calcite/tools/RelBuilder.java

Turn back on RelOptRulesTest.testExpandJoinIn and testExpandJoinInComposite.

Using RelBuilder over rexbuilder in SubQueryRemoveRule. Making CorrelationId final. Not state builder twice.

jamesstarr · 2021-07-08T00:03:34Z

@zabetak I pulled in master, create separate ticket and cleaned up the code, and rewrote the pull request comment. Let me know if any further changes are needed.

zabetak · 2021-07-10T05:40:06Z

Hey @jamesstarr , thanks for pushing this forward. I think it is in good shape right now. I will try to review and merge soon but not sure if I will make it during the next week.

core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml

zabetak · 2021-07-29T09:55:05Z

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

+    switch (join.getJoinType()) {
+    case INNER:
+    case LEFT:
+    case ANTI:
+    case SEMI:
+      break;
+    default:
+      //Incorrect queries will be produced for full and right joins.
+      return;
+    }


Since we cannot treat RIGHT and FULL outer joins I think it makes sense to skip calling onMatch method altogether. Is it possible to move the check as rule predicate or apply the check as part of RelOptRule#matches?

zabetak · 2021-07-29T09:59:35Z

core/src/main/java/org/apache/calcite/plan/RelOptUtil.java

+  /** Shifts a predicate's inputs to the left, replacing early
+   * ones with references to a {@link RexCorrelVariable}. */
+  public static RexNode correlateLeftShiftRight(RexBuilder rexBuilder,


It is not clear what the method does from the javadoc or method name. I know that you moved existing code but given that now the method is public it would be nice if we could improve the doc. You can possibly include sub-plan examples in the javadoc to aid in the understanding of the method.

zabetak · 2021-07-29T10:01:44Z

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

+    RexNode condition = RelOptUtil.correlateLeftShiftRight(builder.getRexBuilder(),
+        join.getLeft(), id, join.getRight(), join.getCondition());
+    boolean found = false;
+    while (true) {


I've seen that matchFilter shares some similarities with this newly added code. Is it correct to assume that we need to do this also for matchProject? Should we log a follow-up JIRA?

match join and as the predicate subquery join rule. Improving javadoc for RelOptUtil.

zabetak · 2021-08-04T10:21:24Z

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

@@ -695,7 +715,7 @@ private static void matchJoin(SubQueryRemoveRule rule, RelOptRuleCall call) {
    Config JOIN = EMPTY
        .withOperandSupplier(b ->
            b.operand(Join.class)
-                .predicate(RexUtil.SubQueryFinder::containsSubQuery)
+                .predicate(SubQueryRemoveRule::hasSubQueryAndJoinTypeIsRewritable)


Two other alternatives for the predicate; not sure if they are better than the current proposal I just like when methods do only one thing:

(join) -> isRewritable(join) && RexUtil.SubQueryFinder.containsSubQuery(join) (join) -> !join.getJoinType().generatesNullsOnLeft && RexUtil.SubQueryFinder.containsSubQuery(join)

the second is not 100% equivalent but I am not sure if we intent to cover CROSS, LEFT_SEMI_JOIN, COMMA, if it ever reaches this rule.

Not necessarily requesting a change, leaving the final decision to @jamesstarr !

The call is reused in assert in matchJoin. So moving to the first one would mean, that it is repeated twice the && containsSubQuery twice while still having a helper function. Concerning !join.getJoinType().generatesNullsOnLeft(), the whole expression would have to be repeated twice, and while unlikely to change, it could allow for drift if some one wants to change the restrictiveness. Furthermore, the phrasing is not particular intuitive.

I will rename hasSubQueryAndJoinTypeIsRewritable to containsSubQueryAndJoinTypeIsRewritable to match the existing verbs and change it use generatesNullsOnLeft() so it will likely work if any further join types are added.

zabetak · 2021-08-04T10:22:22Z

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

+    for (RexSubQuery subQuery = RexUtil.SubQueryFinder.find(condition); subQuery != null;
+        subQuery = RexUtil.SubQueryFinder.find(condition)) {


I like the change from while (true) to for :)

zabetak · 2021-08-04T10:25:50Z

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

+      final Set<CorrelationId>  variablesSet =
+          RelOptUtil.getVariablesUsed(subQuery.rel);
+      final RelOptUtil.Logic logic =
+          LogicVisitor.find(RelOptUtil.Logic.TRUE, ImmutableList.of(condition), subQuery);
+      int offset = builder.peek().getRowType().getFieldCount();
+      final RexNode target = rule.apply(subQuery, variablesSet, logic,
+          builder, 1, offset);
+      final RexShuttle shuttle = new ReplaceSubQueryShuttle(subQuery, target);
+      condition = shuttle.apply(condition);


NIT: The final at this level make the code more verbose with small benefit given that the scope is very small. In some cases removing it could make things fit in one line.

I am normally not a big fan of final in this localized of context when all variables are treated as immutable, but when some variables are mutated, having final on all the immutable variables signals that the mutable variables are in fact mutable.

I will try to shift things to a single where possible.

zabetak · 2021-08-04T10:52:22Z

core/src/main/java/org/apache/calcite/plan/RelOptUtil.java

+  /** Rewrites an {@link RexNode} on top of a correlated join to the rights context by
+   * shifting {@link RexInputRef} if they are from the right side, and rewriting
+   * {@link RexInputRef} to field accesses of {@link RexCorrelVariable}.
+   * @param left Rel fields to be correlated
+   * @param id id to correlate
+   * @param right Rel field to be shifted to the right
+   * @param rexNode expression to be rewritten
+   * @return An expression
+   */


Suggestion below:

Transforms a join condition over two inputs to a correlated condition over the right input. Replaces references on the left relation with correlating variables and shifts references on the right relation as needed to be able to put the condition over the right relation. TODO: Add example similar to the one below. <pre>{@code Join[cond=..] TableScan[EMP] TableScan[DEPT] }</pre> <pre>{@code Operator TableScan[EMP] Filter[cond=..] TableScan[DEPT] }</pre>

Rename method to either:

transformJoinConditionToCorrelate(RexNode condition, RelNode left, RelNode right, CorrelationId id, RexBuilder builder) transformJoinToCorrelateCondition(...) transformConditionFromJoinToCorrelate(...)

@zabetak, I have followed up on your requests: adding an example and renaming the method. Please let me know if you want any further changes.

transformJoinConditionToCorrelate. Also, updating comment and adding an example. Reflowing SubQueryRemoveRule.matchJoin to use single lines where possible. Making local variable offset final for consistency. Renaming SubQueryRemoveRule.hasSubQueryAndJoinTypeIsRewritable to containsSubQueryAndJoinTypeIsRewritable. Changing checking join type to use generatesNullsOnLeft.

…rrelatedSubQueries

jamesstarr · 2021-08-20T21:51:15Z

@zabetak, Could you please merge this.

jamesstarr · 2021-08-24T18:37:28Z

@zabetak, I really do not want this going to the graveyard of calcite pull requests.

zabetak · 2021-08-25T06:05:22Z

Hi @jamesstarr , I am very busy lately but as soon as I get the chance I will make a final pass and merge it.

core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml

olivrlee · 2023-07-11T18:46:00Z

What is the status on this PR? Was it ready to be merged? cc @zabetak
I've been trying to follow the trail of relevant JIRA tickets

jamesstarr · 2023-07-11T23:03:30Z

What is the status on this PR? Was it ready to be merged? cc @zabetak I've been trying to follow the trail of relevant JIRA tickets

Never mind, I had removed the conflicting test some time ago.

~~There is bug in one of the test due to nested queries not being correlated correctly. I have a fix for the nested queries here: https://github.com//pull/3042~~

jamesstarr marked this pull request as draft March 31, 2021 22:44

jamesstarr force-pushed the CALCITE-1045 branch 2 times, most recently from b24f579 to e8f2271 Compare April 15, 2021 16:28

jamesstarr changed the title ~~[CALCITE-1045] WIP Supporting SubQueryRemoveRule for Joins~~ [CALCITE-1045] Supporting SubQueryRemoveRule for Joins Apr 15, 2021

jamesstarr marked this pull request as ready for review April 15, 2021 18:29

jamesstarr and others added 3 commits April 30, 2021 10:49

[CALCITE-1045] Sub-Queries in ON clauses

803d5f0

Moving Shifter to from RelBuilder to RelOptUtil. Treating all variables from the left side as correlated variables then apply them subqueries to thes right side.

Removing unused import

1940109

Relbuilder.correlate emits a join when the correlate value is not used.

932975f

jamesstarr force-pushed the CALCITE-1045 branch 2 times, most recently from de96a1d to 932975f Compare April 30, 2021 18:01

zabetak self-assigned this May 25, 2021

zabetak reviewed Jun 22, 2021

View reviewed changes

core/src/main/java/org/apache/calcite/tools/RelBuilder.java Show resolved Hide resolved

Merge branch 'master' of github.com:apache/calcite into CALCITE-1045

3b244cf

jamesstarr force-pushed the CALCITE-1045 branch from c6c6a08 to 3b244cf Compare July 7, 2021 21:01

jamesstarr added 4 commits July 7, 2021 14:11

Merge branch 'master' of github.com:apache/calcite into CALCITE-1045

15a833d

Removing extra line

f1a714a

Reverting back bad merge for SubqueryRemoveRule

f3013ae

Turn back on RelOptRulesTest.testExpandJoinIn and testExpandJoinInComposite.

Removing resources from bad merge in RelOptRulesTest

3204e0e

Using RelBuilder over rexbuilder in SubQueryRemoveRule. Making CorrelationId final. Not state builder twice.

jamesstarr changed the title ~~[CALCITE-1045] Supporting SubQueryRemoveRule for Joins~~ [CALCITE-4686] SubQueryRemoveRule.matchJoin should correctly rewrite all sub-queries Jul 7, 2021

zabetak reviewed Jul 16, 2021

View reviewed changes

core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml Show resolved Hide resolved

zabetak reviewed Jul 29, 2021

View reviewed changes

jamesstarr added 2 commits July 29, 2021 10:48

Adding hasSubQueryAndJoinTypeIsRewritable to check assert in

6ef7e72

match join and as the predicate subquery join rule. Improving javadoc for RelOptUtil.

Adding whitespace after for.

a945fa4

zabetak reviewed Aug 4, 2021

View reviewed changes

jamesstarr added 2 commits August 4, 2021 10:47

Add back in a test that was deleted in a bad merge.

eb46552

Renaming RelOptRulesTest.testJoinOnCorrelated to testJoinOnMultipleCo…

9ef2b51

…rrelatedSubQueries

zabetak reviewed Sep 20, 2021

View reviewed changes

core/src/test/resources/org/apache/calcite/test/RelOptRulesTest.xml Show resolved Hide resolved

zabetak added the discussion-in-jira There's open discussion in JIRA to be resolved before proceeding with the PR label Sep 21, 2021

zabetak force-pushed the master branch from f14cf4c to dcbc493 Compare March 10, 2022 09:13

julianhyde force-pushed the main branch from fa65a2e to 1226d1a Compare June 20, 2022 20:27

asfgit force-pushed the main branch from 9fc50f2 to e2f949d Compare September 10, 2022 16:37

asfgit force-pushed the main branch from f8f8a51 to a326bd2 Compare January 25, 2023 07:39

julianhyde force-pushed the main branch 2 times, most recently from 8a5cf83 to cf7f71b Compare June 8, 2023 21:21

tanclary force-pushed the main branch from 4804912 to 00db001 Compare September 6, 2023 00:42

libenchao force-pushed the main branch from 47db81a to 0be8eae Compare November 10, 2023 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-4686] SubQueryRemoveRule.matchJoin should correctly rewrite all sub-queries #2385

[CALCITE-4686] SubQueryRemoveRule.matchJoin should correctly rewrite all sub-queries #2385

jamesstarr commented Mar 31, 2021 •

edited

Loading

zabetak commented May 25, 2021

jamesstarr commented May 25, 2021

jamesstarr commented Jun 21, 2021

jamesstarr commented Jul 8, 2021

zabetak commented Jul 10, 2021

zabetak Jul 29, 2021

zabetak Jul 29, 2021

zabetak Jul 29, 2021

zabetak Aug 4, 2021

jamesstarr Aug 4, 2021

zabetak Aug 4, 2021

zabetak Aug 4, 2021

jamesstarr Aug 4, 2021 •

edited

Loading

zabetak Aug 4, 2021

jamesstarr Aug 11, 2021

jamesstarr commented Aug 20, 2021

jamesstarr commented Aug 24, 2021

zabetak commented Aug 25, 2021

olivrlee commented Jul 11, 2023

jamesstarr commented Jul 11, 2023 •

edited

Loading

		for (RexSubQuery subQuery = RexUtil.SubQueryFinder.find(condition); subQuery != null;
		subQuery = RexUtil.SubQueryFinder.find(condition)) {

[CALCITE-4686] SubQueryRemoveRule.matchJoin should correctly rewrite all sub-queries #2385

Are you sure you want to change the base?

[CALCITE-4686] SubQueryRemoveRule.matchJoin should correctly rewrite all sub-queries #2385

Conversation

jamesstarr commented Mar 31, 2021 • edited Loading

zabetak commented May 25, 2021

jamesstarr commented May 25, 2021

jamesstarr commented Jun 21, 2021

jamesstarr commented Jul 8, 2021

zabetak commented Jul 10, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesstarr Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesstarr commented Aug 20, 2021

jamesstarr commented Aug 24, 2021

zabetak commented Aug 25, 2021

olivrlee commented Jul 11, 2023

jamesstarr commented Jul 11, 2023 • edited Loading

jamesstarr commented Mar 31, 2021 •

edited

Loading

jamesstarr Aug 4, 2021 •

edited

Loading

jamesstarr commented Jul 11, 2023 •

edited

Loading