-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Incorporate estimatedComputeCost
into all BitmapColumnIndex
classes.
#17125
Incorporate estimatedComputeCost
into all BitmapColumnIndex
classes.
#17125
Conversation
0a2d6ac
to
e2ffa6a
Compare
return ListFilteredDimensionSpec.filterAllowList( | ||
values, | ||
factory.makeDimensionSelector(delegate), | ||
delegate.getExtractionFn() != null |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
DimensionSpec.getExtractionFn
return ListFilteredDimensionSpec.filterDenyList( | ||
values, | ||
factory.makeDimensionSelector(delegate), | ||
delegate.getExtractionFn() != null |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
DimensionSpec.getExtractionFn
ee92478
to
3ee3900
Compare
3ee3900
to
8eeb5e0
Compare
benchmarks/src/test/java/org/apache/druid/benchmark/query/SqlNestedDataBenchmark.java
Outdated
Show resolved
Hide resolved
processing/src/main/java/org/apache/druid/segment/index/IndexedUtf8ValueIndexes.java
Outdated
Show resolved
Hide resolved
processing/src/test/java/org/apache/druid/segment/filter/FilterBundleTest.java
Outdated
Show resolved
Hide resolved
@Override | ||
public int estimatedComputeCost() | ||
{ | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like this is mainly used for null value index, should this be 1 to be consistent with the equality indexes, like ValueIndexes.forValue
, since the null indexes still have a bitmap?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right this index seems mainly for null index, so it's just 1 bitmap with no union. When I looked up forValue
seems like it's possible there're two bitmaps (one for the value and one for null) with one union. That's why i decided 0 for this, and 1 for other SimpleBitmapIndex
instances. I feel SimpleImmutableBitmapIndex
is slightly cheaper since no binary search for dictionary and no bitmap union.
@@ -1204,6 +1256,9 @@ private abstract class NestedVariantIndexes | |||
final FrontCodedIntArrayIndexed arrayDictionary = globalArrayDictionarySupplier == null | |||
? null | |||
: globalArrayDictionarySupplier.get(); | |||
// For every single String value, we need to look up indexes from stringDictionary, longDictionary and | |||
// doubleDictionary. Hence, the compute cost for one value is 3. | |||
static final int INDEX_COMPUTE_SCALE = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i actually think 1 would probably be ok here too since we still only use a single bitmap, but this is also fine
@@ -199,7 +199,7 @@ public String getFormatString() | |||
// 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%'", | |||
// 44, 45 big cardinality like filter + selector filter | |||
// 44, 45 big cardinality like filter + selector filter with different ordering |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran the tests for 44,45,46,47. The scores are very similar, since we've the ordering:
Benchmark (query) (rowsPerSegment) (schema) (stringEncoding) (vectorize) Mode Cnt Score Error Units
SqlNestedDataBenchmark.querySql 44 5000000 explicit none force avgt 5 7.753 ± 0.380 ms/op
SqlNestedDataBenchmark.querySql 44 5000000 auto none force avgt 5 8.023 ± 0.940 ms/op
SqlNestedDataBenchmark.querySql 45 5000000 explicit none force avgt 5 7.976 ± 0.735 ms/op
SqlNestedDataBenchmark.querySql 45 5000000 auto none force avgt 5 7.820 ± 0.863 ms/op
SqlNestedDataBenchmark.querySql 46 5000000 explicit none force avgt 5 7.495 ± 0.279 ms/op
SqlNestedDataBenchmark.querySql 46 5000000 auto none force avgt 5 7.861 ± 0.691 ms/op
SqlNestedDataBenchmark.querySql 47 5000000 explicit none force avgt 5 7.577 ± 0.405 ms/op
SqlNestedDataBenchmark.querySql 47 5000000 auto none force avgt 5 7.735 ± 0.491 ms/op
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comparing with upstream/master branch:
Benchmark (query) (rowsPerSegment) (schema) (stringEncoding) (vectorize) Mode Cnt Score Error Units
SqlNestedDataBenchmark.querySql 44 5000000 explicit none force avgt 5 215.894 ± 2.712 ms/op
SqlNestedDataBenchmark.querySql 44 5000000 auto none force avgt 5 208.718 ± 5.545 ms/op
SqlNestedDataBenchmark.querySql 45 5000000 explicit none force avgt 5 221.829 ± 7.157 ms/op
SqlNestedDataBenchmark.querySql 45 5000000 auto none force avgt 5 216.632 ± 2.712 ms/op
SqlNestedDataBenchmark.querySql 46 5000000 explicit none force avgt 5 7.431 ± 0.286 ms/op
SqlNestedDataBenchmark.querySql 46 5000000 auto none force avgt 5 7.396 ± 0.186 ms/op
SqlNestedDataBenchmark.querySql 47 5000000 explicit none force avgt 5 7.487 ± 0.287 ms/op
SqlNestedDataBenchmark.querySql 47 5000000 auto none force avgt 5 7.451 ± 0.203 ms/op
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤘 🚀
processing/src/main/java/org/apache/druid/segment/filter/BoundFilter.java
Outdated
Show resolved
Hide resolved
{ | ||
return Integer.MAX_VALUE; | ||
} | ||
int estimatedComputeCost(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i know this isn't new in this PR, but i feel like maybe the javadoc should mention that the estimated cost should be related to the number of bitmap operations that need to be performed to compute the filter bitmap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added more explanation on this.
…Filter.java Co-authored-by: Clint Wylie <[email protected]>
…es. (apache#17125) changes: * filter index processing is now automatically ordered based on estimated 'cost', which is approximated based on how many expected bitmap operations are required to construct the bitmap used for the 'offset' * cursorAutoArrangeFilters context flag now defaults to true, but can be set to false to disable cost based filter index sorting
…es. (#17125) (#17172) changes: * filter index processing is now automatically ordered based on estimated 'cost', which is approximated based on how many expected bitmap operations are required to construct the bitmap used for the 'offset' * cursorAutoArrangeFilters context flag now defaults to true, but can be set to false to disable cost based filter index sorting
Incorporate
estimatedComputeCost
into allBitmapColumnIndex
classes.Description
In #17055, we added
estimatedIndexComputeCost
field toFilterBundle.Builder
class, which would be used to sort child filters in AndFilter/OrFilter. The goal is to compute less expensive filters first, thereby enhancing query performance. This PR aims to incorporateestimatedComputeCost
into allBitmapColumnIndex
classes, serving as an initial measure for estimating the cost of filters.An overall approach of estimating the cost is to assess how many bitmaps we expect to union or intersect. Dictionary lookup would also incur some overhead.
AllTrueBitmapColumnIndex
,AllFalseBitmapColumnIndex
,AllUnknownBitmapColumnIndex
. The cost is 0.SimpleImmutableBitmapIndex
. The cost is 0.SimpleBitmapColumnIndex
instances. It generally involves one binary search, and maybe union with null bitmap. The cost is 1.ListFilteredDruidPredicateIndexes
to useDictionaryScanningBitmapIndex
instead.DictionaryRangeScanningBitmapIndex
. The cost is the size of scanning range.DictionaryScanningBitmapIndex
. The cost is the size of dictionary.BaseValueSetIndexesFromIterable
.buildBitmapColumnIndexFromSortedIteratorScan
. Cost is max of value set size and dictionary size.buildBitmapColumnIndexFromSortedIteratorBinarySearch
. Cost is value set size.buildBitmapColumnIndexFromIteratorBinarySearch
. Cost is value set size.NestedVariantStringValueSetIndexes
. For each value, we need to look up from three global dictionaries (stringDictionary, longDictionary and doubleDictionary), and one local dictionary. Therefore we define a base cost of 3 (INDEX_COMPUTE_SCALE
). The cost of building index from value set would be3 * size of value set
.IsBooleanFilter
andNotFilter
, the cost is the same asbaseIndex
.AndFilter
andOrFilter
, cost is 0 since the bundle would sum up the cost of its child filters.SpatialFilter
. There's no good way to define cost, so putting down max integer for now, it'll always be evaluated last.Turned on
CURSOR_AUTO_ARRANGE_FILTERS
by default in this PR.While working on this PR, I had some ideas on refactoring some of the usages of
BitmapColumnIndex
, specifically:SimpleImmutableBitmapIndex
andSimpleBitmapColumnIndex
. TheBitmapColumnIndex
interface defines a methodcomputeBitmapResult(BitmapResultFactory<T> bitmapResultFactory, boolean includeUnknown)
. TheincludeUnknown
param is not used inSimpleImmutableBitmapIndex
because the bitmap is already pre-computed. Maybe we could have anUnknownBitmapSupplier
instead.DictionaryBinarySearchBitmapIndex
class which could extract the iterator definition out, maybe can replace all usages ofSimpleImmutableBitmapDelegatingIterableIndex
.Benchmark comparison
I'm comparing the results with
CURSOR_AUTO_ARRANGE_FILTERS
flag enabled and disabled.query 1
Flag off
Flag on
query 2
Flag off
Flag on
query 3
Flag off
Flag on
This PR has: