imply-cheddar commented on code in PR #12388: URL: https://github.com/apache/druid/pull/12388#discussion_r867594601
########## processing/src/main/java/org/apache/druid/segment/column/DictionaryEncodedStringValueIndex.java: ########## @@ -0,0 +1,59 @@ +/* + * 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.collections.bitmap.ImmutableBitmap; + +import javax.annotation.Nullable; + +/** + * This exposes a 'raw' view into a bitmap value indexes of a string {@link DictionaryEncodedColumn}, allowing + * operation via dictionaryIds, as well as access to lower level details of such a column like value lookup and + * value cardinality. + * + * This interface should only be used when it is beneficial to operate in such a manner, most filter implementations + * should likely be using {@link StringValueSetIndex} or {@link LexicographicalRangeIndex} or some other higher level + * index instead. + */ +public interface DictionaryEncodedStringValueIndex +{ + boolean hasNulls(); + /** + * Get the cardinality of the underlying value dictionary + */ + int getCardinality(); + + /** + * Get the value in the underlying value dictionary of the specified dictionary id + */ + @Nullable + String getValue(int index); + + /** + * Returns the index of "value" in this DictionaryEncodedStringValueIndex, or a negative value, if not present in + * the underlying dictionary + */ + int getIndex(@Nullable String value); + + /** + * Get the {@link ImmutableBitmap} for dictionary id of the underlying dictionary + */ + ImmutableBitmap getBitmap(int idx); Review Comment: Nah, makes sense. ########## processing/src/main/java/org/apache/druid/segment/ColumnSelectorColumnIndexSelector.java: ########## @@ -0,0 +1,91 @@ +/* + * 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.ColumnIndexSupplier; +import org.apache.druid.segment.column.NumericColumn; + +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; + } + + @Override + public ColumnIndexSupplier getIndexSupplier(String column) + { + final ColumnIndexSupplier indexSupplier; + if (isVirtualColumn(column)) { + indexSupplier = virtualColumns.getIndexSupplier(column, columnSelector); + } else { + final ColumnHolder columnHolder = columnSelector.getColumnHolder(column); + if (columnHolder == null || !columnHolder.getCapabilities().isFilterable()) { Review Comment: Seems like a great thing to resolve with bundles. Everything is technically filterable with a value predicate... ########## processing/src/main/java/org/apache/druid/segment/filter/AndFilter.java: ########## @@ -68,44 +73,81 @@ public AndFilter(List<Filter> filters) } public static <T> ImmutableBitmap getBitmapIndex( - BitmapIndexSelector selector, + ColumnIndexSelector selector, BitmapResultFactory<T> bitmapResultFactory, List<Filter> filters ) { - return bitmapResultFactory.toImmutableBitmap(getBitmapResult(selector, bitmapResultFactory, filters)); + return bitmapResultFactory.toImmutableBitmap( + getBitmapIndex(selector, filters).computeBitmapResult(bitmapResultFactory) + ); } - private static <T> T getBitmapResult( - BitmapIndexSelector selector, - BitmapResultFactory<T> bitmapResultFactory, + private static BitmapColumnIndex getBitmapIndex( + ColumnIndexSelector selector, Collection<Filter> filters ) { if (filters.size() == 1) { - return Iterables.getOnlyElement(filters).getBitmapResult(selector, bitmapResultFactory); + return Iterables.getOnlyElement(filters).getBitmapColumnIndex(selector); } - final List<T> bitmapResults = Lists.newArrayListWithCapacity(filters.size()); + final List<BitmapColumnIndex> bitmapColumnIndices = new ArrayList<>(filters.size()); + ColumnIndexCapabilities merged = new SimpleColumnIndexCapabilities(true, true); for (final Filter filter : filters) { - Preconditions.checkArgument(filter.supportsBitmapIndex(selector), - "Filter[%s] does not support bitmap index", filter - ); - final T bitmapResult = filter.getBitmapResult(selector, bitmapResultFactory); - if (bitmapResultFactory.isEmpty(bitmapResult)) { - // Short-circuit. - return bitmapResultFactory.wrapAllFalse(Filters.allFalse(selector)); + final BitmapColumnIndex index = filter.getBitmapColumnIndex(selector); + if (index == null) { Review Comment: When will index be `null`? I'm trying to think of what that semantic could be and the dots are not really connecting for me? It seems like this should never be null and the filter should always do *something* to return a non-null response? I'm thinking it might be related to the whole "not filterable columns" thingie? But, it seems like the logic that we are keeping is that not-filterable-columns are filterable, just all null. Alternatively, if this is an error case, I find myself wondering why there isn't an exception thrown deeper down that gives a meaningful error message about why the error is occurring... -- 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]
