Repository navigation
Account for TopK common-prefix converter memory - #26286
Conversation
- Counts optional prefix converter exactly once
… reservation equals size() - Updated assertions in mod.rs to verify batch insertion and reservation size.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #26286 +/- ##
==========================================
+ Coverage 82.80% 82.84% +0.04%
==========================================
Files 1149 1151 +2
Lines 454146 455546 +1400
Branches 454146 455546 +1400
==========================================
+ Hits 376035 377382 +1347
+ Misses 55260 55246 -14
- Partials 22851 22918 +67 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
nuno-faria
left a comment
There was a problem hiding this comment.
Thanks @kosiew, LGTM.
| with_prefix.insert_batch(make_ab_batch( | ||
| Arc::clone(&schema), | ||
| &[Some(1), Some(2)], | ||
| &[1.0, 2.0], | ||
| )?)?; | ||
| assert_eq!(with_prefix.reservation.size(), with_prefix.size()); |
There was a problem hiding this comment.
Is this part necessary for this test?
There was a problem hiding this comment.
It’s not necessary for this regression. I added it to check memory-pool charging, but insert_batch() already resizes the reservation using self.size(), so this equality also passes with the converter omitted.
The preceding size-delta assertion catches the bug. I’ll remove the insertion and reservation assertion.
- Removed insertion and reservation assertion. - Removed unused mut variable.
|
🚀 |
Which issue does this PR close?
sizefunctions #23393Rationale for this change
TopKunderreports retained memory when a common sort prefix is present because its memory accounting omits the optionalcommon_sort_prefix_converter. This can cause memory reservations to underestimate TopK's retained memory, weakening memory-limit enforcement.What changes are included in this PR?
common_sort_prefix_converter.size()exactly once inTopK::size()when the converter is present.Are these changes tested?
Added
test_topk_size_common_sort_prefix_converter, which verifies:TopK::size()retains its existing formula.insert_batch(), the memory reservation matchesTopK::size().The test does not rely on global allocator measurements.
Are there any user-facing changes?
No changes to query results or public APIs. The change corrects internal memory accounting for TopK with a common sort prefix, potentially improving memory-limit enforcement.
LLM-generated code disclosure
This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.