-
Notifications
You must be signed in to change notification settings - Fork 298
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
feat: add a cache for last value result in row group #4369
Conversation
WalkthroughThe changes implement a caching mechanism for time series selector results within 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 (
|
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.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (5)
src/store-api/src/storage/requests.rs (2)
20-20
: Add documentation forLastRow
variant.The
LastRow
variant in theTimeSeriesRowSelector
enum lacks documentation. Adding a brief comment explaining its purpose would improve code readability./// Only keep the last row of each time-series. LastRow,
Line range hint
33-33
: Add documentation forseries_row_selector
field.The
series_row_selector
field in theScanRequest
struct lacks documentation. Adding a brief comment explaining its purpose would improve code readability./// Optional hint to select rows from time-series. pub series_row_selector: Option<TimeSeriesRowSelector>,src/mito2/src/config.rs (3)
87-88
: Add documentation forselector_result_cache_size
field.The
selector_result_cache_size
field in theMitoConfig
struct lacks documentation. Adding a brief comment explaining its purpose would improve code readability./// Cache size for time series selector (e.g. `last_value()`). Setting it to 0 to disable the cache. pub selector_result_cache_size: ReadableSize,
138-138
: Add initialization forselector_result_cache_size
inDefault
implementation.The
selector_result_cache_size
field should be initialized in theDefault
implementation ofMitoConfig
.selector_result_cache_size: ReadableSize::mb(512),
243-243
: Add adjustment forselector_result_cache_size
inadjust_buffer_and_cache_size
method.The
selector_result_cache_size
field should be adjusted according to system memory in theadjust_buffer_and_cache_size
method.self.selector_result_cache_size = mem_cache_size;
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- src/mito2/src/cache.rs (10 hunks)
- src/mito2/src/config.rs (3 hunks)
- src/mito2/src/read.rs (1 hunks)
- src/mito2/src/worker.rs (2 hunks)
- src/store-api/src/storage/requests.rs (1 hunks)
- tests-integration/tests/http.rs (2 hunks)
Files skipped from review due to trivial changes (2)
- src/mito2/src/read.rs
- tests-integration/tests/http.rs
Additional comments not posted (16)
src/mito2/src/cache.rs (14)
33-33
: Add import forTimeSeriesRowSelector
.The
TimeSeriesRowSelector
is imported to be used in the cache mechanism.
52-52
: Add constant for selector result cache type.The
SELECTOR_RESULT_TYPE
constant is added for metrics labeling.
69-70
: Add field for selector result cache inCacheManager
.The
selector_result_cache
field is added to theCacheManager
struct.
175-186
: Add method to get selector result from cache.The
get_selector_result
method is added to retrieve cached selector results.
188-200
: Add method to put selector result into cache.The
put_selector_result
method is added to store selector results in the cache.
221-221
: Add field for selector result cache size inCacheManagerBuilder
.The
selector_result_cache_size
field is added to theCacheManagerBuilder
struct.
261-266
: Add method to set selector result cache size inCacheManagerBuilder
.The
selector_result_cache_size
method is added to configure the selector result cache size.
305-316
: Add initialization for selector result cache inCacheManagerBuilder
.The
selector_result_cache
is initialized in theCacheManagerBuilder
'sbuild
method.
323-323
: Add field for selector result cache inCacheManager
.The
selector_result_cache
is added to theCacheManager
struct.
342-344
: Add weigher function for selector result cache.The
selector_result_cache_weight
function is added to compute the weight of cache entries.
406-415
: AddSelectorResultKey
struct.The
SelectorResultKey
struct is added to represent the cache key for selector results.
417-434
: AddSelectorResultValue
struct.The
SelectorResultValue
struct is added to represent the cached selector results.
444-445
: Add type alias forSelectorResultCache
.The
SelectorResultCache
type alias is added for the cache of selector results.
546-561
: Add test for selector result cache.A test is added to verify the functionality of the selector result cache.
src/mito2/src/worker.rs (2)
161-161
: Add selector result cache size configuration inWorkerGroup
.The
selector_result_cache_size
configuration is used to initialize the cache manager in theWorkerGroup
.
296-296
: Add selector result cache size configuration in testWorkerGroup
.The
selector_result_cache_size
configuration is used to initialize the cache manager in the testWorkerGroup
.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4369 +/- ##
==========================================
- Coverage 85.24% 84.96% -0.28%
==========================================
Files 1068 1068
Lines 191277 191356 +79
==========================================
- Hits 163046 162580 -466
- Misses 28231 28776 +545 |
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.
LGTM
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.
LGTM
@@ -133,6 +135,7 @@ impl Default for MitoConfig { | |||
sst_meta_cache_size: ReadableSize::mb(128), | |||
vector_cache_size: ReadableSize::mb(512), | |||
page_cache_size: ReadableSize::mb(512), | |||
selector_result_cache_size: ReadableSize::mb(512), |
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.
Forget to update the config.toml?
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 a new cache
selector_result_cache
forTimeSeriesRowSelector
results. Now it caches thelast_value()
result for parquet row groups.Checklist
Summary by CodeRabbit
New Features
selector_result_cache_size
to control cache behavior.Improvements
CacheManager
with methods to manage selector result caching.MitoConfig
to initialize and assign selector result cache size settings.Tests