[GitHub] [druid] gianm commented on a change in pull request #12315: push value range and set index get operations into BitmapIndex

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

gianm commented on a change in pull request #12315:
URL: https://github.com/apache/druid/pull/12315#discussion_r822098147



##########
File path: processing/src/main/java/org/apache/druid/segment/ColumnSelectorBitmapIndexSelector.java
##########
@@ -264,6 +265,78 @@ public ImmutableBitmap getBitmap(int idx)
             return bitmapFactory.makeEmptyImmutableBitmap();
           }
         }
+
+        @Override
+        public ImmutableBitmap getBitmapForValue(@Nullable String value)
+        {
+          if (NullHandling.isNullOrEquivalent(value)) {
+            return bitmapFactory.complement(bitmapFactory.makeEmptyImmutableBitmap(), getNumRows());
+          } else {
+            return bitmapFactory.makeEmptyImmutableBitmap();
+          }
+        }
+
+        @Override
+        public Iterable<ImmutableBitmap> getBitmapsInRange(

Review comment:
       This code block is for a bitmap selector on an all-null column, right? It looks overly complex for that. Shouldn't it check if `matcher` matches `null` and then either return a single `getBitmap(0)`, or no bitmaps, appropriately?

##########
File path: processing/src/main/java/org/apache/druid/query/dimension/PredicateFilteredDimensionSelector.java
##########
@@ -97,7 +97,7 @@ public void inspectRuntimeShape(RuntimeShapeInspector inspector)
   @Override
   public ValueMatcher makeValueMatcher(final Predicate<String> matcherPredicate)
   {
-    final boolean matchNull = predicate.apply(null);
+    final boolean matchNull = predicate.apply(null) && matcherPredicate.apply(null);

Review comment:
       Also a cool bug.

##########
File path: processing/src/main/java/org/apache/druid/segment/serde/StringBitmapIndexColumnPartSupplier.java
##########
@@ -94,6 +101,179 @@ public ImmutableBitmap getBitmap(int idx)
         final ImmutableBitmap bitmap = bitmaps.get(idx);
         return bitmap == null ? bitmapFactory.makeEmptyImmutableBitmap() : bitmap;
       }
+
+      @Override
+      public ImmutableBitmap getBitmapForValue(@Nullable String value)
+      {
+        final int idx = dictionary.indexOf(value);
+        return getBitmap(idx);
+      }
+
+      @Override
+      public Iterable<ImmutableBitmap> getBitmapsInRange(
+          @Nullable String startValue,
+          boolean startStrict,
+          @Nullable String endValue,
+          boolean endStrict
+      )
+      {
+        int startIndex, endIndex;
+        if (startValue == null) {
+          startIndex = 0;
+        } else {
+          final int found = dictionary.indexOf(NullHandling.emptyToNullIfNeeded(startValue));
+          if (found >= 0) {
+            startIndex = startStrict ? found + 1 : found;
+          } else {
+            startIndex = -(found + 1);
+          }
+        }
+
+        if (endValue == null) {
+          endIndex = dictionary.size();
+        } else {
+          final int found = dictionary.indexOf(NullHandling.emptyToNullIfNeeded(endValue));
+          if (found >= 0) {
+            endIndex = endStrict ? found : found + 1;
+          } else {
+            endIndex = -(found + 1);
+          }
+        }
+
+        endIndex = startIndex > endIndex ? startIndex : endIndex;
+        final IntIterator rangeIterator = IntListUtils.fromTo(startIndex, endIndex).iterator();
+        return () -> new Iterator<ImmutableBitmap>()

Review comment:
       This isn't really a proper Iterable. It will only be iterable once; a proper Iterable should be re-iterable. If it doesn't need to be re-iterable, it may be simpler to return an Iterator instead of an Iterable.

##########
File path: processing/src/main/java/org/apache/druid/query/dimension/PredicateFilteredDimensionSelector.java
##########
@@ -81,8 +81,8 @@ public boolean matches()
             nullRow = false;
           }
         }
-        // null should match empty rows in multi-value columns
-        return nullRow && value == null;
+        // null should match empty rows in multi-value columns if predicate matches null
+        return nullRow && value == null && predicate.apply(null);

Review comment:
       Cache `predicate.apply(null)`?
   
   (Cool bug, btw.)




-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org