Introduce IdSet and add IdSetAggregationFunction #5926
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kishoreg
reviewed
Aug 26, 2020
| private final Type _type; | ||
| private final RoaringBitmap _bitmap; | ||
| private final Roaring64NavigableMap _longBitmap; | ||
|
|
There was a problem hiding this comment.
Do you think this constructor is the only thing we need? If not, We should make this private and add static builders
| /** | ||
| * The {@code IdSet} represents a collection of ids. It can be used to optimize the query with huge IN clause. | ||
| */ | ||
| public class IdSet implements Comparable<IdSet> { |
There was a problem hiding this comment.
Let’s make this an interface with multiple implementations?
Jackie-Jiang
force-pushed
the
id_set
branch
2 times, most recently
from
456de34 to
fab464f
Compare
|
@kishoreg Refactored the PR and introduced the |
Codecov Report
@@ Coverage Diff @@
## master #5926 +/- ##
==========================================
+ Coverage 66.44% 67.35% +0.90%
==========================================
Files 1075 1191 +116
Lines 54773 62438 +7665
Branches 8168 9533 +1365
==========================================
+ Hits 36396 42054 +5658
- Misses 15700 17286 +1586
- Partials 2677 3098 +421
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
kishoreg
approved these changes
Aug 28, 2020
...src/main/java/org/apache/pinot/core/query/aggregation/function/IdSetAggregationFunction.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/apache/pinot/core/query/aggregation/function/IdSetAggregationFunction.java
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
For issue #5925
Introduce
IdSetwhich represents a collection of ids and can be used to replace the large IN clause to optimize the query.4 types of
IdSetare introduced:EmptyIdSet: Used as a place holder for empty id setRoaringBitmapIdSet: Based onRoaringBitmapand can be used to store INT idsRoaring64NavigableMapIdSet: Based onRoaring64NavigableMapand can be used to store LONG idsBloomFilterIdSet: Based onBloomFilterand can be used to store any type of ids (contains won't be 100% accurate because of the nature ofBloomFilter)Add
IdSetAggregationFunctionto createIdSetfor a column.3 configurable parameters for the function:
sizeThresholdInBytes: Once the size of theIdSetreaches this threshold, convert it toBloomFilterIdSetto save spaceexpectedInsertions: Number of expected insertions for the BloomFilter, must be positivefpp: Desired false positive probability for the BloomFilter, must be positive and less than 1.0The parameters can be passed to the function as the second argument, e.g.:
SELECT IDSET(intColumn, 'sizeThresholdInBytes=1000;expectedInsertions=1000;fpp=0.03') FROM testTableBump up the version of
RoaringBitmapto0.9.0to include some bug fixes forRoaring64NavigableMap.