review stuff

docs/querying/sql-data-types.md

@@ -155,9 +155,9 @@ values are treated as zeroes. This was the default prior to Druid 28.0.0.
 The [`druid.expressions.useStrictBooleans`](../configuration/index.md#expression-processing-configurations)
 runtime property controls Druid's boolean logic mode. For the most SQL compliant behavior, set this to `true` (the default).
 
-When `druid.expressions.useStrictBooleans = true`, Druid uses three-valued logic for
+When `druid.expressions.useStrictBooleans = true`, Druid uses [three-valued logic](https://en.wikipedia.org/wiki/Three-valued_logic#SQL) for
 [expressions](math-expr.md) evaluation, such as `expression` virtual columns or `expression` filters.
-If `druid.generic.useDefaultValueForNull = false` in addition to `druid.expressions.useStrictBooleans = true`, Druid also uses three-valued logic for native filters.
+If `druid.generic.useDefaultValueForNull = false` (in combination with `druid.expressions.useStrictBooleans = true`), Druid also uses three-valued logic for native filters.
 
 When `druid.expressions.useStrictBooleans = false` (legacy mode), Druid uses two-valued logic for expressions, and if `druid.generic.useDefaultValueForNull = true`, Druid uses two-valued logic for native filters, even if `druid.expressions.useStrictBooleans = true`.
 

extensions-core/druid-bloom-filter/src/main/java/org/apache/druid/query/filter/BloomDimFilter.java

@@ -98,6 +98,8 @@ public Filter toFilter()
         dimension,
         new DruidPredicateFactory()
         {
+          private final boolean isNullUnknown = !bloomKFilter.testBytes(null, 0, 0);
+
           @Override
           public Predicate<String> makeStringPredicate()
           {
@@ -169,7 +171,7 @@ public boolean applyNull()
           @Override
           public boolean isNullInputUnknown()
           {
-            return !bloomKFilter.testBytes(null, 0, 0);
+            return isNullUnknown;
           }
         },
         extractionFn,

processing/src/main/java/org/apache/druid/query/filter/DruidPredicateFactory.java

@@ -22,6 +22,8 @@
 import com.google.common.base.Predicate;
 import org.apache.druid.annotations.SubclassesMustOverrideEqualsAndHashCode;
 import org.apache.druid.java.util.common.UOE;
+import org.apache.druid.query.BitmapResultFactory;
+import org.apache.druid.query.filter.vector.ReadableVectorMatch;
 import org.apache.druid.segment.column.TypeSignature;
 import org.apache.druid.segment.column.ValueType;
 
@@ -59,6 +61,16 @@ default Predicate<Object> makeObjectPredicate()
     return o -> stringPredicate.apply(null);
   }
 
+  /**
+   * Indicator for if null inputs should be considered 'unknown' matches when used for filter matching with
+   * {@link ValueMatcher#matches(boolean)},
+   * {@link org.apache.druid.query.filter.vector.VectorValueMatcher#match(ReadableVectorMatch, boolean)}, or
+   * {@link org.apache.druid.segment.index.BitmapColumnIndex#computeBitmapResult(BitmapResultFactory, boolean)}.
+   *
+   * If returns true, unknown (null) inputs can automatically be considered matches if {@code includeUnknown} is set
+   * to true on these methods, else null inputs should be evaluated against the predicate as any other value to
+   * determine a match
+   */
   default boolean isNullInputUnknown()
   {
     return true;

processing/src/main/java/org/apache/druid/query/filter/EqualityFilter.java

@@ -537,7 +537,7 @@ public ValueMatcher makeDimensionProcessor(DimensionSelector selector, boolean m
     {
       final ExprEval<?> castForComparison = ExprEval.castForEqualityComparison(matchValue, ExpressionType.STRING);
       if (castForComparison == null) {
-        return ValueMatchers.makeAlwaysFalseMatcher(selector, multiValue);
+        return ValueMatchers.makeAlwaysFalseDimensionMatcher(selector, multiValue);
       }
       return ValueMatchers.makeStringValueMatcher(selector, castForComparison.asString(), multiValue);
     }
@@ -547,7 +547,7 @@ public ValueMatcher makeFloatProcessor(BaseFloatColumnValueSelector selector)
     {
       final ExprEval<?> castForComparison = ExprEval.castForEqualityComparison(matchValue, ExpressionType.DOUBLE);
       if (castForComparison == null) {
-        return ValueMatchers.makeAlwaysFalseMatcher(selector);
+        return ValueMatchers.makeAlwaysFalseNumericMatcher(selector);
       }
       return ValueMatchers.makeFloatValueMatcher(selector, (float) castForComparison.asDouble());
     }
@@ -557,7 +557,7 @@ public ValueMatcher makeDoubleProcessor(BaseDoubleColumnValueSelector selector)
     {
       final ExprEval<?> castForComparison = ExprEval.castForEqualityComparison(matchValue, ExpressionType.DOUBLE);
       if (castForComparison == null) {
-        return ValueMatchers.makeAlwaysFalseMatcher(selector);
+        return ValueMatchers.makeAlwaysFalseNumericMatcher(selector);
       }
       return ValueMatchers.makeDoubleValueMatcher(selector, castForComparison.asDouble());
     }
@@ -567,7 +567,7 @@ public ValueMatcher makeLongProcessor(BaseLongColumnValueSelector selector)
     {
       final ExprEval<?> castForComparison = ExprEval.castForEqualityComparison(matchValue, ExpressionType.LONG);
       if (castForComparison == null) {
-        return ValueMatchers.makeAlwaysFalseMatcher(selector);
+        return ValueMatchers.makeAlwaysFalseNumericMatcher(selector);
       }
       return ValueMatchers.makeLongValueMatcher(selector, castForComparison.asLong());
     }

processing/src/main/java/org/apache/druid/query/filter/InDimFilter.java

@@ -564,6 +564,7 @@ public static class InFilterDruidPredicateFactory implements DruidPredicateFacto
     private final Supplier<DruidLongPredicate> longPredicateSupplier;
     private final Supplier<DruidFloatPredicate> floatPredicateSupplier;
     private final Supplier<DruidDoublePredicate> doublePredicateSupplier;
+    private final boolean hasNull;
 
     public InFilterDruidPredicateFactory(
         final ExtractionFn extractionFn,
@@ -572,6 +573,7 @@ public InFilterDruidPredicateFactory(
     {
       this.extractionFn = extractionFn;
       this.values = values;
+      this.hasNull = !values.contains(null);
 
       // As the set of filtered values can be large, parsing them as numbers should be done only if needed, and
       // only once. Pass in a common long predicate supplier to all filters created by .toFilter(), so that we only
@@ -630,7 +632,7 @@ public DruidDoublePredicate makeDoublePredicate()
     @Override
     public boolean isNullInputUnknown()
     {
-      return !values.contains(null);
+      return hasNull;
     }
 
     @Override

processing/src/main/java/org/apache/druid/query/filter/SelectorPredicateFactory.java

@@ -40,10 +40,12 @@ public class SelectorPredicateFactory implements DruidPredicateFactory
   private volatile DruidLongPredicate longPredicate;
   private volatile DruidFloatPredicate floatPredicate;
   private volatile DruidDoublePredicate doublePredicate;
+  private final boolean isNullUnknown;
 
   public SelectorPredicateFactory(@Nullable String value)
   {
     this.value = value;
+    this.isNullUnknown = value != null;
   }
 
   @Override
@@ -76,7 +78,7 @@ public DruidDoublePredicate makeDoublePredicate()
   @Override
   public boolean isNullInputUnknown()
   {
-    return value != null;
+    return isNullUnknown;
   }
 
   private void initLongPredicate()

processing/src/main/java/org/apache/druid/query/filter/ValueMatcher.java

@@ -37,7 +37,7 @@ public interface ValueMatcher extends HotLoopCallee
    * @param includeUnknown mapping for Druid native two state logic system into SQL three-state logic system. If set
    *                       to true, this method should also return true if the result is 'unknown' to be a match, such
    *                       as from the input being null valued. Used primarily to allow
-   *                       {@link org.apache.druid.segment.filter.NotFilter} to invert a match in an SQL compliant
+   *                       {@link org.apache.druid.segment.filter.NotFilter} to invert a match in a SQL compliant
    *                       manner
    * @return true if the current row matches the condition, or is unknown and {@code includeUnknown} is set to true
    */

processing/src/main/java/org/apache/druid/query/filter/vector/VectorValueMatcher.java

@@ -178,8 +178,8 @@ public ReadableVectorMatch match(ReadableVectorMatch mask, boolean includeUnknow
               if (row.size() == 0) {
                 selection[numRows++] = rowNum;
               } else {
-                //noinspection SSBasedInspection
-                for (int j = 0; j < row.size(); j++) {
+                final int size = row.size();
+                for (int j = 0; j < size; j++) {
                   if (NullHandling.isNullOrEquivalent(selector.lookupName(row.get(j)))) {
                     selection[numRows++] = rowNum;
                     break;
@@ -216,8 +216,8 @@ public ReadableVectorMatch match(ReadableVectorMatch mask, boolean includeUnknow
               if (row.size() == 0) {
                 selection[numRows++] = rowNum;
               } else {
-                //noinspection SSBasedInspection
-                for (int j = 0; j < row.size(); j++) {
+                final int size = row.size();
+                for (int j = 0; j < size; j++) {
                   if (row.get(j) == nullId) {
                     selection[numRows++] = rowNum;
                     break;

processing/src/main/java/org/apache/druid/segment/DimensionSelector.java

@@ -187,9 +187,6 @@ static DimensionSelector multiConstant(@Nullable final List<String> values)
       // does not report value cardinality, but otherwise behaves identically when used for grouping or selecting to a
       // normal multi-value dimension selector (getObject on a row with a single value returns the object instead of
       // the list)
-      if (values.get(0) == null) {
-        return NullDimensionSelectorHolder.NULL_DIMENSION_SELECTOR;
-      }
       return constant(values.get(0));
     } else {
       return new ConstantMultiValueDimensionSelector(values);

processing/src/main/java/org/apache/druid/segment/StringDimensionIndexer.java

@@ -391,11 +391,11 @@ public boolean matches(boolean includeUnknown)
               {
                 if (includeUnknown) {
                   IndexedInts row = getRow();
-                  if (row.size() == 0) {
+                  final int size = row.size();
+                  if (size == 0) {
                     return true;
                   }
-                  //noinspection SSBasedInspection
-                  for (int i = 0; i < row.size(); i++) {
+                  for (int i = 0; i < size; i++) {
                     if (row.get(i) == nullValueId) {
                       return true;
                     }

processing/src/main/java/org/apache/druid/segment/column/StringUtf8DictionaryEncodedColumn.java

@@ -20,6 +20,7 @@
 package org.apache.druid.segment.column;
 
 import com.google.common.base.Predicate;
+import com.google.common.collect.Lists;
 import org.apache.druid.java.util.common.StringUtils;
 import org.apache.druid.query.extraction.ExtractionFn;
 import org.apache.druid.query.filter.DruidPredicateFactory;
@@ -798,14 +799,14 @@ public Object[] getObjectVector()
 
       for (int i = 0; i < offset.getCurrentVectorSize(); i++) {
         IndexedInts ithRow = vector[i];
-        if (ithRow.size() == 0) {
+        final int size = ithRow.size();
+        if (size == 0) {
           strings[i] = null;
-        } else if (ithRow.size() == 1) {
+        } else if (size == 1) {
           strings[i] = lookupName(ithRow.get(0));
         } else {
-          List<String> row = new ArrayList<>(ithRow.size());
-          // noinspection SSBasedInspection
-          for (int j = 0; j < ithRow.size(); j++) {
+          List<String> row = Lists.newArrayListWithCapacity(size);
+          for (int j = 0; j < size; j++) {
             row.add(lookupName(ithRow.get(j)));
           }
           strings[i] = row;

processing/src/main/java/org/apache/druid/segment/filter/BoundFilter.java

@@ -343,6 +343,7 @@ static class BoundDimFilterDruidPredicateFactory implements DruidPredicateFactor
     private final Supplier<DruidLongPredicate> longPredicateSupplier;
     private final Supplier<DruidFloatPredicate> floatPredicateSupplier;
     private final Supplier<DruidDoublePredicate> doublePredicateSupplier;
+    private final boolean isNullUnknown;
 
     BoundDimFilterDruidPredicateFactory(ExtractionFn extractionFn, BoundDimFilter boundDimFilter)
     {
@@ -351,6 +352,11 @@ static class BoundDimFilterDruidPredicateFactory implements DruidPredicateFactor
       this.longPredicateSupplier = boundDimFilter.getLongPredicateSupplier();
       this.floatPredicateSupplier = boundDimFilter.getFloatPredicateSupplier();
       this.doublePredicateSupplier = boundDimFilter.getDoublePredicateSupplier();
+      if (extractionFn != null) {
+        this.isNullUnknown = !doesMatch(extractionFn.apply(null), boundDimFilter);
+      } else {
+        this.isNullUnknown = !doesMatch(null, boundDimFilter);
+      }
     }
 
     @Override
@@ -402,10 +408,7 @@ public DruidDoublePredicate makeDoublePredicate()
     @Override
     public boolean isNullInputUnknown()
     {
-      if (extractionFn != null) {
-        return !doesMatch(extractionFn.apply(null), boundDimFilter);
-      }
-      return !doesMatch(null, boundDimFilter);
+      return isNullUnknown;
     }
 
     @Override

processing/src/main/java/org/apache/druid/segment/filter/ColumnComparisonFilter.java

@@ -48,6 +48,11 @@
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
 
+/**
+ * Compares values between columns, first converting them all to strings. This filter behaves like "not distinct from",
+ * e.g. given columns x and y, the SQL equivalent would be "x is not distinct from y" (and so ignores
+ * {@code includeUnknown}).
+ */
 public class ColumnComparisonFilter implements Filter
 {
   private final List<DimensionSpec> dimensions;
@@ -87,8 +92,6 @@ public static ValueMatcher makeValueMatcher(final List<Supplier<String[]>> value
       @Override
       public boolean matches(boolean includeUnknown)
       {
-        // todo (clint): what to do about includeUnknown
-
         // Keep all values to compare against each other.
         String[][] values = new String[valueGetters.size()][];
 

processing/src/main/java/org/apache/druid/segment/filter/DimensionPredicateFilter.java

@@ -146,12 +146,14 @@ static class DelegatingStringPredicateFactory implements DruidPredicateFactory
     private final Predicate<String> baseStringPredicate;
     private final DruidPredicateFactory predicateFactory;
     private final ExtractionFn extractionFn;
+    private final boolean isNullUnknown;
 
     DelegatingStringPredicateFactory(DruidPredicateFactory predicateFactory, ExtractionFn extractionFn)
     {
       this.predicateFactory = predicateFactory;
       this.baseStringPredicate = predicateFactory.makeStringPredicate();
       this.extractionFn = extractionFn;
+      this.isNullUnknown = !baseStringPredicate.apply(extractionFn.apply(null));
     }
 
     @Override
@@ -220,7 +222,7 @@ public boolean applyNull()
     @Override
     public boolean isNullInputUnknown()
     {
-      return !baseStringPredicate.apply(extractionFn.apply(null));
+      return isNullUnknown;
     }
 
     @Override

processing/src/main/java/org/apache/druid/segment/filter/PredicateValueMatcherFactory.java

@@ -99,7 +99,7 @@ public ValueMatcher makeArrayProcessor(
       if (matchesNull) {
         return ValueMatchers.allTrue();
       }
-      return ValueMatchers.makeAlwaysFalseMatcher(selector);
+      return ValueMatchers.makeAlwaysFalseObjectMatcher(selector);
     } else {
       // use the object predicate
       final Predicate<Object[]> predicate = predicateFactory.makeArrayPredicate(columnCapabilities);
@@ -140,7 +140,7 @@ public ValueMatcher makeComplexProcessor(BaseObjectColumnValueSelector<?> select
       if (predicateMatches) {
         return ValueMatchers.allTrue();
       }
-      return ValueMatchers.makeAlwaysFalseMatcher(selector);
+      return ValueMatchers.makeAlwaysFalseObjectMatcher(selector);
     } else if (!isNumberOrString(selector.classOfObject())) {
       // if column is definitely not a number of string, use the object predicate
       final Predicate<Object> predicate = predicateFactory.makeObjectPredicate();