[GitHub] [calcite] jamesstarr commented on a change in pull request #2385: [CALCITE-4686] SubQueryRemoveRule.matchJoin should correctly rewrite all sub-queries

[GitBox <gi...@apache.org>]

jamesstarr commented on a change in pull request #2385:
URL: https://github.com/apache/calcite/pull/2385#discussion_r682803633



##########
File path: 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)

Review comment:
       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.




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