feat(inverted_index): inverted index cache #4309
Conversation
Update dependencies and add caching for inverted index reader - Updated `atomic` to 0.6.0 and `uuid` to 1.9.1 in `Cargo.lock`. - Added `moka` and `uuid` dependencies in `Cargo.toml`. - Introduced `seek_read` method in `InvertedIndexBlobReader` for common seek and read operations. - Added `cache.rs` module to implement caching for inverted index reader using `moka`. - Updated `async-compression` to 0.4.11 in `puffin/Cargo.toml`.
Refactor InvertedIndexReader and Add Index Cache Support - Refactored `InvertedIndexReader` to include `seek_read` method and default implementations for `fst` and `bitmap`. - Implemented `seek_read` in `InvertedIndexBlobReader` and `CachedInvertedIndexBlobReader`. - Introduced `InvertedIndexCache` in `CacheManager` and `SstIndexApplier`. - Updated `SstIndexApplierBuilder` to accept and utilize `InvertedIndexCache`. - Added `From<FileId> for Uuid` implementation.
Update Cargo.toml and refactor SstIndexApplier - Moved `uuid.workspace` entry in Cargo.toml for better organization.
|
Important Review skippedReview was skipped due to path filters Files ignored due to path filters (1)
You can disable this status message by setting the WalkthroughThe updates include the addition and enhancement of methods to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Refactor InvertedIndexCache to use type alias for Arc - Replaced `Arc<InvertedIndexCache>` with `InvertedIndexCacheRef` type alias.
Add Prometheus metrics and caching improvements for inverted index - Introduced `prometheus` and `puffin` dependencies for metrics.
Refactor InvertedIndexReader and Cache handling - Simplified `InvertedIndexReader` trait by removing seek-related comments.
Add configurable cache sizes for inverted index metadata and content - Introduced `index_metadata_size` and `index_content_size` in `CacheManagerBuilder`.
Refactor and optimize inverted index caching - Removed `metrics.rs` and integrated cache metrics into `index.rs`.
Remove unused dependencies from Cargo.lock and Cargo.toml - Removed `moka`, `prometheus`, and `puffin` dependencies from both Cargo.lock and Cargo.toml.
v0y4g3r
force-pushed
the
feat/inverted-index-cache
branch
from
5abcaef
to
611109a
Compare
v0y4g3r
requested review from
evenyag,
waynexia,
zhongzc and
a team
as code owners
|
Do we have any benchmark program and results for this PR? @v0y4g3r |
I updated the PR description. This PR improves those queries which are already fast, but not fast enough, namely those 5~6ms queries. An improvement of 2 to 3ms means cutting the total cost time by half. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
Files selected for processing (19)
- src/index/Cargo.toml (1 hunks)
- src/index/src/inverted_index/format/reader.rs (1 hunks)
- src/index/src/inverted_index/format/reader/blob.rs (2 hunks)
- src/index/src/inverted_index/search/index_apply/predicates_apply.rs (4 hunks)
- src/mito2/src/cache.rs (7 hunks)
- src/mito2/src/cache/index.rs (1 hunks)
- src/mito2/src/config.rs (2 hunks)
- src/mito2/src/read/scan_region.rs (1 hunks)
- src/mito2/src/sst/file.rs (1 hunks)
- src/mito2/src/sst/index/inverted_index/applier.rs (7 hunks)
- src/mito2/src/sst/index/inverted_index/applier/builder.rs (6 hunks)
- src/mito2/src/sst/index/inverted_index/applier/builder/between.rs (5 hunks)
- src/mito2/src/sst/index/inverted_index/applier/builder/comparison.rs (4 hunks)
- src/mito2/src/sst/index/inverted_index/applier/builder/eq_list.rs (7 hunks)
- src/mito2/src/sst/index/inverted_index/applier/builder/in_list.rs (5 hunks)
- src/mito2/src/sst/index/inverted_index/applier/builder/regex_match.rs (4 hunks)
- src/mito2/src/sst/index/inverted_index/creator.rs (2 hunks)
- src/mito2/src/worker.rs (1 hunks)
- tests-integration/tests/http.rs (1 hunks)
Files skipped from review due to trivial changes (5)
- src/index/Cargo.toml
- src/mito2/src/sst/index/inverted_index/applier/builder/between.rs
- src/mito2/src/sst/index/inverted_index/applier/builder/comparison.rs
- src/mito2/src/sst/index/inverted_index/applier/builder/eq_list.rs
- src/mito2/src/sst/index/inverted_index/applier/builder/in_list.rs
Additional comments not posted (46)
src/index/src/inverted_index/format/reader.rs (5)
34-34: LGTM!The
read_allmethod is correctly implemented, reading all data into the destination vector and returning the size of the data read.
37-37: LGTM!The
seek_readmethod is correctly implemented, seeking to the given offset and reading the specified size of data.
40-40: LGTM!The
metadatamethod correctly retrieves and returns the metadata wrapped in anArcfor shared ownership.
43-46: LGTM!The
fstmethod correctly retrieves the FST map by using theseek_readmethod and creating anFstMapfrom the retrieved data.
49-51: LGTM!The
bitmapmethod correctly retrieves the bitmap by using theseek_readmethod and creating aBitVecfrom the retrieved data.src/mito2/src/sst/index/inverted_index/applier/builder/regex_match.rs (5)
Line range hint
10-31:
LGTM!The
collect_regex_matchmethod correctly collects a regex match expression, performing necessary checks and conditions to ensure validity.
Line range hint
61-80:
LGTM!The
test_regex_match_basictest correctly verifies the basic functionality of thecollect_regex_matchmethod.
Line range hint
95-104:
LGTM!The
test_regex_match_field_columntest correctly verifies that thecollect_regex_matchmethod does not add a regex match predicate for field columns.
Line range hint
117-126:
LGTM!The
test_regex_match_type_mismatchtest correctly verifies that thecollect_regex_matchmethod does not add a regex match predicate when there is a type mismatch.
Line range hint
139-143:
LGTM!The
test_regex_match_type_nonexist_columntest correctly verifies that thecollect_regex_matchmethod returns an error when the column does not exist.src/mito2/src/cache/index.rs (12)
41-49: LGTM!The
CachedInvertedIndexBlobReaderconstructor correctly initializes the struct with the provided file ID, inner reader, and cache.
57-83: LGTM!The
get_or_loadmethod correctly retrieves index data from the cache or loads it from the source if not already cached, handling errors and edge cases appropriately.
89-94: LGTM!The
read_allmethod correctly delegates the read operation to the inner reader and returns the size of the data read.
96-102: LGTM!The
seek_readmethod correctly delegates the seek and read operations to the inner reader and returns the data read.
104-113: LGTM!The
metadatamethod correctly retrieves metadata from the cache or loads it from the source if not already cached, handling errors and edge cases appropriately.
116-124: LGTM!The
fstmethod correctly retrieves the FST map by using theget_or_loadmethod and creating anFstMapfrom the retrieved data.
149-177: LGTM!The
InvertedIndexCacheconstructor correctly initializes the cache with the provided metadata and content capacities, setting up eviction listeners to update metrics.
181-183: LGTM!The
get_index_metadatamethod correctly retrieves metadata from the cache for the specified file ID.
185-191: LGTM!The
put_index_metadatamethod correctly inserts metadata into the cache for the specified file ID and updates the metrics.
194-196: LGTM!The
get_indexmethod correctly retrieves index data from the cache for the specified key.
198-203: LGTM!The
put_indexmethod correctly inserts index data into the cache for the specified key and updates the metrics.
207-214: LGTM!The
index_metadata_weightandindex_content_weightmethods correctly calculate the weight of the metadata and content for caching purposes.src/index/src/inverted_index/format/reader/blob.rs (2)
53-58: LGTM!The
read_allmethod correctly seeks to the start and reads all data into the destination vector, handling errors appropriately.
61-69: LGTM!The
seek_readmethod correctly seeks to the given offset and reads the specified size of data into a buffer, handling errors appropriately.src/mito2/src/sst/file.rs (1)
68-72: LGTM!The implementation of the
Fromtrait forFileIdtoUuidconversion is correct.src/mito2/src/sst/index/inverted_index/applier.rs (4)
59-61: LGTM! Ensure proper utilization of the cache.The addition of the
inverted_index_cachefield and its inclusion in thenewmethod is correct.Ensure that the cache is properly utilized in the rest of the codebase.
Also applies to: 73-86
108-125: LGTM! Verify the correct usage ofCachedInvertedIndexBlobReader.The changes to use
CachedInvertedIndexBlobReaderifinverted_index_cacheis available are correct.Ensure that
CachedInvertedIndexBlobReaderis correctly implemented and used.
220-220: LGTM!The addition of the
index_cacheparameter in thetest_index_applier_apply_basicfunction is correct.
262-262: LGTM!The addition of the
index_cacheparameter in thetest_index_applier_apply_invalid_blob_typefunction is correct.src/mito2/src/sst/index/inverted_index/applier/builder.rs (2)
66-68: LGTM! Ensure proper utilization of the cache.The addition of the
index_cachefield and its inclusion in thenewmethod is correct.Ensure that the cache is properly utilized in the rest of the codebase.
Also applies to: 77-89
331-331: LGTM!The addition of the
index_cacheparameter in thetest_collect_and_basicfunction is correct.src/index/src/inverted_index/search/index_apply/predicates_apply.rs (2)
Line range hint
166-180:
LGTM!The update to return
Arc<InvertedIndexMetas>in themock_metasfunction is correct.
305-309: LGTM!The update to use
Arc<InvertedIndexMetas>in thetest_index_applier_with_empty_indexfunction is correct.src/mito2/src/cache.rs (4)
20-20: LGTM! Addition ofindexmodule.The addition of the
indexmodule aligns with the purpose of introducing caching mechanisms for inverted index.
64-65: LGTM! Addition ofindex_cachefield inCacheManager.The addition of the
index_cachefield is appropriate for managing the cache for inverted index.
175-177: LGTM! Addition ofindex_cachemethod inCacheManager.The method provides access to the
index_cachefield, which is essential for managing the cache for inverted index.
216-226: LGTM! Addition ofindex_metadata_sizeandindex_content_sizemethods inCacheManagerBuilder.These methods allow configuring the cache sizes for index metadata and content, which is crucial for managing the cache effectively.
src/mito2/src/config.rs (2)
87-90: LGTM! Addition ofinverted_index_metadata_cache_sizeandinverted_index_cache_sizefields inMitoConfig.These fields are essential for configuring the cache sizes for inverted index metadata and content, and their default values are set appropriately.
140-141: LGTM! Default values forinverted_index_metadata_cache_sizeandinverted_index_cache_sizeinMitoConfig::default.The default values of 32MB for both fields are reasonable and align with typical cache size configurations.
src/mito2/src/sst/index/inverted_index/creator.rs (1)
418-423: LGTM! Usage ofInvertedIndexCacheinSstIndexApplierBuilder.The integration of the
InvertedIndexCachein theSstIndexApplierBuilderis crucial for leveraging the caching mechanisms introduced in this PR.src/mito2/src/read/scan_region.rs (2)
333-338: LGTM!The
index_cachevariable is correctly initialized by safely accessing and cloning the value from thecache_manager.
343-343: LGTM!The
index_cachevariable is correctly passed to theSstIndexApplierBuilder::newmethod, enabling caching functionality.src/mito2/src/worker.rs (2)
161-161: Good addition: Index metadata size configuration.The addition of
index_metadata_sizeconfiguration enhances the cache management for index metadata.
162-162: Good addition: Index content size configuration.The addition of
index_content_sizeconfiguration enhances the cache management for index content.tests-integration/tests/http.rs (2)
823-823: Good addition: Inverted index metadata cache size configuration.The addition of
inverted_index_metadata_cache_sizeconfiguration in the config API test ensures the new feature is tested.
824-824: Good addition: Inverted index cache size configuration.The addition of
inverted_index_cache_sizeconfiguration in the config API test ensures the new feature is tested.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4309 +/- ##
==========================================
- Coverage 85.11% 84.85% -0.27%
==========================================
Files 1060 1061 +1
Lines 187648 187832 +184
==========================================
- Hits 159720 159387 -333
- Misses 27928 28445 +517 |
Replace Uuid with FileId in CachedInvertedIndexBlobReader - Updated `file_id` type from `Uuid` to `FileId` in `CachedInvertedIndexBlobReader` and related methods.
Refactor cache configuration for inverted index - Moved `inverted_index_metadata_cache_size` and `inverted_index_cache_size` from `MitoConfig` to `InvertedIndexConfig`.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- src/mito2/src/cache/index.rs (1 hunks)
- src/mito2/src/config.rs (2 hunks)
- src/mito2/src/sst/file.rs (1 hunks)
- src/mito2/src/worker.rs (1 hunks)
- tests-integration/tests/http.rs (1 hunks)
Files skipped from review as they are similar to previous changes (5)
- src/mito2/src/cache/index.rs
- src/mito2/src/config.rs
- src/mito2/src/sst/file.rs
- src/mito2/src/worker.rs
- tests-integration/tests/http.rs
Remove unnecessary conversion of `file_id` in `SstIndexApplier` - Simplified the initialization of `CachedInvertedIndexBlobReader` by removing the redundant `into()` conversion for `file_id`.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/mito2/src/sst/index/inverted_index/applier.rs (7 hunks)
Additional comments not posted (5)
src/mito2/src/sst/index/inverted_index/applier.rs (5)
60-61: Addition ofinverted_index_cachefield is appropriate.The
inverted_index_cachefield is correctly added to theSstIndexApplierstruct to hold an optional reference to an inverted index cache.
Line range hint
73-86: Changes to thenewmethod are appropriate.The
newmethod correctly accepts an additional parameterindex_cacheand assigns it to theinverted_index_cachefield.
109-125: Changes to theapplymethod are appropriate.The
applymethod correctly includes logic to useCachedInvertedIndexBlobReaderifinverted_index_cacheis available, enhancing the efficiency of the index application process.
220-220: Changes to thetest_index_applier_apply_basictest are appropriate.The test correctly passes
Nonefor theindex_cacheparameter to thenewmethod ofSstIndexApplier, aligning with the updated method signature.
262-262: Changes to thetest_index_applier_apply_invalid_blob_typetest are appropriate.The test correctly passes
Nonefor theindex_cacheparameter to thenewmethod ofSstIndexApplier, aligning with the updated method signature.
| metadata_cache_size = "32MiB" | ||
| content_cache_size = "32MiB" |
There was a problem hiding this comment.
Try to make their default value auto.
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
This PR adds im-memory cache for inverted index that boost most queries by reducing the pruning cost, which reduces total cost by 2~3ms:
Future work
PuffinReaderhas been refactored recently and I need to find a proper way to passInvertedIndexCachetoBoundedStager.Checklist
Summary by CodeRabbit
New Features
Improvements
SstIndexApplierto conditionally use the cache for index reading.Arc.Configuration Updates
Testing Enhancements