[GitHub] [calcite] rubenada commented on a diff in pull request #3172: [CALCITE-5670] Assertion error in SemiJoinJoinTransposeRule

["rubenada (via GitHub)" <gi...@apache.org>]

rubenada commented on code in PR #3172:
URL: https://github.com/apache/calcite/pull/3172#discussion_r1174933805


##########
core/src/main/java/org/apache/calcite/rel/rules/SemiJoinJoinTransposeRule.java:
##########
@@ -102,16 +101,18 @@ public SemiJoinJoinTransposeRule(RelBuilderFactory relBuilderFactory) {
 
     // determine which operands below the semi-join are the actual
     // Rels that participate in the semi-join
+    final ImmutableIntList leftKeys = semiJoin.analyzeCondition().leftKeys;
     int nKeysFromX = 0;
     for (int leftKey : leftKeys) {
       if (leftKey < nFieldsX) {
         nKeysFromX++;
       }
     }
 
-    // the keys must all originate from either the left or right;
-    // otherwise, a semi-join wouldn't have been created
-    assert (nKeysFromX == 0) || (nKeysFromX == leftKeys.size());
+    if ((nKeysFromX != 0) && (nKeysFromX != leftKeys.size())) {
+      // We can not push semi-join down if join keys come from different tables under the join.

Review Comment:
   I'd suggest to make the comment a bit less ambiguous, e.g. as you described in the Jira: "We cannot proceed if Semi-Join has keys from both tables of the bottom Join".
   
   Also, I think the Jira title (and hence the PR title and commit message) should also be more specific, e.g.: "Assertion error in SemiJoinJoinTransposeRule when Semi-Join has keys from both tables of the bottom Join"



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

To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org

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