kasakrisz commented on code in PR #6712:
URL: https://github.com/apache/hive/pull/6712#discussion_r3894696347


##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/FilterSelectivityEstimator.java:
##########
@@ -1015,8 +1019,8 @@ public static double lessThanSelectivity(KllFloatsSketch 
kll, float value) {
    * @throws IllegalArgumentException if leftValue is equal to rightValue
    */
   public static double betweenSelectivity(KllFloatsSketch kll, float 
leftValue, float rightValue) {
-    // column >= leftValue AND column <= rightValue
-    if (rightValue < leftValue) {
+    // is it possible to fulfill the BETWEEN?
+    if (rightValue < leftValue || rightValue < kll.getMinItem() || leftValue > 
kll.getMaxItem()) {

Review Comment:
   nit: Maybe it is not necessary but do we have test coverage when this is 
true?



##########
ql/src/test/org/apache/hadoop/hive/ql/optimizer/calcite/stats/TestFilterSelectivityEstimator.java:
##########
@@ -272,6 +273,25 @@ public void testIsHistogramAvailableWhenEmptyArray() {
     Assert.assertFalse(isHistogramAvailable(colStatistics));
   }
 
+  /**
+   * Check the KLL resolution.
+   * <p>
+   * The resolution may not be lower than the minimum rank error [HIVE-29365].
+   * </p> */
+  @Test
+  public void testKllResolution() {
+    Assert.assertEquals(MIN_KLL_SELECTIVITY, betweenSelectivity(KLL, 1.20f, 
1.21f), DELTA);
+
+    
doReturn(Collections.singletonList(stats)).when(tableMock).getColStat(Collections.singletonList(0));
+    RexNode filter = REX_BUILDER.makeCall(SqlStdOperatorTable.AND,
+        REX_BUILDER.makeCall(SqlStdOperatorTable.GREATER_THAN, inputRef0, 
literalFloat(1.20f)),
+        REX_BUILDER.makeCall(SqlStdOperatorTable.LESS_THAN, inputRef0, 
literalFloat(1.21f)));
+    filter = simplify(filter);
+    Assert.assertEquals(SqlKind.SEARCH, filter.getKind());
+    FilterSelectivityEstimator estimator = new 
FilterSelectivityEstimator(scan, mq);
+    Assert.assertEquals(MIN_KLL_SELECTIVITY, 
estimator.estimateSelectivity(filter), DELTA);
+  }

Review Comment:
   What value does this part of the test add? Doesn't this eventually calls 
`betweenSelectivity` and `rangedSelectivity`?



##########
ql/src/java/org/apache/hadoop/hive/ql/optimizer/calcite/stats/FilterSelectivityEstimator.java:
##########
@@ -937,12 +937,16 @@ private static Range<Float> 
convertRangeToClosedOpen(Range<Float> boundaries) {
    * @return the selectivity of "val1 &lt;= column &lt; val2"
    */
   static double rangedSelectivity(KllFloatsSketch kll, float val1, float val2) 
{
-    if (val1 >= val2) {
+    if (val1 >= val2 || val2 <= kll.getMinItem() || val1 > kll.getMaxItem()) {

Review Comment:
   nit: Maybe it is not necessary but do we have test coverage when this is 
true?



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to