do not prune intermediate aggregation results at the root - #6778
do not prune intermediate aggregation results at the root#6778DeviousCardi wants to merge 3 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.
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.