Repository navigation
do not prune intermediate aggregation results at the root - #6778
DeviousCardi wants to merge 6 commits into
Conversation
Pruning to `segment_size` bounds what a leaf sends upstream, so it is worth paying there. The root has no such consumer: finalization already prunes to the requested `size`, and cutting the candidate set a second time discards counts that finalization could still have used, which surfaces as an inflated `doc_count_error_upper_bound`. Give `QuickwitCollector` a `MergeLevel`, set by whoever builds it, and prune intermediate results only when merging at a leaf. The effect is visible once the root merges two or more leaf responses; a root with a single leaf response returns it untouched and never reached the pruning either way.
0fbc27d to
d134c0e
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fbc27d324
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let merge_collector = make_merge_collector( | ||
| search_request, | ||
| searcher_context.get_aggregation_limits(), | ||
| MergeLevel::Root, |
There was a problem hiding this comment.
When a gRPC RootSearch caller sets skip_aggregation_finalization = true and this root merges two or more leaf responses, finalize_aggregation_if_any returns these intermediate bytes unchanged. Passing MergeLevel::Root here now bypasses intermediate pruning, so a high-cardinality terms aggregation can return roughly the union of every leaf's segment_size candidates rather than a set bounded to segment_size, defeating the response-size bound needed by multi-step callers and potentially exceeding transport limits. Select the leaf/intermediate merge level when finalization is skipped, reserving Root for results finalized locally.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, this was a real hole. finalize_aggregation_if_any returns the merged bytes untouched when skip_aggregation_finalization is set, so with Root there the result was left unbounded.
Fixed in b20d289. The root now picks pruning based on whether finalization will actually run:
fn root_intermediate_pruning(search_request: &SearchRequest) -> IntermediatePruning {
if search_request.skip_aggregation_finalization {
IntermediatePruning::Apply
} else {
IntermediatePruning::Skip
}
}This also showed that leaf-versus-root was the wrong thing to encode: what matters is whether anything downstream still bounds the result. The flag is now IntermediatePruning::{Apply, Skip} and reads the same way at every call site. Covered by test_root_intermediate_pruning_follows_finalization.
`skip_aggregation_finalization` makes `finalize_aggregation_if_any` hand the merged bytes straight back to the caller, so the root merge is then the only place that can bound them. Skipping intermediate pruning there let a high-cardinality terms aggregation return close to the union of every leaf's candidate set. Rename the flag to say what it decides rather than where it is used: the question is whether anything downstream will prune the result, not whether the merge happens at a leaf or at the root.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea9b729b7b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if search_request.skip_aggregation_finalization { | ||
| IntermediatePruning::Apply | ||
| } else { | ||
| IntermediatePruning::Skip |
There was a problem hiding this comment.
When skip_aggregation_finalization is false and multiple leaves return a terms aggregation with a large segment_size, choosing Skip makes the finalizer handle roughly one candidate set per leaf instead of a root-pruned set. The merge itself runs on search_thread_pool, but root_search_aux invokes finalize_aggregation_if_any directly after fetch_docs_phase (root.rs:1100), and into_final_result at root.rs:1151-1152 deserializes and prunes that full union synchronously on the Tokio worker; this newly leaf-count-scaled CPU work can stall unrelated requests. Fuse or offload finalization with the CPU-bound root merge, preserving raw serialization only for callers that actually skip finalization.
AGENTS.md reference: AGENTS.md:L87-L87
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Description
Closes #6676.
merge_intermediate_aggregation_resultprunes merged terms back tosegment_sizeon every call. That is right for a leaf, whose result still has to cross the network and be merged again upstream, but the root has no such consumer: finalization already prunes to the requestedsize. Pruning a second time drops counts that finalization could otherwise have used, and the loss is reported as a widerdoc_count_error_upper_bound.QuickwitCollectorcould not tell the two cases apart, becausemake_merge_collectoris used both by leaf nodes merging their splits (leaf.rs) and by the root merging leaf responses (root.rs). This adds aMergeLevelthat the caller sets, and prunes intermediate results only atMergeLevel::Leaf.Following @trinity-1686a's suggestion on the issue:
Where each level is set:
make_collector_for_splitLeaf— collecting a single splitleaf.rs(2 sites)Leaf— merging splits within a leaf noderoot.rs:844Root— merging leaf responsesroot.rs:1427Root— only contributeswarmup_info, never mergesOne caveat worth stating: the improvement shows up once the root merges two or more leaf responses.
merge_leaf_responsesreturns early when there is a single response, so a single-node deployment never reached the root pruning in the first place and is unaffected.How was this PR tested?
test_merge_intermediate_terms_prunes_to_segment_sizefrom #6674 is now parameterised over the merge level, and a second case covers the root:Leaf— nine distinct terms merge down tosegment_size(4), unchanged behaviour.Root— all nine survive, so finalization still has the full candidate set to choose from.The REST API aggregation scenarios were also run against a local build, and pass unchanged:
Those scenarios run a single node, so their
doc_count_error_upper_boundexpectations are unaffected for the reason above and needed no update.make fmtis clean.Generated with Claude Opus 5, then reviewed and tested locally before submission.