[GitHub] [calcite] rubenada commented on a change in pull request #1702: Use concurrent test execution by default

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

rubenada commented on a change in pull request #1702: Use concurrent test execution by default
URL: https://github.com/apache/calcite/pull/1702#discussion_r363195740
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableHashJoin.java
 ##########
 @@ -101,44 +101,21 @@ public static EnumerableHashJoin create(
 
   @Override public RelOptCost computeSelfCost(RelOptPlanner planner,
       RelMetadataQuery mq) {
-    double rowCount = mq.getRowCount(this);
-
-    // Joins can be flipped, and for many algorithms, both versions are viable
-    // and have the same cost. To make the results stable between versions of
-    // the planner, make one of the versions slightly more expensive.
-    switch (joinType) {
-    case SEMI:
-    case ANTI:
-      // SEMI and ANTI join cannot be flipped
-      break;
-    case RIGHT:
-      rowCount = RelMdUtil.addEpsilon(rowCount);
-      break;
-    default:
-      if (RelNodes.COMPARATOR.compare(left, right) > 0) {
-        rowCount = RelMdUtil.addEpsilon(rowCount);
-      }
-    }
-
-    // Cheaper if the smaller number of rows is coming from the LHS.
-    // Model this by adding L log L to the cost.
+    RelOptCostFactory costFactory = planner.getCostFactory();
     final double rightRowCount = right.estimateRowCount(mq);
 
 Review comment:
   First of all, thanks for you huge effort on this PR, @vlsi 
   I have a small comment here, according to RelNode#estimateRowCount javadoc: _"don't call this method directly. Instead, use RelMetadataQuery#getRowCount, which gives plugins a chance to override the rel's default ideas about row count"._ I wonder if we could use this PR to replace:
   ```
   final double rightRowCount = right.estimateRowCount(mq);
   final double leftRowCount = left.estimateRowCount(mq);
   ```
   with
   ```
   final double rightRowCount = mq.getRowCount(right);
   final double leftRowCount = mq.getRowCount(left);
   ```
   which seems more correct, according to that javadoc comment. We could do that in EnumerableHashJoin, EnumerableMergeJoin, EnumerableNestedLoopJoin, EnumerableBatchNestedLoopJoin and Correlate (which were already impacted by the PR). What do you think?

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