You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/04/09 17:06:52 UTC

[GitHub] [calcite] zabetak opened a new pull request #2393: [CALCITE-4574] Wrong/Invalid plans when using RelBuilder#join with correlations

zabetak opened a new pull request #2393:
URL: https://github.com/apache/calcite/pull/2393


   1. Gather required columns from the right side after the handling of the
   filter to account for those columns present in the join condition.
   2. Predicate for SEMI/ANTI join types should be pushed to the right
   cause otherwise columns in the condition referencing the right side will
   be invalid.
   3. Throw IllegalArgumentException for non-supported correlation joins.
   4. Update existing tests with the correct plans
   5. Add new tests for RelBuilder#join with correlation covering all join
   types.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] jamesstarr commented on a change in pull request #2393: [CALCITE-4574] Wrong/Invalid plans when using RelBuilder#join with correlations

Posted by GitBox <gi...@apache.org>.
jamesstarr commented on a change in pull request #2393:
URL: https://github.com/apache/calcite/pull/2393#discussion_r613587690



##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -2368,24 +2368,28 @@ public RelBuilder join(JoinRelType joinType, RexNode condition,
     }
     if (correlate) {
       final CorrelationId id = Iterables.getOnlyElement(variablesSet);
-      final ImmutableBitSet requiredColumns =
-          RelOptUtil.correlationColumns(id, right.rel);
       if (!RelOptUtil.notContainsCorrelation(left.rel, id, Litmus.IGNORE)) {
         throw new IllegalArgumentException("variable " + id
             + " must not be used by left input to correlation");
       }
+      // Correlate does not have an ON clause.
       switch (joinType) {
       case LEFT:
-        // Correlate does not have an ON clause.
-        // For a LEFT correlate, predicate must be evaluated first.
-        // For INNER, we can defer.
+      case SEMI:
+      case ANTI:
+        // For a LEFT/SEMI/ANTI, predicate must be evaluated first.
         stack.push(right);
         filter(condition.accept(new Shifter(left.rel, id, right.rel)));
         right = stack.pop();
         break;
-      default:
+      case INNER:
+        // For INNER, we can defer.
         postCondition = condition;
+        break;
+      default:
+        throw new IllegalArgumentException("Correlated " + joinType + " join is not supported");
       }
+      final ImmutableBitSet requiredColumns = RelOptUtil.correlationColumns(id, right.rel);

Review comment:
       I am not sure if this is place to do it, but should we detect if correlated variables only occur in the condition and emit a normal join if that is case?




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] rubenada commented on pull request #2393: [CALCITE-4574] Wrong/Invalid plans when using RelBuilder#join with correlations

Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2393:
URL: https://github.com/apache/calcite/pull/2393#issuecomment-826862344


   LGTM


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] zabetak closed pull request #2393: [CALCITE-4574] Wrong/Invalid plans when using RelBuilder#join with correlations

Posted by GitBox <gi...@apache.org>.
zabetak closed pull request #2393:
URL: https://github.com/apache/calcite/pull/2393


   


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [calcite] zabetak commented on a change in pull request #2393: [CALCITE-4574] Wrong/Invalid plans when using RelBuilder#join with correlations

Posted by GitBox <gi...@apache.org>.
zabetak commented on a change in pull request #2393:
URL: https://github.com/apache/calcite/pull/2393#discussion_r613992188



##########
File path: core/src/main/java/org/apache/calcite/tools/RelBuilder.java
##########
@@ -2368,24 +2368,28 @@ public RelBuilder join(JoinRelType joinType, RexNode condition,
     }
     if (correlate) {
       final CorrelationId id = Iterables.getOnlyElement(variablesSet);
-      final ImmutableBitSet requiredColumns =
-          RelOptUtil.correlationColumns(id, right.rel);
       if (!RelOptUtil.notContainsCorrelation(left.rel, id, Litmus.IGNORE)) {
         throw new IllegalArgumentException("variable " + id
             + " must not be used by left input to correlation");
       }
+      // Correlate does not have an ON clause.
       switch (joinType) {
       case LEFT:
-        // Correlate does not have an ON clause.
-        // For a LEFT correlate, predicate must be evaluated first.
-        // For INNER, we can defer.
+      case SEMI:
+      case ANTI:
+        // For a LEFT/SEMI/ANTI, predicate must be evaluated first.
         stack.push(right);
         filter(condition.accept(new Shifter(left.rel, id, right.rel)));
         right = stack.pop();
         break;
-      default:
+      case INNER:
+        // For INNER, we can defer.
         postCondition = condition;
+        break;
+      default:
+        throw new IllegalArgumentException("Correlated " + joinType + " join is not supported");
       }
+      final ImmutableBitSet requiredColumns = RelOptUtil.correlationColumns(id, right.rel);

Review comment:
       Correlation has slightly different semantics than join so I am not sure if it is always safe to perform this transformation. It may be but we should tackle it in another JIRA/PR to avoid complicating this case.




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org