Skip to content

fix(aggs): the columnar histogram paths honour config.limits.max_buckets - #125

Merged
xerj-org merged 1 commit into
mainfrom
fix/issue-121-v2
Aug 2, 2026
Merged

fix(aggs): the columnar histogram paths honour config.limits.max_buckets#125
xerj-org merged 1 commit into
mainfrom
fix/issue-121-v2

Conversation

@xerj-org

@xerj-org xerj-org commented Aug 2, 2026

Copy link
Copy Markdown
Owner

Partially closes #121. See the bottom for what is deliberately left open.

fast_aggs::exec_date_histogram and exec_histogram each carried their own const MAX_BUCKETS: i64 = 65_536 and never consulted aggs::max_buckets(). An operator lowering config.limits.max_buckets to protect memory was therefore protected only on queries that happened to miss the columnar path.

Measured at cap 37 on a 12,000-doc index that provably took the fast path: 38 buckets returned, no error. The same shape on a 600-doc index (below FAST_AGG_MIN_DOCS, so brute) correctly refused.

What changed

Both read a shared aggs::max_buckets_i64(), hoisted once above the gap-fill loop so no per-bucket lookup is added, and the too_many_buckets message names the cap actually enforced.

The brute run_date_histogram / run_histogram hardcoded the same constant, so fixing only the fast path would have made the two disagree the instant a cap moved. They read the same accessor now, which is also what carries #100's per-call AggLimits override into both.

A change in both directions

Raising the cap now raises the histogram ceiling too, which the hardcoded constant previously prevented. An operator setting max_buckets = 200_000 gets 200,000-bucket gap fills on both paths. That is the correct reading of the setting and matches ES and the already-config-driven terms path, but the effect is not only downward and is worth knowing before raising it.

Deliberately NOT closed here

exec_terms still has no bucket cap of any kind — only a size-derived output cap. Measured at max_buckets = 37 over 200 distinct terms: the 12,000-doc index (fast path) returned top5 = 300 plus sum_other_doc_count = 11,700, i.e. all 200 terms materialised; the 600-doc index of identical shape (brute) returned exactly 37 terms' worth.

That is the OOM vector the cap comment cites, and it is still open on the columnar path. It is not folded in here because it needs a different fix rather than a wider one: the brute cap is order-dependent and reports a sum_other_doc_count that conceals the terms it silently dropped, so mirroring brute would propagate that bug into the fast path instead of fixing anything. #121 stays open for it.

Relatedly, Engine::new's process-wide set_max_buckets is unchanged and still latent, per the issue's own second bullet.

Verification

Rebased onto current main by applying only the #121 diff (the branch carried #118's two unsquashed commits while main has it squashed, so a plain merge conflicted). cargo test -p xerj-engine --lib329 passed, 0 failed, plus the new agg_bucket_cap integration test. cargo fmt --check and cargo clippy -D warnings clean.

Two of the three tests are load-bearing, confirmed by revert: reverting fast_aggs alone fails agg_bucket_cap.rs:150; reverting aggs.rs alone fails agg_bucket_cap.rs:186 and date_histogram_gap_fill_honours_the_per_call_cap. The third (max_buckets_i64_carries_the_per_call_override) is a unit test of the new accessor and passes under both reverts — it is coverage, not a regression detector, and is not claimed as one.

Also corrects two comments that did not describe their code: a test comment that said "one doc per hour, 5 hours apart" for a fixture that is two docs at 00:00 and 04:00, and a doc claiming the two executors "are designed to return byte-identical results" as though that were already true — #99, #104 and #114 each closed a divergence, and exec_terms is another.

`fast_aggs::exec_date_histogram` and `exec_histogram` each carried their own
`const MAX_BUCKETS: i64 = 65_536` and never consulted `aggs::max_buckets()`, so
an operator who lowered `config.limits.max_buckets` to protect memory was
protected only on queries that happened to miss the columnar path. Measured at
cap 37 on a 12,000-doc index: the fast path returned 38 buckets with no error.

Both now read a shared `aggs::max_buckets_i64()`, hoisted once above the
gap-fill loop so no per-bucket lookup is added, and the `too_many_buckets`
message names the cap actually enforced. The brute `run_date_histogram` /
`run_histogram` hardcoded the same constant, so fixing only the fast path would
have made the two disagree the moment a cap moved; they read the same accessor
now, which is also what carries #100's per-call `AggLimits` override into both.

Raising the cap now raises the histogram ceiling too, which the previous
hardcoded constant prevented. That is the correct reading of the setting and
matches both ES and the already-config-driven terms path, but it is a change in
both directions rather than only downward.

NOT CLOSED HERE, and the comment on `FAST_PATH_AGGS_SERVED` now says so:
`exec_terms` builds its term map with no bucket cap of any kind, only a
`size`-derived output cap. Measured at cap 37 over 200 distinct terms, the fast
path materialised all 200 while the brute path stopped at 37. That is the OOM
vector the cap exists for, and it needs its own change: the brute cap is
order-dependent and reports a `sum_other_doc_count` that hides the terms it
dropped, so mirroring it would propagate that rather than fix anything.
@xerj-org
xerj-org merged commit ea7ce28 into main Aug 2, 2026
9 checks passed
@xerj-org
xerj-org deleted the fix/issue-121-v2 branch August 2, 2026 04:40
xerj-org added a commit that referenced this pull request Aug 2, 2026
…121)

`fast_aggs::exec_terms` built its term map with no bucket cap of any kind,
only a `size`-derived OUTPUT cap. An operator who lowered
`config.limits.max_buckets` to protect memory got that protection only on
queries that missed the columnar path: measured at max_buckets=37 over 200
distinct terms, the fast path (a 12k-doc index) materialised all 200
(top5=300 + sum_other_doc_count=11,700) while the brute `run_terms` stopped
at 37. That is the OOM vector the cap exists to close — the half PR #125
left open after fixing the histogram executors.

The two executors must AGREE past the cap. Brute `run_terms` does not error
there; it keeps the first `max_buckets` distinct terms in doc-iteration
order, drops the rest, and reports a `sum_other_doc_count` computed only
over the terms it kept, so the dropped terms vanish silently and the total
conceals them (order-dependent). Mirroring that truncation onto the fast
path would copy the bug, not the contract; erroring would make the fast
path disagree with a brute path that still returns a body.

So `exec_terms` BAILS to brute the moment its distinct-term count would
exceed the cap, checked as the map grows (per segment and after the
memtable) so the map is bounded to ~`max_buckets`. After the bail the
request is answered by the very `run_terms` a test measures the fast path
against, so the two AGREE past the cap by construction — the same shape as
the #104 / #120 bail-to-brute fallbacks. An index whose distinct count is
at or under the cap is served columnarly as before, and the default-cap
(65,536) common case is untouched.

Test (its own binary, one function, like agg_bucket_cap.rs, because the cap
lives in a process-wide static): a 10,050-doc index over 200 terms at cap 37
is no longer served columnarly (`fast_path_aggs_served` does not tick) and
honours the cap (37 buckets, not 200); a 600-doc brute index of the same
shape agrees on the count; and an at-cap index (exactly 37 terms) is still
served columnarly, guarding against over-bailing. Reverting only
fast_aggs.rs fails it: served==1, 200 buckets against max_buckets=37.
xerj-org added a commit that referenced this pull request Aug 2, 2026
…brute (#134)

* fix(aggs): the columnar terms path honours config.limits.max_buckets (#121)

`fast_aggs::exec_terms` built its term map with no bucket cap of any kind,
only a `size`-derived OUTPUT cap. An operator who lowered
`config.limits.max_buckets` to protect memory got that protection only on
queries that missed the columnar path: measured at max_buckets=37 over 200
distinct terms, the fast path (a 12k-doc index) materialised all 200
(top5=300 + sum_other_doc_count=11,700) while the brute `run_terms` stopped
at 37. That is the OOM vector the cap exists to close — the half PR #125
left open after fixing the histogram executors.

The two executors must AGREE past the cap. Brute `run_terms` does not error
there; it keeps the first `max_buckets` distinct terms in doc-iteration
order, drops the rest, and reports a `sum_other_doc_count` computed only
over the terms it kept, so the dropped terms vanish silently and the total
conceals them (order-dependent). Mirroring that truncation onto the fast
path would copy the bug, not the contract; erroring would make the fast
path disagree with a brute path that still returns a body.

So `exec_terms` BAILS to brute the moment its distinct-term count would
exceed the cap, checked as the map grows (per segment and after the
memtable) so the map is bounded to ~`max_buckets`. After the bail the
request is answered by the very `run_terms` a test measures the fast path
against, so the two AGREE past the cap by construction — the same shape as
the #104 / #120 bail-to-brute fallbacks. An index whose distinct count is
at or under the cap is served columnarly as before, and the default-cap
(65,536) common case is untouched.

Test (its own binary, one function, like agg_bucket_cap.rs, because the cap
lives in a process-wide static): a 10,050-doc index over 200 terms at cap 37
is no longer served columnarly (`fast_path_aggs_served` does not tick) and
honours the cap (37 buckets, not 200); a 600-doc brute index of the same
shape agrees on the count; and an at-cap index (exactly 37 terms) is still
served columnarly, guarding against over-bailing. Reverting only
fast_aggs.rs fails it: served==1, 200 buckets against max_buckets=37.

* doc(aggs): drop the now-false exec_terms 'no bucket cap' note

This PR gives exec_terms a bucket cap, so the FAST_PATH_AGGS_SERVED comment
that cited it as a still-uncapped divergence is now false. Replaced with the
current list (#99/#104/#114/#120 closed, #128 nested-object still open).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fast_aggs ignores config.limits.max_buckets entirely

1 participant