clintropolis commented on code in PR #12388: URL: https://github.com/apache/druid/pull/12388#discussion_r866532462
########## 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: i don't think it is necessary; `BitmapColumnIndex` is specifically for filter processing, while `DictionaryEncodedStringValueIndex` should never be used by filters because it makes assumptions about how the column is represented that filters shouldn't concern themselves with. It is intended to be a 'leaky' abstraction to the way dictionary encoded columns with bitmap value indexes store things since it basically provides raw access. This class is only used by `SegmentAnalyzer`, `ListFilteredVirtualColumn`, search query strategies, and segment merging code, all of which are doing lots of invasive things in general to know things about the segments. Part of me wants to detach this from the `ColumnIndexSupplier` and make it its own thing so that it can't be for filtering and these invasive things are done, but on the other hand, it needs all of the same things that `ColumnIndexSupplier` has, and all existing methods were using the same machinery as filters, so its a bit disruptive. I will maybe consider this as a follow up, change though, so that way we can tie this class more strongly to dictionary encoded string columns, since it isn't really valid for any other type of column. -- 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]
