perf: bound run-index lookup to the requested physical range - #10849
Draft
sunchao wants to merge 1 commit into
Draft
perf: bound run-index lookup to the requested physical range#10849sunchao wants to merge 1 commit into
sunchao wants to merge 1 commit into
Conversation
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.
Why are the changes needed?
Which issue does this PR close?
Closes #10846. Benchmark source is supplied separately in #10848, following the contributor guide's benchmark-first workflow.
Rationale for this change
Draft status: Follow-up measurements found an approximately 33% slowdown for dynamic last-index selections over 1,048,576 physical runs, reproduced in both baseline-first and head-first orders. No follow-up repair has been selected or published. This PR is not claimed to meet a no-regression gate.
Selecting two early logical indices currently scans the remaining physical runs even after both outputs are known. A short logical slice can likewise retain a large backing buffer and scan beyond the relevant range.
What changes were proposed in this PR?
What changes are included in this PR?
Find the physical run containing the already-validated largest requested logical index once, then bound the existing iteration with a slice. The inner loop is unchanged. Enumerating before skipping retains the original physical indices; the existing bounds checks and error fallback remain in place.
Are there any user-facing changes?
Small selections that end early in a large backing buffer avoid scanning later runs. Results, ordering, duplicate handling, slicing, bounds errors, and the public API are unchanged. This is not a new sparse-lookup algorithm for arbitrary distant indices.
How was this PR tested?
Are these changes tested?
arrow-bufferlibrary tests passed, including a new reordered/duplicate prefix test with sliced and invalid-index controls.Representative first-run Criterion point estimates:
Those initial measurements improved prefix cases, but later dedicated-host testing found the late-selection regression described above and full-selection tradeoffs. A tested early-return candidate removed the approximately 33% slowdown but remained 5–8% slower than baseline in one full-selection harness; a separate full-scan fallback experiment did not remove the regressions. Neither candidate is an accepted or published repair. These are local microbenchmarks, not an end-to-end query-speedup claim. Earlier shared-target comparisons were discarded because Cargo reused a binary across worktrees; an iterator-
takevariant also regressed full scans and is not included in this PR.AI assistance: Codex generated the implementation, regression test, benchmark, and PR text, and performed the stated local checks and source review. This does not claim a separate human review.