Direct UTF-8 access for "in" filters. #12517

gianm · 2022-05-13T02:03:13Z

Directly related:

InDimFilter: Store stored Strings (in ValuesSet) plus sorted UTF-8
ByteBuffers (in valuesUtf8). Use valuesUtf8 whenever possible. If
necessary, the input set is copied into a ValuesSet. Much logic is
simplified, because we always know what type the values set will be.
I think that there won't even be an efficiency loss in most cases.
InDimFilter is most frequently created by deserialization, and this
patch updates the JsonCreator constructor to deserialize
directly into a ValuesSet.
Add Utf8ValueSetIndex, which InDimFilter uses to avoid UTF-8 decodes
during index lookups.
Add unsigned comparator to ByteBufferUtils and use it in
GenericIndexed.BYTE_BUFFER_STRATEGY. This is important because UTF-8
bytes can be compared as bytes if, and only if, the comparison
is unsigned.
Add specialization to GenericIndexed.singleThreaded().indexOf that
avoids needless ByteBuffer allocations.
Clarify that objects returned by ColumnIndexSupplier.as are not
thread-safe. DictionaryEncodedStringIndexSupplier now calls
singleThreaded() on all relevant GenericIndexed objects, saving
a ByteBuffer allocation per access.

Also:

Fix performance regression in LikeFilter: since push value range and set index get operations into BitmapIndex #12315, it applied
the suffix matcher to all values in range even for type MATCH_ALL.
Add ObjectStrategy.canCompare() method. This fixes LikeFilterBenchmark,
which was broken due to calls to strategy.compare in
GenericIndexed.fromIterable.

Benchmarks:

Improvements to "in" filters (due to using UTF-8 comparisons) and "like" filters (due to fixing a perf regression).

patch

Benchmark                         (dictionarySize)  (filterSize)  Mode  Cnt     Score    Error  Units
InFilterBenchmark.doFilter                 1000000         10000  avgt   10  3522.824 ± 35.642  us/op

Benchmark                            (cardinality)                Mode  Cnt     Score    Error  Units
LikeFilterBenchmark.matchLikePrefix        1000000                avgt   10   660.164 ± 19.946  us/op

9177515 (master)

Benchmark                         (dictionarySize)  (filterSize)  Mode  Cnt     Score    Error  Units
InFilterBenchmark.doFilter                 1000000         10000  avgt   10  6723.385 ± 59.267  us/op

Benchmark                            (cardinality)                Mode  Cnt     Score    Error  Units
LikeFilterBenchmark.matchLikePrefix        1000000                avgt   10  1348.478 ± 12.959  us/op

deb69d1 (before #12388)

Benchmark                         (dictionarySize)  (filterSize)  Mode  Cnt     Score    Error  Units
InFilterBenchmark.doFilter                 1000000         10000  avgt   10  6803.080 ± 87.903  us/op

Benchmark                            (cardinality)                Mode  Cnt     Score   Error  Units
LikeFilterBenchmark.matchLikePrefix        1000000                avgt   10   717.091 ± 3.451  us/op

Directly related: 1) InDimFilter: Store stored Strings (in ValuesSet) plus sorted UTF-8 ByteBuffers (in valuesUtf8). Use valuesUtf8 whenever possible. If necessary, the input set is copied into a ValuesSet. Much logic is simplified, because we always know what type the values set will be. I think that there won't even be an efficiency loss in most cases. InDimFilter is most frequently created by deserialization, and this patch updates the JsonCreator constructor to deserialize directly into a ValuesSet. 2) Add Utf8ValueSetIndex, which InDimFilter uses to avoid UTF-8 decodes during index lookups. 3) Add unsigned comparator to ByteBufferUtils and use it in GenericIndexed.BYTE_BUFFER_STRATEGY. This is important because UTF-8 bytes can be compared as bytes if, and only if, the comparison is unsigned. 4) Add specialization to GenericIndexed.singleThreaded().indexOf that avoids needless ByteBuffer allocations. 5) Clarify that objects returned by ColumnIndexSupplier.as are not thread-safe. DictionaryEncodedStringIndexSupplier now calls singleThreaded() on all relevant GenericIndexed objects, saving a ByteBuffer allocation per access. Also: 1) Fix performance regression in LikeFilter: since apache#12315, it applied the suffix matcher to all values in range even for type MATCH_ALL. 2) Add ObjectStrategy.canCompare() method. This fixes LikeFilterBenchmark, which was broken due to calls to strategy.compare in GenericIndexed.fromIterable.

lgtm-com · 2022-05-13T03:21:57Z

This pull request introduces 2 alerts when merging 7f2af63 into 9177515 - view on LGTM.com

new alerts:

2 for Cast from abstract to concrete collection

lgtm-com · 2022-05-13T15:25:39Z

This pull request introduces 2 alerts when merging 301057b into 7ab2170 - view on LGTM.com

new alerts:

2 for Cast from abstract to concrete collection

gianm · 2022-05-14T16:47:17Z

Added tests and made some updates to satisfy the coverage and style checkers.

lgtm-com · 2022-05-14T18:03:19Z

This pull request introduces 2 alerts when merging c1a2d94 into 7ab2170 - view on LGTM.com

new alerts:

2 for Cast from abstract to concrete collection

lgtm-com · 2022-05-14T20:59:32Z

This pull request introduces 2 alerts when merging f7ea7e5 into 7ab2170 - view on LGTM.com

new alerts:

2 for Cast from abstract to concrete collection

lgtm-com · 2022-05-19T18:41:43Z

This pull request introduces 2 alerts when merging ca68d16 into 90531fd - view on LGTM.com

new alerts:

2 for Cast from abstract to concrete collection

clintropolis

overall lgtm 🤘

clintropolis · 2022-05-19T20:44:41Z

processing/src/main/java/org/apache/druid/segment/column/LexicographicalRangeIndex.java

   */
  BitmapColumnIndex forRange(
      @Nullable String startValue,
      boolean startStrict,
      @Nullable String endValue,
      boolean endStrict,
-      Predicate<String> matcher
+      @Nullable Predicate<String> matcher


What do you think about instead of making this @Nullable to remove the default implementation of the version of forRange which does not take a predicate matcher? I think that is actually the code path your benchmarks were measuring due to how you changed LikeFilter, since DictionaryEncodedStringIndexSupplier actually does implement that other version.

The reason it had a default implementation is a left-over artifact of #12315 where all of the methods had to be implemented on BitmapIndex, meaning a lot of silly implementations were required for things that didn't support those kinds of indexes, so the default implementation was a laziness on my part.

I think the implementation of the version that doesn't have the matcher should be slightly better than the @Nullable matcher since it can avoid the checks. Though its probably small fries when computing indexes, everything counts.

I think for ListFilteredVirtualColumn it would be ok to just use the old 'default' implementation for its version, since its the only other thing that implements this index interface.

Alternatively, we could remove the version that doesn't take a matcher argument and make this version with @Nullable the only method, though I think that's slightly less cool because having it split (and not having a default) sort of pushes implementors towards providing the more optimal predicate-less implementation. If we do go this route, we should change DictionaryEncodedStringIndexSupplier to make the version without ranges private and just use that when the matcher is null.

This confusion was my fault I think, this PR made it more obvious that the current state isn't the most intuitive on what to call or implement.

What do you think about instead of making this @nullable to remove the default implementation of the version of forRange which does not take a predicate matcher?

☝️ I did this one

clintropolis · 2022-05-19T20:50:21Z

...ssing/src/main/java/org/apache/druid/segment/serde/DictionaryEncodedStringIndexSupplier.java

-    if (clazz.equals(StringValueSetIndex.class)) {
-      return (T) new GenericIndexedDictionaryEncodedStringValueSetIndex(bitmapFactory, dictionary, bitmaps);
-    } else if (clazz.equals(DruidPredicateIndex.class)) {
+    if (bitmaps != null && clazz.equals(StringValueSetIndex.class)) {


good catch 👍

clintropolis · 2022-05-19T20:51:32Z

...ssing/src/main/java/org/apache/druid/segment/serde/DictionaryEncodedStringIndexSupplier.java

        GenericIndexed<ImmutableBitmap> bitmaps
    )
    {
      this.bitmapFactory = bitmapFactory;
-      this.dictionary = dictionary;
-      this.bitmaps = bitmaps;
+      this.dictionary = dictionary.singleThreaded();


is a shame that this method loses the more specific type, but is still probably better

clintropolis

🤘

lgtm-com · 2022-05-19T22:38:17Z

This pull request introduces 2 alerts when merging d61bb23 into 90531fd - view on LGTM.com

new alerts:

2 for Cast from abstract to concrete collection

gianm added Performance Area - Querying labels May 13, 2022

gianm added 2 commits May 13, 2022 07:03

Add like-filter implementation tests.

9bd6089

Add in-filter implementation tests.

301057b

Add tests, fix issues.

c1a2d94

Fix style.

f7ea7e5

Merge branch 'master' into perf-in-filters

ca68d16

clintropolis reviewed May 19, 2022

View reviewed changes

Adjustments from review.

d61bb23

clintropolis approved these changes May 19, 2022

View reviewed changes

clintropolis merged commit 69aac6c into apache:master May 20, 2022

gianm deleted the perf-in-filters branch May 20, 2022 13:30

abhishekagarwal87 added this to the 24.0.0 milestone Aug 26, 2022

techdocsmith mentioned this pull request Aug 26, 2022

[Draft] 24.0 Release notes #12825

Closed

abhishekagarwal87 mentioned this pull request Sep 8, 2022

Test issue [Please ignore] #13055

Closed

clintropolis mentioned this pull request Sep 27, 2022

add support for 'front coded' string dictionaries for smaller string columns #12277

Merged

6 tasks

gianm mentioned this pull request May 10, 2023

Updates to filter processing for inner query in Joins #14237

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Direct UTF-8 access for "in" filters. #12517

Direct UTF-8 access for "in" filters. #12517

gianm commented May 13, 2022

lgtm-com bot commented May 13, 2022

lgtm-com bot commented May 13, 2022

gianm commented May 14, 2022

lgtm-com bot commented May 14, 2022

lgtm-com bot commented May 14, 2022

lgtm-com bot commented May 19, 2022

clintropolis left a comment

clintropolis May 19, 2022

gianm May 19, 2022

clintropolis May 19, 2022

clintropolis May 19, 2022

clintropolis left a comment

lgtm-com bot commented May 19, 2022

Direct UTF-8 access for "in" filters. #12517

Direct UTF-8 access for "in" filters. #12517

Conversation

gianm commented May 13, 2022

lgtm-com bot commented May 13, 2022

lgtm-com bot commented May 13, 2022

gianm commented May 14, 2022

lgtm-com bot commented May 14, 2022

lgtm-com bot commented May 14, 2022

lgtm-com bot commented May 19, 2022

clintropolis left a comment

Choose a reason for hiding this comment

clintropolis May 19, 2022

Choose a reason for hiding this comment

gianm May 19, 2022

Choose a reason for hiding this comment

clintropolis May 19, 2022

Choose a reason for hiding this comment

clintropolis May 19, 2022

Choose a reason for hiding this comment

clintropolis left a comment

Choose a reason for hiding this comment

lgtm-com bot commented May 19, 2022