[GitHub] [calcite] julianhyde commented on a change in pull request #1112: [CALCITE-2930] FilterCorrelateRule on a Correlate with SemiJoinType SEMI (or ANTI) throws IllegalStateException (Ruben Quesada Lopez)

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

julianhyde commented on a change in pull request #1112: [CALCITE-2930] FilterCorrelateRule on a Correlate with SemiJoinType SEMI (or ANTI) throws IllegalStateException  (Ruben Quesada Lopez)
URL: https://github.com/apache/calcite/pull/1112#discussion_r267500175
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/plan/RelOptUtil.java
 ##########
 @@ -2380,6 +2381,8 @@ public static boolean classifyFilters(
         joinRel.getInputs().get(1).getRowType().getFieldList();
     final int nFieldsRight = rightFields.size();
     assert nTotalFields == (joinRel instanceof SemiJoin
 
 Review comment:
   The goal of the assert is to educate future readers and maintainers of this code. In this case, try refactoring the assert to improve clarity.
   
   The maintainer of the code does need to know about all that complexity (in particular how complex the type hierarchy of Join, Correlate and SemiJoin has gotten). Removing the assert is just sweeping the issue under the rug. We should be embarrassed about the complexity and strive to fix it.

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


With regards,
Apache Git Services