compute: thin peek results by partitioning, not sorting - #38634
Open
antiguru wants to merge 1 commit into
Open
Conversation
Thinning only needs to know which rows fall outside the first `max_results`, not the order among those that stay. That order is established once anyway, when the answer is collected, so sorting here costs a log factor per row for an ordering that is then thrown away. Over a walk of N rows, thinning runs about N/max_results times on twice `max_results` rows each, which turns `O(N log max_results)` into `O(N)`. It is the walk these peeks spend their time in: a finishing that carries an ordering is never streamable, so such a peek never reaches the peek stash and accumulates until the walk ends. Partitioning is unstable, so when rows tie across the cut it is unspecified which of them survives, and entries carry counts, so the retained multiset does depend on that choice. What the client reads does not, for three reasons together. A tie here means the rows are byte-identical, because the tiebreaker compares the whole encoded row. Exactly `max_results` entries stay, each with a count of at least one, so they expand to at least `max_results` rows. And `max_results` is `limit + offset`, which the finishing reads as `offset..offset + limit` of the merged answer, whose prefix depends only on the prefixes of the runs it merges. The second and third are why a peek result must not be consumed without applying the finishing's limit. Thinning no longer orders what it keeps, so the two tests that asserted an exact order over a thinned result compare multisets instead. They passed on the partition's incidental output, which is not a property the implementation promises. The phase's fields become `thinning_time` and `rows_thinned`, since neither measures a sort any more. The two metrics keep their names, which are already released, and their help text now describes thinning rather than sorting. The reasoning here is Aljoscha's, from #38041, which is closed.
antiguru
force-pushed
the
peek/thinning
branch
from
September 3, 2026 08:28
8b52535 to
6c7d0a5
Compare
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.
Thinning only needs to know which rows fall outside the first
max_results, not the order among those that stay. That order is established once anyway, when the answer is collected, so sorting here costs a log factor per row for an ordering that is then thrown away. Over a walk of N rows, thinning runs about N/max_results times on twicemax_resultsrows each, which turnsO(N log max_results)intoO(N). It is the walk these peeks spend their time in: a finishing that carries an ordering is never streamable, so such a peek never reaches the peek stash and accumulates until the walk ends.Partitioning is unstable, so when rows tie across the cut it is unspecified which of them survives, and entries carry counts, so the retained multiset does depend on that choice. What the client reads does not, for three reasons together. A tie here means the rows are byte-identical, because the tiebreaker compares the whole encoded row. Exactly
max_resultsentries stay, each with a count of at least one, so they expand to at leastmax_resultsrows. Andmax_resultsislimit + offset, which the finishing reads asoffset..offset + limitof the merged answer, whose prefix depends only on the prefixes of the runs it merges. The second and third are why a peek result must not be consumed without applying the finishing's limit.Two properties of the layers below make this safe here. The unordered early stop and the peek stash both require an absent ordering, while thinning requires one, so neither ever consumes a partitioned prefix. And
select_nth_unstable_byis reached only after a row has been pushed and only when the prefix holds twicemax_resultsentries, so its index is always below the length.Thinning no longer orders what it keeps, so the two tests that asserted an exact order over a thinned result compare multisets instead. They passed on the partition's incidental output, which is not a property the implementation promises.
The phase's fields become
thinning_timeandrows_thinned, since neither measures a sort any more. The two metrics keep their names, which are already released, and their help text now describes thinning rather than sorting.The reasoning here is Aljoscha's, from #38041, which is closed.