fix: estimate size for topn cache extract_if #14564
Conversation
| pub fn extract_if<F>(&mut self, pred: F) -> ExtractIf<'_, CacheKey, CompactedRow, F, Global> | ||
| pub fn extract_if<'a, F1>( | ||
| &'a mut self, | ||
| mut pred: F1, |
There was a problem hiding this comment.
Why mut here? And no need to change to F1
There was a problem hiding this comment.
Just learn form @BugenZhao, FnMut::call_mut accept &mut self. Calling pred will mutate itself.
Added a issue link. |
| @@ -108,18 +108,25 @@ impl TopNCacheState { | |||
| self.inner.range(range) | |||
| } | |||
|
|
|||
| pub fn extract_if<F>(&mut self, pred: F) -> ExtractIf<'_, CacheKey, CompactedRow, F, Global> | |||
| pub fn extract_if<'a, F1>( | |||
There was a problem hiding this comment.
This change makes me think:
- Are there any other places that the size is not correctly maintained?
- Why this
kv_heap_sizestuff is inTopNCacheState? It looks like a generalBTreeMapWithEstimateSizeor sth.
There was a problem hiding this comment.
There are some special usage of an estimated structure. For example https://github.com/singularity-data/risingwave/blob/9cd7f6486e659af72857aef7ac27247305e880c0/src/stream/src/executor/managed_state/join/join_entry_state.rs#L75. It can be hard to provide an EstimatedBtreeMap with an unified semantic.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
add estimate size for topn cache extract_if
close #14530
Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.