fix(aggs): the columnar histogram paths honour config.limits.max_buckets - #125
Merged
Conversation
`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
force-pushed
the
fix/issue-121-v2
branch
from
August 2, 2026 01:07
62c38dc to
deedef7
Compare
This was referenced Aug 2, 2026
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).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Partially closes #121. See the bottom for what is deliberately left open.
fast_aggs::exec_date_histogramandexec_histogrameach carried their ownconst MAX_BUCKETS: i64 = 65_536and never consultedaggs::max_buckets(). An operator loweringconfig.limits.max_bucketsto 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 thetoo_many_bucketsmessage names the cap actually enforced.The brute
run_date_histogram/run_histogramhardcoded 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-callAggLimitsoverride 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_000gets 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_termsstill has no bucket cap of any kind — only asize-derived output cap. Measured atmax_buckets = 37over 200 distinct terms: the 12,000-doc index (fast path) returnedtop5 = 300plussum_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_countthat 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-wideset_max_bucketsis unchanged and still latent, per the issue's own second bullet.Verification
Rebased onto current
mainby 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 --lib→ 329 passed, 0 failed, plus the newagg_bucket_capintegration test.cargo fmt --checkandcargo clippy -D warningsclean.Two of the three tests are load-bearing, confirmed by revert: reverting
fast_aggsalone failsagg_bucket_cap.rs:150; revertingaggs.rsalone failsagg_bucket_cap.rs:186anddate_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_termsis another.