fix(group): bound the tied groups the radix top-N selection keeps - #591
Merged
Merged
Conversation
The native top-N of the radix path kept every group at or beyond the threshold. Groups strictly beyond it number fewer than N, but the groups AT the threshold can be nearly all of them: a count-per-group top-10 over a near-unique key has a threshold of 1, every group ties it, and the "kept superset" is the whole grouping, heap-sorted by first row — a comparison sort over 100M pairs for a query whose answer is ten rows. The emitted prefix is the tied groups' first-seen order, so only the N tied groups with the smallest first row can ever be taken. The selection now keeps the strictly-better groups plus exactly those N, through a bounded max-heap, and sorts at most 2N entries. The rows emitted are the ones the full kept set produced. Tests: the pinned kept count of the radix native top-N case becomes N; a near-unique two-key grouping whose threshold every group ties, in both directions and under a row selection, against the full grouping. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.
Bug
The native top-N of the radix grouping path (
agg_radix_select_topn,src/ops/agg_engine.c) keeps every group at or beyond the threshold, then heap-sorts the kept set by first row to emit it in first-seen order. Groups strictly beyond the threshold number fewer than N by construction, but the groups at the threshold can be nearly all of them: a count-per-group top-10 over a near-unique key pair has a threshold of 1, every group ties it, and the "kept superset" is the whole grouping — a comparison sort over ~100M pairs for a query whose answer is ten rows.On a 100M-row table,
by: [WatchID ClientIP] c: (count …) desc: c take: 10went from 3.4 s to 78 s, and the same query under a row selection from 0.45 s to 6.5 s; 53% of the profile isagg_sort_pairs_by_key.Fix
The emitted prefix is the tied groups' first-seen order, so only the N tied groups with the smallest first row can ever be taken. The selection now collects the strictly-better groups (fewer than N) and, among the groups equal to the threshold, exactly the N with the smallest first row through a bounded max-heap, the same scheme
agg_radix_select_first_nuses. The sort then runs over at most 2N entries. When more than N groups turn out strictly better than the threshold (which the threshold contract excludes) the selection declines and the caller takes the full path, as it does for an aggregate without a scalar order.The rows emitted are the ones the full kept set produced: identical output on every shape checked, including the ascending direction and a row selection. The same query is now 1.8 s and 0.35 s, faster than before the native selection.
Tests
test/test_agg_contract.cagg_contract/radix_native_topn: the pinned kept count of the tie-heavy case becomes N (it counted the whole tie set before).test/rfl/group/emit_filter_v2_route.rfl: a near-unique two-key grouping whose threshold every group ties, in both directions and under a row selection, against the full grouping.