Skip to content

perf(sort-merge): cache current-row bytes in RowValues for SortPreservingMerge - #23990

Open
zhuqi-lucas wants to merge 1 commit into
apache:mainfrom
zhuqi-lucas:perf/spm-rowvalues-cache
Open

perf(sort-merge): cache current-row bytes in RowValues for SortPreservingMerge#23990
zhuqi-lucas wants to merge 1 commit into
apache:mainfrom
zhuqi-lucas:perf/spm-rowvalues-cache

Conversation

@zhuqi-lucas

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of the SortPreservingMerge cursor-cache work in #23840, split into smaller PRs for easier review (per @alamb / @rluvaton). This one carries the largest share of the win.

Rationale for this change

SortPreservingMerge compares cursor heads in a loser tree; every emitted row triggers log2(k) compare calls. For multi-column sort keys the key is serialized into arrow Rows and wrapped in RowValues, and each compare called Rows::row(idx) for both sides — walking Arc<Rows>offsets[idx] / offsets[idx+1] → buffer slice.

The offsets buffer (~65 KB per batch per partition, ×16 partitions) is far too large to stay cache-resident, so those lookups typically land in L2/L3 with a DRAM tail — and they are repeated for the same idx on every compare of a stable loser-tree head, even though the answer never changes until the cursor advances.

What changes are included in this PR?

Cache the current row's (ptr, len) once per Cursor::advance (via the existing CursorValues::set_offset hook) and read it in compare, so the hot path resolves to two plain field loads plus a memcmp instead of two Arc-chased offset walks.

  • The pointer is into the Arc-owned buffer heap and stays valid across struct moves (a cursor is written into a Vec<Option<Cursor<..>>> slot), so Send/Sync are implemented by hand with a SAFETY note.
  • eq / eq_to_previous take arbitrary cross-batch indices and continue to index Rows directly (the cache only holds the current offset).
  • compare keeps a debug-only assert that the cache invariant holds (indices equal the cursors' current offsets).

Only datafusion/physical-plan/src/sorts/cursor.rs changes. Follow-up PRs will apply the same pattern to the single-column string cursors (ByteArrayValues, StringViewArray) and add a null-wrapper fast path.

Are these changes tested?

Yes:

  • test_row_values_cache_matches_rows_index drives the cache across every offset of a multi-row batch and asserts identical ordering to per-row Rows indexing, plus the cross-batch eq path.
  • test_row_values_single_row_batch covers the up-front row-0 cache and the length snapshot.
  • Verified red/green: breaking set_offset (skip the refresh) makes the first test fail as expected.
  • Existing sorts::* merge tests (83) pass.

Are there any user-facing changes?

No API changes. The sort_tpch10 benchmark from the combined series (#23840) showed the multi-column queries driven by this cache: Q4 +1.23x, Q9 +1.15x, Q8 +1.13x, Q5 / Q6 / Q11 ~+1.12x; no regressions. CI benchmark to confirm on this split.

…vingMerge

Multi-column sorts serialize the sort key into arrow `Rows` and wrap it in
`RowValues`. Every loser-tree comparison called `Rows::row(idx)` for each
side, and each call walks `Arc<Rows>` -> offsets[idx] / offsets[idx+1] ->
buffer slice. The offsets buffer is far too large to stay cache-resident,
so those lookups typically cost an L2/L3 hit with a DRAM tail — repeated
for the same idx on every compare of a stable loser-tree head.

Cache the current row's `(ptr, len)` once per `Cursor::advance` (via the
existing `CursorValues::set_offset` hook) and read it in `compare`, so the
hot path resolves to two plain field loads plus a memcmp. The pointer is
into the Arc-owned buffer heap and stays valid across struct moves (the
cursor is written into a `Vec<Option<Cursor<..>>>` slot), so `Send`/`Sync`
are implemented by hand with a SAFETY note. `eq` / `eq_to_previous` take
arbitrary cross-batch indices and continue to index `Rows` directly.

This is the first of a series splitting apart the SPM cursor-cache work
for easier review; it carries the largest share of the win (multi-column
sort-tpch). Follow-ups add the same pattern to the single-column string
cursors and a null-wrapper fast path.

sort_tpch10 benchmark on the full series showed the multi-column queries
(Q4 +1.23x, Q8 +1.13x, Q9 +1.15x, Q5/Q6/Q11 ~+1.12x) driven by this cache.

Tests: `test_row_values_cache_matches_rows_index` checks the cached
ordering against per-row `Rows` indexing across every offset of a batch,
including the cross-batch `eq` path; `test_row_values_single_row_batch`
covers the up-front row-0 cache. Existing `sorts::*` merge tests (83)
pass.
Copilot AI review requested due to automatic review settings July 30, 2026 07:49

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.75%. Comparing base (2e3626e) to head (ea890b0).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #23990   +/-   ##
=======================================
  Coverage   80.75%   80.75%           
=======================================
  Files        1096     1096           
  Lines      373588   373645   +57     
  Branches   373588   373645   +57     
=======================================
+ Hits       301691   301740   +49     
- Misses      53896    53899    +3     
- Partials    18001    18006    +5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants