smarter nested column index utilization #13977
Conversation
changes: * adds skipValueRangeIndexScale and skipValuePredicateIndexScale to ColumnConfig (e.g. DruidProcessingConfig) available as system config via druid.processing.indexes.skipValueRangeIndexScale and druid.processing.indexes.skipValuePredicateIndexScale * NestedColumnIndexSupplier uses skipValueRangeIndexScale and skipValuePredicateIndexScale to multiply by the total number of rows to be processed to determine the threshold at which we should no longer consider using bitmap indexes because it will be too many operations * Default values for skipValueRangeIndexScale and skipValuePredicateIndexScale have been initially set to 0.08, but are separate to allow independent tuning * these are not documented on purpose yet because they are kind of hard to explain, the mainly exist to help conduct larger scale experiments than the jmh benchmarks used to derive the initial set of values * these changes provide a pretty sweet performance boost for filter processing on nested columns
|
I decided to do the |
|
Also looks like I might have some legitimate test failures, possibly due to mismatch of behavior between indexes and value matchers on some nested columns 😓 investigating |
|
the different test results seem to be because of different behavior of the predicate index on the bound filter vs the value matcher, the latter looks like it is matching nulls. The results now actually look more consistent with other numeric filters using predicates due to lexicographic sorting. I think there is a bug with the bound filter, it needs to wrap the predicate index in the null matching lower bound in the same way that the range index does. |
Actually the mismatch is between the double predicate being used for the |
| * This is indicated returning null from {@link ColumnIndexSupplier#as(Class)} even though it would have otherwise | ||
| * been able to create a {@link BitmapColumnIndex}. For range indexes on columns where every value has an index, the |
There was a problem hiding this comment.
I'm scared of exposing this behavior through adjusting the return of .as(). I think it would be better to have the columns continue to return a meaningful object for .as() but have the those interfaces that benefit from this be able to opt themselves out of being used for bitmaps. (Or, in some future world, return an equivalent ValueMatcher)
There was a problem hiding this comment.
This was already true of as itself, returning null has always been an indicator that the index we asked for is not available. I just made it a bit clearer in the docs and also leaned into allowing the things as returns to also return null to indicate that an index is not available.
For the types of short circuits done in this PR, i feel like the index supplier has better information to make the decision than anything else above it since it has direct access to the size of dictionary, number of rows to be scanned, etc, but I will think on it
| * This can make many standalone filters faster in cases where the overhead of doing bitmap operations to construct a | ||
| * {@link org.apache.druid.segment.BitmapOffset} or {@link org.apache.druid.segment.vector.BitmapVectorOffset} can | ||
| * often exceed the cost of just using doing a full scan and using a | ||
| * {@link org.apache.druid.query.filter.ValueMatcher}. |
There was a problem hiding this comment.
I think the "many" in "This can make many standalone filters faster" is a bit of a hyperbola. What about "Some standalone filters speed up with this optimization in cases where the overhead of walking the dictionary and combining bitmaps to construct ..."?
| "SELECT SUM(JSON_VALUE(nested, '$.long1' RETURNING BIGINT)) FROM foo WHERE JSON_VALUE(nested, '$.nesteder.double3' RETURNING DOUBLE) < 2000.0 AND JSON_VALUE(nested, '$.nesteder.double3' RETURNING DOUBLE) > 1000.0", | ||
| // 30, 31 | ||
| "SELECT SUM(long1) FROM foo WHERE double3 < 3000.0 AND double3 > 1000.0", | ||
| "SELECT SUM(JSON_VALUE(nested, '$.long1' RETURNING BIGINT)) FROM foo WHERE JSON_VALUE(nested, '$.nesteder.double3' RETURNING DOUBLE) < 3000.0 AND JSON_VALUE(nested, '$.nesteder.double3' RETURNING DOUBLE) > 1000.0", | ||
| // 32,33 | ||
| "SELECT SUM(long1) FROM foo WHERE double3 < 5000.0 AND double3 > 1000.0", | ||
| "SELECT SUM(JSON_VALUE(nested, '$.long1' RETURNING BIGINT)) FROM foo WHERE JSON_VALUE(nested, '$.nesteder.double3' RETURNING DOUBLE) < 5000.0 AND JSON_VALUE(nested, '$.nesteder.double3' RETURNING DOUBLE) > 1000.0", | ||
| // 34,35 smaller cardinality like range filter | ||
| "SELECT SUM(long1) FROM foo WHERE string1 LIKE '1%'", | ||
| "SELECT SUM(JSON_VALUE(nested, '$.long1' RETURNING BIGINT)) FROM foo WHERE JSON_VALUE(nested, '$.nesteder.string1') LIKE '1%'", | ||
| // 36,37 smaller cardinality like predicate filter | ||
| "SELECT SUM(long1) FROM foo WHERE string1 LIKE '%1%'", | ||
| "SELECT SUM(JSON_VALUE(nested, '$.long1' RETURNING BIGINT)) FROM foo WHERE JSON_VALUE(nested, '$.nesteder.string1') LIKE '%1%'", | ||
| // 38-39 moderate cardinality like range | ||
| "SELECT SUM(long1) FROM foo WHERE string5 LIKE '1%'", | ||
| "SELECT SUM(JSON_VALUE(nested, '$.long1' RETURNING BIGINT)) FROM foo WHERE JSON_VALUE(nested, '$.nesteder.string5') LIKE '1%'", | ||
| // 40, 41 big cardinality lex range | ||
| "SELECT SUM(long1) FROM foo WHERE string5 > '1'", | ||
| "SELECT SUM(JSON_VALUE(nested, '$.long1' RETURNING BIGINT)) FROM foo WHERE JSON_VALUE(nested, '$.nesteder.string5') > '1'", | ||
| // 42, 43 big cardinality like predicate filter | ||
| "SELECT SUM(long1) FROM foo WHERE string5 LIKE '%1%'", | ||
| "SELECT SUM(JSON_VALUE(nested, '$.long1' RETURNING BIGINT)) FROM foo WHERE JSON_VALUE(nested, '$.nesteder.string5') LIKE '%1%'", |
There was a problem hiding this comment.
Can you add various sized IN clauses to your benchmark? 10, 100, 1000, 10000, 100000?
After reading the PR, I don't think they should be impacted, but it would be good to know that they aren't as they are an example of a thing that falls back to predicate-like behavior, but which we have seen also cause performance issues when run as a ValueMatcher
There was a problem hiding this comment.
InDimFilter typically only uses Utf8ValueSetIndex and StringValueSetIndex which cannot be impacted by the changes currently in this PR, unless it is using an ExtractionFn which causes it to use a DruidPredicateIndex. I need to figure out how to get something that would plan to something with an extractionFn in here to see how this PR would impact this filter.
| ColumnarInts intsColumn = ints.get(); | ||
| final int size = intsColumn.size(); | ||
| intsColumn.close(); |
There was a problem hiding this comment.
This type of pattern is a bit scary. In this instance given that it's when deserializing the object, it's probably fine, but we've had issues where we thought we were getting a thing and then closing it, only to find out that we were getting a thing that was cached and supposed to live for the lifetime of a query and the immediate close was closing things.
That said, I think we can make this cleaner by making the Supplier<> itself be a class that has a method that knows the number of rows represented by the thing that it supplies. It should be easy enough to implement this for the suppliers used in this case (the number is already on the jvm heap for those objects anyway) and this avoids any need to create an object and close it immediately.
| columnConfig.skipValueRangeIndexScale(), | ||
| columnConfig.skipValuePredicateIndexScale() |
There was a problem hiding this comment.
Why not just pass the columnConfig object itself?
| if (skipPredicateIndex && clazz.equals(DruidPredicateIndex.class)) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Technically, this would indicate that this column cannot implement the given the interface, so the filter in question should fall back to another interface (i.e. it might make the filter to cascade down into trying an even worse strategy). Instead, we should return a DruidPredicateIndex that returns null when asked to compute the index, indicating that it was unable to compute the index. This will make it easier to do this "correctly" when we make the change to have this object able to also return ValueMatcher objects.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
processing/src/main/java/org/apache/druid/segment/AutoTypeColumnMerger.java
Fixed
Show fixed
Hide fixed
| @@ -199,7 +207,9 @@ | |||
| Supplier<FrontCodedIntArrayIndexed> arrayDictionarySupplier, | |||
| Supplier<ColumnarInts> encodedValueColumnSupplier, | |||
| GenericIndexed<ImmutableBitmap> valueIndexes, | |||
| @SuppressWarnings("unused") BitmapFactory bitmapFactory | |||
| @SuppressWarnings("unused") BitmapFactory bitmapFactory, | |||
| @SuppressWarnings("unused") ColumnConfig columnConfig, | |||
Check notice
Code scanning / CodeQL
Useless parameter
| @SuppressWarnings("unused") BitmapFactory bitmapFactory | ||
| @SuppressWarnings("unused") BitmapFactory bitmapFactory, | ||
| @SuppressWarnings("unused") ColumnConfig columnConfig, | ||
| @SuppressWarnings("unused") int numRows |
Check notice
Code scanning / CodeQL
Useless parameter
| this.skipRangeIndexThreshold = (int) Math.ceil(columnConfig.skipValueRangeIndexScale() * numRows); | ||
| this.skipPredicateIndex = localDictionarySupplier.get().size() > Math.ceil(columnConfig.skipValuePredicateIndexScale() * numRows); |
There was a problem hiding this comment.
I think it's a bit nicer to make these methods instead of fields that are computed on every constructor. The computation that's being done is minimal, that's true, but it's better to only do it when needed. I think that if you just store the reference to the ColumnConfig you can pretty easily move this check into private methods. All of the places where you would check for skipping the predicate index, you would probably also be grabbing a localDictionarySupplier anyway, so you can reuse that singular reference too.
| @@ -36,4 +36,12 @@ | |||
| */ | |||
| @Nullable | |||
| BitmapColumnIndex forPredicate(DruidPredicateFactory matcherFactory); | |||
|
|
|||
| static boolean checkSkipThreshold(@Nullable ColumnConfig columnConfig, int numRowsToScan, int dictionaryCardinality) | |||
There was a problem hiding this comment.
shouldSkip? checkSkipThreshold doesn't really tell me how I'm supposed to interpret true/false. Does true mean that it exceeded the threshold and therefore should be skipped? Or does it mean that it did not exceed the threshold and should be used?
|
|
||
| static boolean checkSkipThreshold(ColumnConfig columnConfig, int numRowsToScan, int rangeSize) | ||
| { | ||
| return rangeSize > (int) Math.ceil(columnConfig.skipValueRangeIndexScale() * numRowsToScan); |
There was a problem hiding this comment.
You aren't doing the null check on this one. That intentional?
…nto smarter-nested-column-indexes
Description
A sort of follow-up to #12830, which added
NumericRangeIndexto improve the performance of nested column bound filters on numeric columns, but noted that there were still cases where the cost of bitmap operations overran the cost of just doing a full scan and using value matchers. This is also more or less the same thing described in #3878.This PR attempts to improve this situation by adding cardinality based thresholds for range and predicate indexes to choose to skip using bitmap indexes.
changes:
skipValueRangeIndexScaleandskipValuePredicateIndexScaletoColumnConfig(e.g.DruidProcessingConfig) available as system config viadruid.processing.indexes.skipValueRangeIndexScaleanddruid.processing.indexes.skipValuePredicateIndexScaleNestedColumnIndexSupplierusesskipValueRangeIndexScaleandskipValuePredicateIndexScaleto multiply by the total number of rows to be processed to determine the threshold at which we should no longer consider using bitmap indexes because it will be too many operationsskipValueRangeIndexScaleandskipValuePredicateIndexScalehave been initially set to 0.08, but are separate to allow independent tuningWe might want to consider making these same changes to
DictionaryEncodedStringIndexSupplier, but I have not done so at this time.Benchmarks comparing to regular columns
The really big wins here will be queries like the last one, where some selector or other cheap filters are combined with high cardinality ranges or predicates, since the filters get pushed into being 'post' filters.
Release note
Key changed/added classes in this PR
ColumnConfigNestedFieldLiteralColumnIndexSupplierThis PR has: