[GitHub] [calcite] zabetak commented on a change in pull request #2077: [CALCITE-4132] Estimate the NDV accurately (Liya Fan)

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

zabetak commented on a change in pull request #2077:
URL: https://github.com/apache/calcite/pull/2077#discussion_r457175453



##########
File path: core/src/main/java/org/apache/calcite/rel/metadata/RelMdUtil.java
##########
@@ -293,7 +293,7 @@ public static void setLeftRightBitmaps(
    * @param numSelected number selected from the domain
    * @return number of distinct values for subset selected
    */
-  public static Double numDistinctVals(
+  public static Double numDistinctValsApprox(

Review comment:
       Is this method used? If not it should be removed.

##########
File path: core/src/test/java/org/apache/calcite/rel/RelCollationTest.java
##########
@@ -98,6 +101,12 @@
     assertThat(RelCollations.containsOrderless(collations, Arrays.asList(0)), is(false));
   }
 
+  @Test void testNumDistinctVals() {

Review comment:
       The test is not relevant to collation so it doesn't fit well in this class. It may be better to create another class `RelMdDistinctRowCountTest` or `RelMdUtilTest` (depending on what exactly you are going to test) and put it inside. 
   
   Also it may be a good opportunity to add a bit more tests around this metric (big/small domain, selected combinations and boundary cases).




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