Skip to content

Commit c096b2a

Browse files
committed
aggregation/terms: charge fused term_counts to the memory limit
term_counts (one u32/term) was allocated but not charged to AggregationLimitsGuard, so a memory limit could be exceeded silently. Charge it, skip allocating it when unbounded, and add a regression test.
1 parent ac7a3d3 commit c096b2a

1 file changed

Lines changed: 73 additions & 17 deletions

File tree

src/aggregation/bucket/term_agg/term_histogram.rs

Lines changed: 73 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,8 @@ const MAX_FUSED_GRID_BUCKETS: usize = 16384;
4242
#[derive(Debug)]
4343
pub(crate) struct SegmentTermHistogramCollector {
4444
/// Per-term count of docs *outside* `hard_bounds` (still in `doc_count`, but in no bucket).
45-
/// Per-term total = this + the term's `counts` row-sum; all-zero when there are no bounds.
45+
/// Per-term total = this + the term's `counts` row-sum; left empty when there are no hard
46+
/// bounds (every doc is in-bounds, so there's no remainder to track).
4647
term_counts: Vec<u32>,
4748
/// Flattened `[num_terms * num_time_buckets]` histogram counters (`u32`, see
4849
/// `term_counts`).
@@ -84,17 +85,16 @@ impl SegmentAggregationCollector for SegmentTermHistogramCollector {
8485
// Expand the flat grid back into the regular structures and reuse the shared builders, so
8586
// ordering/cut-off/dict handling and cross-segment merging match the general path exactly.
8687
let mut bucket_id_provider = BucketIdProvider::default();
87-
// Per-term total = histogram row-sum (in-bounds) + `term_counts` (out-of-bounds remainder).
88+
// Per-term total = histogram row-sum (in-bounds) + `term_counts` (out-of-bounds remainder,
89+
// empty when there are no hard bounds).
8890
let term_buckets = VecTermBuckets {
8991
buckets: self
90-
.term_counts
91-
.iter()
92+
.counts
93+
.chunks_exact(self.num_time_buckets)
9294
.enumerate()
93-
.map(|(term_id, &out_of_bounds)| {
94-
let in_bounds: u32 = self.counts
95-
[term_id * self.num_time_buckets..(term_id + 1) * self.num_time_buckets]
96-
.iter()
97-
.sum();
95+
.map(|(term_id, row)| {
96+
let in_bounds: u32 = row.iter().sum();
97+
let out_of_bounds = self.term_counts.get(term_id).copied().unwrap_or(0);
9898
Bucket {
9999
count: in_bounds + out_of_bounds,
100100
bucket_id: bucket_id_provider.next_bucket_id(),
@@ -258,17 +258,23 @@ pub(super) fn maybe_build_collector(
258258
return Ok(None);
259259
}
260260

261-
let counts = vec![0u32; num_terms * range.len];
262-
agg_data
263-
.context
264-
.limits
265-
.add_memory_consumed((counts.len() * std::mem::size_of::<u32>()) as u64)?;
266261
// No hard bounds means every doc is in-bounds, letting `collect` short-circuit the bounds
267-
// check.
262+
// check — and leaving `term_counts` (the out-of-bounds remainder) unused, so we skip allocating
263+
// it.
268264
let all_docs_in_bounds =
269265
hist_req_data.bounds.min == f64::MIN && hist_req_data.bounds.max == f64::MAX;
266+
let counts = vec![0u32; num_terms * range.len];
267+
let term_counts = if all_docs_in_bounds {
268+
Vec::new()
269+
} else {
270+
vec![0u32; num_terms]
271+
};
272+
// Charge both grids to the aggregation memory limit.
273+
agg_data.context.limits.add_memory_consumed(
274+
((counts.len() + term_counts.len()) * std::mem::size_of::<u32>()) as u64,
275+
)?;
270276
Ok(Some(Box::new(SegmentTermHistogramCollector {
271-
term_counts: vec![0u32; num_terms],
277+
term_counts,
272278
counts,
273279
num_time_buckets: range.len,
274280
base_pos: range.base_pos,
@@ -284,7 +290,11 @@ pub(super) fn maybe_build_collector(
284290
#[cfg(test)]
285291
mod tests {
286292
use crate::aggregation::agg_req::Aggregations;
287-
use crate::aggregation::tests::{exec_request, get_test_index_from_values_and_terms};
293+
use crate::aggregation::tests::{
294+
exec_request, exec_request_with_query_and_memory_limit,
295+
get_test_index_from_values_and_terms,
296+
};
297+
use crate::aggregation::AggregationLimitsGuard;
288298

289299
/// Hand-computed correctness check for the fused terms×histogram fast path
290300
/// ([`super::SegmentTermHistogramCollector`]): low-cardinality terms × a histogram leaf over
@@ -526,4 +536,50 @@ mod tests {
526536

527537
Ok(())
528538
}
539+
540+
/// Regression: with hard bounds the fused path allocates `term_counts` (one `u32`/term) on top
541+
/// of the grid, and that allocation must be charged to the memory limit. With many terms and a
542+
/// single time bucket the two are equal in size, so a limit admitting the grid alone but not
543+
/// grid + `term_counts` must fail.
544+
#[test]
545+
fn fused_term_histogram_hard_bounds_charges_term_counts() -> crate::Result<()> {
546+
// 16k distinct terms, one doc each; values alternate in/out of the single-bucket bounds
547+
// [5, 5] so the bounds bind and `term_counts` is allocated. num_terms=16000,
548+
// num_time_buckets=1 => `counts` and `term_counts` are ~64 KB each.
549+
let docs: Vec<(f64, String)> = (0..16_000u64)
550+
.map(|i| (if i % 2 == 0 { 5.0 } else { 10.0 }, format!("t{i:05}")))
551+
.collect();
552+
let index = get_test_index_from_values_and_terms(true, &[docs])?;
553+
554+
let agg_req: Aggregations = serde_json::from_value(serde_json::json!({
555+
"by_term": {
556+
"terms": { "field": "string_id" },
557+
"aggs": {
558+
"histo": {
559+
"histogram": {
560+
"field": "score_f64",
561+
"interval": 1.0,
562+
"hard_bounds": { "min": 5.0, "max": 5.0 }
563+
}
564+
}
565+
}
566+
}
567+
}))
568+
.unwrap();
569+
570+
// ~96 KB admits the grid (~64 KB) but not grid + `term_counts` (~128 KB).
571+
let err = exec_request_with_query_and_memory_limit(
572+
agg_req,
573+
&index,
574+
None,
575+
AggregationLimitsGuard::new(Some(96_000), None),
576+
)
577+
.unwrap_err();
578+
assert!(
579+
err.to_string().contains("memory limit was exceeded"),
580+
"expected a memory-limit error, got: {err}"
581+
);
582+
583+
Ok(())
584+
}
529585
}

0 commit comments

Comments
 (0)