imply-cheddar commented on code in PR #12388:
URL: https://github.com/apache/druid/pull/12388#discussion_r846943761
##########
processing/src/main/java/org/apache/druid/query/filter/InDimFilter.java:
##########
@@ -280,11 +280,18 @@ public Filter toFilter()
}
@Override
- public <T> T getBitmapResult(BitmapIndexSelector selector,
BitmapResultFactory<T> bitmapResultFactory)
+ public <T> T getBitmapResult(ColumnIndexSelector selector,
BitmapResultFactory<T> bitmapResultFactory)
{
if (extractionFn == null) {
- final BitmapIndex bitmapIndex = selector.getBitmapIndex(dimension);
- return
bitmapResultFactory.unionDimensionValueBitmaps(getBitmapIterable(values,
bitmapIndex));
+ final StringValueSetIndex valueSetIndex = selector.as(dimension,
StringValueSetIndex.class);
+ if (valueSetIndex == null) {
Review Comment:
It strikes me that this could be `null` because the column doesn't exist OR
it could be `null` because the column exists, but doesn't support
`StringValueSetIndex`. the `.as()` method should likely not actually take a
String parameter and instead the call should be split into 2 calls, one for the
column and then one for the interface. In this way, the semantics of `null`
becomes extremely clear.
##########
processing/src/main/java/org/apache/druid/query/filter/Filter.java:
##########
@@ -150,7 +129,7 @@ default VectorValueMatcher
makeVectorMatcher(VectorColumnSelectorFactory factory
*
* @return true if this Filter supports selectivity estimation, false
otherwise.
*/
- boolean supportsSelectivityEstimation(ColumnSelector columnSelector,
BitmapIndexSelector indexSelector);
+ boolean supportsSelectivityEstimation(ColumnSelector columnSelector,
ColumnIndexSelector indexSelector);
Review Comment:
Once we move the selectivityEstimation value as suggested elsewhere, this
method can be completely eliminated.
##########
processing/src/main/java/org/apache/druid/segment/ColumnSelectorColumnIndexSelector.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.segment;
+
+import org.apache.druid.collections.bitmap.BitmapFactory;
+import org.apache.druid.query.filter.ColumnIndexSelector;
+import org.apache.druid.segment.column.ColumnCapabilities;
+import org.apache.druid.segment.column.ColumnHolder;
+import org.apache.druid.segment.column.ColumnIndexCapabilities;
+import org.apache.druid.segment.column.ColumnIndexSupplier;
+import org.apache.druid.segment.column.NumericColumn;
+import org.apache.druid.segment.column.SimpleColumnIndexCapabilities;
+
+import javax.annotation.Nullable;
+
+/**
+ */
+public class ColumnSelectorColumnIndexSelector implements ColumnIndexSelector
+{
+ private final BitmapFactory bitmapFactory;
+ private final VirtualColumns virtualColumns;
+ private final ColumnSelector columnSelector;
+
+ public ColumnSelectorColumnIndexSelector(
+ final BitmapFactory bitmapFactory,
+ final VirtualColumns virtualColumns,
+ final ColumnSelector index
+ )
+ {
+ this.bitmapFactory = bitmapFactory;
+ this.virtualColumns = virtualColumns;
+ this.columnSelector = index;
+ }
+
+ @Override
+ public int getNumRows()
+ {
+ try (final NumericColumn column = (NumericColumn)
columnSelector.getColumnHolder(ColumnHolder.TIME_COLUMN_NAME).getColumn()) {
+ return column.length();
+ }
+ }
+
+ @Override
+ public BitmapFactory getBitmapFactory()
+ {
+ return bitmapFactory;
+ }
+
+ @Nullable
+ @Override
+ public <T> ColumnIndexCapabilities getIndexCapabilities(
+ String column,
+ Class<T> clazz
+ )
+ {
+ final ColumnIndexSupplier indexSupplier;
+ if (isVirtualColumn(column)) {
+ indexSupplier = virtualColumns.getIndexSupplier(column, columnSelector);
+ } else {
+ final ColumnHolder columnHolder = columnSelector.getColumnHolder(column);
+ if (columnHolder == null ||
!columnHolder.getCapabilities().isFilterable()) {
+ // if a column doesn't exist or isn't filterable, return true so that
callers can use a value matcher to
+ // either make an all true or all false bitmap if the matcher matches
null
+ return SimpleColumnIndexCapabilities.getConstant();
+ }
+ indexSupplier = columnHolder.getIndexSupplier();
Review Comment:
It would seem like if it's "not filterable" then it could return `null` from
`getIndexSupplier()` and we could key off of that to determine if a column is
not filterable?
##########
processing/src/main/java/org/apache/druid/query/filter/Filter.java:
##########
@@ -75,13 +77,13 @@ default ImmutableBitmap getBitmapIndex(BitmapIndexSelector
selector)
* with reasonable sacrifice of the accuracy.
* As a result, the estimated selectivity might be different from the exact
value.
*
- * @param indexSelector Object used to retrieve bitmap indexes
+ * @param indexSelector Object used to retrieve indexes
*
* @return an estimated selectivity ranging from 0 (filter selects no rows)
to 1 (filter selects all rows).
*
- * @see Filter#getBitmapIndex(BitmapIndexSelector)
+ * @see Filter#getBitmapIndex(ColumnIndexSelector)
*/
- double estimateSelectivity(BitmapIndexSelector indexSelector);
+ double estimateSelectivity(ColumnIndexSelector indexSelector);
Review Comment:
Similar to some other suggestions made, I think that selectivity estimation
is a function of the bitmap that is returned. We should perhaps move it to an
object that is returned when the bitmap is gotten instead of having it here.
Looking at, e.g. the BoundsFilter implementation, this method effectively does
all of the work to build the bitmap only to then figure out if it's okay to
actually use the bitmap. If it is okay, it's going to do all of the work to
build the bitmap again. If we have to do the work anyway, might as well just
do it once and the current API doesn't actually allow the code to do that...
Maybe we don't fix it in this PR and we fix it in a follow-up that
additionally adds the various things for IndexCapabilities, but either way we
should put a target on this method as it's an abstraction that can be improved.
##########
processing/src/main/java/org/apache/druid/segment/column/ColumnIndexSupplier.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.segment.column;
+
+import javax.annotation.Nullable;
+
+/**
+ * Provides indexes and information about them ({@link
ColumnIndexCapabilities}) for a column. Indexes which satisfy
+ * the {@link org.apache.druid.query.filter.Filter} used in a {@link
org.apache.druid.query.Query}, allow the query
+ * engine to construct {@link org.apache.druid.segment.data.Offset} (and
+ * {@link org.apache.druid.segment.vector.VectorOffset}) to build {@link
org.apache.druid.segment.Cursor}
+ * (and {@link org.apache.druid.segment.vector.VectorCursor}). This allows the
engine to only scan the rows which match
+ * the filter values, instead of performing a full scan of the column and
using a
+ * {@link org.apache.druid.query.filter.ValueMatcher}
+ * (or {@link org.apache.druid.query.filter.vector.VectorValueMatcher}) to
filter the values (or in addition to, in the
+ * case of indexes which {@link ColumnIndexCapabilities#isExact()} is false,
which must still use a value matcher
+ * when using an index).
+ */
+public interface ColumnIndexSupplier
+{
+ /**
+ * Get the {@link ColumnIndexCapabilities} for the specified type of index.
If the index does not exist
+ * this method will return null. A null return value from this method
indicates that an index of the desired type
+ * in unavailable.
+ */
+ @Nullable
+ <T> ColumnIndexCapabilities getIndexCapabilities(Class<T> clazz);
Review Comment:
As suggested in other comments, perhaps move this to be fields on the Bitmap
object returned by the index object instead of generic representations of the
column as a whole.
##########
processing/src/main/java/org/apache/druid/query/filter/ColumnIndexSelector.java:
##########
@@ -19,33 +19,33 @@
package org.apache.druid.query.filter;
-import com.google.errorprone.annotations.MustBeClosed;
import org.apache.druid.collections.bitmap.BitmapFactory;
-import org.apache.druid.collections.bitmap.ImmutableBitmap;
-import org.apache.druid.collections.spatial.ImmutableRTree;
import org.apache.druid.segment.ColumnInspector;
-import org.apache.druid.segment.column.BitmapIndex;
-import org.apache.druid.segment.column.ColumnCapabilities;
-import org.apache.druid.segment.data.CloseableIndexed;
+import org.apache.druid.segment.column.ColumnIndexCapabilities;
import javax.annotation.Nullable;
/**
*/
-public interface BitmapIndexSelector extends ColumnInspector
+public interface ColumnIndexSelector extends ColumnInspector
{
- @MustBeClosed
- @Nullable
- CloseableIndexed<String> getDimensionValues(String dimension);
-
- @Deprecated
- ColumnCapabilities.Capable hasMultipleValues(String dimension);
-
int getNumRows();
+
BitmapFactory getBitmapFactory();
+
+ /**
+ * Get the {@link ColumnIndexCapabilities} of a column for the specified
type of index. If the index does not exist
+ * this method will return null. Note that 'missing' columns should in fact
return a non-null value from this method
+ * to allow for filters to use 'nil' bitmaps if the filter matches nulls, in
order to produce an all true or all
+ * false index.
+ */
@Nullable
- BitmapIndex getBitmapIndex(String dimension);
+ <T> ColumnIndexCapabilities getIndexCapabilities(String column, Class<T>
clazz);
+
+ /**
+ * Get the specified type of index for the specified column. {@link
#getIndexCapabilities(String, Class)} should
+ * be called prior to this method to distinguish 'missing' columns from
columns without indexes.
+ */
@Nullable
- ImmutableBitmap getBitmapIndex(String dimension, String value);
- ImmutableRTree getSpatialIndex(String dimension);
+ <T> T as(String column, Class<T> clazz);
Review Comment:
The fact that `.as()` has two arguments makes this call site ambiguous and
introduces superfluous methods like `getIndexCapabilities()`. We should
instead just do a `.get(String column)` that returns `ColumnIndexSupplier`
(this can be used to determine if the column exists or not) and then we can use
the `ColumnIndexSupplier` object to call `.getIndexAs()` and get the
implementation. If we do this, we don't need `getIndexCapabilities()` at all.
##########
processing/src/main/java/org/apache/druid/segment/FilteredOffset.java:
##########
@@ -56,7 +57,10 @@ public final class FilteredOffset extends Offset
rowOffsetMatcherFactory
);
} else {
- if (postFilter.shouldUseBitmapIndex(bitmapIndexSelector)) {
+ final ColumnIndexCapabilities capabilities =
postFilter.getIndexCapabilities(bitmapIndexSelector);
+ // we only consider "exact" filters here, because if false, we've
already used the bitmap index for the base
+ // offset
+ if (capabilities != null && capabilities.isExact()) {
Review Comment:
Instead of checking capabilities, this can very easily be
```
BitmapIndex index = postFilter.getBitmapIndex(bitmapIndexSelector);
if (index != null) {
```
##########
processing/src/main/java/org/apache/druid/query/filter/BooleanFilter.java:
##########
@@ -70,30 +72,27 @@ ValueMatcher makeMatcher(
return allColumns;
}
+ @Nullable
@Override
- default boolean supportsBitmapIndex(BitmapIndexSelector selector)
- {
- for (Filter filter : getFilters()) {
- if (!filter.supportsBitmapIndex(selector)) {
- return false;
- }
- }
- return true;
- }
-
- @Override
- default boolean shouldUseBitmapIndex(BitmapIndexSelector selector)
+ default ColumnIndexCapabilities getIndexCapabilities(ColumnIndexSelector
selector)
{
+ ColumnIndexCapabilities capabilities = null;
for (Filter f : getFilters()) {
- if (!f.shouldUseBitmapIndex(selector)) {
- return false;
+ ColumnIndexCapabilities other = f.getIndexCapabilities(selector);
+ if (other == null) {
+ return null;
Review Comment:
Is it really okay to short-circuit all of the other filters in this case?
##########
processing/src/main/java/org/apache/druid/segment/column/ColumnIndexSupplier.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.segment.column;
+
+import javax.annotation.Nullable;
+
+/**
+ * Provides indexes and information about them ({@link
ColumnIndexCapabilities}) for a column. Indexes which satisfy
+ * the {@link org.apache.druid.query.filter.Filter} used in a {@link
org.apache.druid.query.Query}, allow the query
+ * engine to construct {@link org.apache.druid.segment.data.Offset} (and
+ * {@link org.apache.druid.segment.vector.VectorOffset}) to build {@link
org.apache.druid.segment.Cursor}
+ * (and {@link org.apache.druid.segment.vector.VectorCursor}). This allows the
engine to only scan the rows which match
+ * the filter values, instead of performing a full scan of the column and
using a
+ * {@link org.apache.druid.query.filter.ValueMatcher}
+ * (or {@link org.apache.druid.query.filter.vector.VectorValueMatcher}) to
filter the values (or in addition to, in the
+ * case of indexes which {@link ColumnIndexCapabilities#isExact()} is false,
which must still use a value matcher
+ * when using an index).
+ */
+public interface ColumnIndexSupplier
+{
+ /**
+ * Get the {@link ColumnIndexCapabilities} for the specified type of index.
If the index does not exist
+ * this method will return null. A null return value from this method
indicates that an index of the desired type
+ * in unavailable.
+ */
+ @Nullable
+ <T> ColumnIndexCapabilities getIndexCapabilities(Class<T> clazz);
+
+ /**
+ * Get a column 'index' of the specified type. If the index of the desired
type is not available, this method will
+ * return null. If the value is non-null, the index may be used for the
eventual construction of an
+ * {@link org.apache.druid.segment.data.Offset} to form the basis of a
{@link org.apache.druid.segment.Cursor}
+ * (or {@link org.apache.druid.segment.vector.VectorOffset} and {@link
org.apache.druid.segment.vector.VectorCursor})
+ * which can greatly reduce the total number of rows which need to be
scanned and processed.
+ */
+ @Nullable
+ <T> T getIndex(Class<T> clazz);
Review Comment:
Naming nit: `.getIndexAs()` to indicate that we aren't trying to access a
field called Index, but we are asking the object to "morph" itself into the
specified class.
##########
processing/src/main/java/org/apache/druid/segment/ColumnSelectorColumnIndexSelector.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.segment;
+
+import org.apache.druid.collections.bitmap.BitmapFactory;
+import org.apache.druid.query.filter.ColumnIndexSelector;
+import org.apache.druid.segment.column.ColumnCapabilities;
+import org.apache.druid.segment.column.ColumnHolder;
+import org.apache.druid.segment.column.ColumnIndexCapabilities;
+import org.apache.druid.segment.column.ColumnIndexSupplier;
+import org.apache.druid.segment.column.NumericColumn;
+import org.apache.druid.segment.column.SimpleColumnIndexCapabilities;
+
+import javax.annotation.Nullable;
+
+/**
+ */
+public class ColumnSelectorColumnIndexSelector implements ColumnIndexSelector
+{
+ private final BitmapFactory bitmapFactory;
+ private final VirtualColumns virtualColumns;
+ private final ColumnSelector columnSelector;
+
+ public ColumnSelectorColumnIndexSelector(
+ final BitmapFactory bitmapFactory,
+ final VirtualColumns virtualColumns,
+ final ColumnSelector index
+ )
+ {
+ this.bitmapFactory = bitmapFactory;
+ this.virtualColumns = virtualColumns;
+ this.columnSelector = index;
+ }
+
+ @Override
+ public int getNumRows()
+ {
+ try (final NumericColumn column = (NumericColumn)
columnSelector.getColumnHolder(ColumnHolder.TIME_COLUMN_NAME).getColumn()) {
+ return column.length();
+ }
+ }
+
+ @Override
+ public BitmapFactory getBitmapFactory()
+ {
+ return bitmapFactory;
+ }
+
+ @Nullable
+ @Override
+ public <T> ColumnIndexCapabilities getIndexCapabilities(
+ String column,
+ Class<T> clazz
+ )
+ {
+ final ColumnIndexSupplier indexSupplier;
+ if (isVirtualColumn(column)) {
+ indexSupplier = virtualColumns.getIndexSupplier(column, columnSelector);
+ } else {
+ final ColumnHolder columnHolder = columnSelector.getColumnHolder(column);
+ if (columnHolder == null ||
!columnHolder.getCapabilities().isFilterable()) {
+ // if a column doesn't exist or isn't filterable, return true so that
callers can use a value matcher to
+ // either make an all true or all false bitmap if the matcher matches
null
+ return SimpleColumnIndexCapabilities.getConstant();
+ }
+ indexSupplier = columnHolder.getIndexSupplier();
+ }
+ if (indexSupplier == null) {
+ // column exists, but doesn't have an index supplier
+ return null;
+ }
+ return indexSupplier.getIndexCapabilities(clazz);
+ }
+
+ @Nullable
+ @Override
+ public <T> T as(String column, Class<T> clazz)
Review Comment:
Looking at the implementation for `as()` and `getColumnIndexCapabilities()`,
I really think this should just be `getColumnIndexSupplier()` where `null`
effectively means "not filterable". We then have a choice of whether we return
a `NullColumnIndexSupplier` for cases where the dimension doesn't exist (i.e.
at this level, act like it does exist by returning something that knows how to
deal with all nulls) or also have `null` mean that the column didn't exist.
Personally, I tend to think that we should return a `NullColumnIndexSupplier`
and let the callers believe the column actually existed, but maybe there's
reasons not to do that.
##########
processing/src/main/java/org/apache/druid/segment/ColumnSelectorColumnIndexSelector.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.segment;
+
+import org.apache.druid.collections.bitmap.BitmapFactory;
+import org.apache.druid.query.filter.ColumnIndexSelector;
+import org.apache.druid.segment.column.ColumnCapabilities;
+import org.apache.druid.segment.column.ColumnHolder;
+import org.apache.druid.segment.column.ColumnIndexCapabilities;
+import org.apache.druid.segment.column.ColumnIndexSupplier;
+import org.apache.druid.segment.column.NumericColumn;
+import org.apache.druid.segment.column.SimpleColumnIndexCapabilities;
+
+import javax.annotation.Nullable;
+
+/**
+ */
+public class ColumnSelectorColumnIndexSelector implements ColumnIndexSelector
+{
+ private final BitmapFactory bitmapFactory;
+ private final VirtualColumns virtualColumns;
+ private final ColumnSelector columnSelector;
+
+ public ColumnSelectorColumnIndexSelector(
+ final BitmapFactory bitmapFactory,
+ final VirtualColumns virtualColumns,
+ final ColumnSelector index
+ )
+ {
+ this.bitmapFactory = bitmapFactory;
+ this.virtualColumns = virtualColumns;
+ this.columnSelector = index;
+ }
+
+ @Override
+ public int getNumRows()
+ {
+ try (final NumericColumn column = (NumericColumn)
columnSelector.getColumnHolder(ColumnHolder.TIME_COLUMN_NAME).getColumn()) {
+ return column.length();
+ }
+ }
+
+ @Override
+ public BitmapFactory getBitmapFactory()
+ {
+ return bitmapFactory;
+ }
+
+ @Nullable
+ @Override
+ public <T> ColumnIndexCapabilities getIndexCapabilities(
+ String column,
+ Class<T> clazz
+ )
+ {
+ final ColumnIndexSupplier indexSupplier;
+ if (isVirtualColumn(column)) {
+ indexSupplier = virtualColumns.getIndexSupplier(column, columnSelector);
+ } else {
+ final ColumnHolder columnHolder = columnSelector.getColumnHolder(column);
+ if (columnHolder == null ||
!columnHolder.getCapabilities().isFilterable()) {
+ // if a column doesn't exist or isn't filterable, return true so that
callers can use a value matcher to
+ // either make an all true or all false bitmap if the matcher matches
null
+ return SimpleColumnIndexCapabilities.getConstant();
Review Comment:
Why can the callers not user the fact that it is `null` to then make the
necessary bitmap?
##########
processing/src/main/java/org/apache/druid/segment/column/ColumnIndexCapabilities.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.segment.column;
+
+import org.apache.druid.query.filter.ColumnIndexSelector;
+
+/**
+ * Sort of like {@link ColumnCapabilities}, except for indexes supplied by
{@link ColumnIndexSelector}, provides
+ * information for how query processing may use indexes.
+ */
+public interface ColumnIndexCapabilities
Review Comment:
The values that this class represent (`isInvertible` and `isExact`) are not
functions of a column, but are functions of a specific Bitmap returned by said
column. `ColumnIndexCapabilities` belongs to a column and is acting like it
represents all things from that column, it cannot, however do that. To handle
this case, we should instead move these things onto the actually
`ImmutableBitmap` object or some other wrapper object that gets return and
indicates what all can be done. I would recommend that put them on an object
that also contains the `ValueMatcher` for when a ValueMatcher is required.
As is, this interface and API structure is not actually in alignment with
the physical realities of the data and the extensions, so we will, once again,
just be fighting with it. So, we should fix this before merging. One way to
fix it (and my recommendation) is to just remove the Capabilities altogether
here and re-introduce `isInvertible` and `isExact` in the PR that also
introduces a combination of `BitmapIndex` and `ValueMatchers`.
##########
processing/src/main/java/org/apache/druid/query/search/UseIndexesStrategy.java:
##########
@@ -251,21 +256,23 @@ public IndexOnlyExecutor(
continue;
}
- final BitmapIndex bitmapIndex = columnHolder.getBitmapIndex();
- Preconditions.checkArgument(bitmapIndex != null,
+ final ColumnIndexSupplier indexSupplier =
columnHolder.getIndexSupplier();
+ Preconditions.checkArgument(indexSupplier != null,
"Dimension [%s] should support bitmap
index", dimension.getDimension()
);
-
+ final DictionaryEncodedStringValueIndex bitmapIndex =
+ indexSupplier.getIndex(DictionaryEncodedStringValueIndex.class);
ExtractionFn extractionFn = dimension.getExtractionFn();
if (extractionFn == null) {
extractionFn = IdentityExtractionFn.getInstance();
}
- for (int i = 0; i < bitmapIndex.getCardinality(); ++i) {
- String dimVal = extractionFn.apply(bitmapIndex.getValue(i));
+ // if index is null here, it means the column is missing
Review Comment:
Shouldn't you check `indexSupplier == null` above to see if the column is
missing?
--
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]
