[improvement](load) sort memtable with the vectorized ColumnSorter - #66588
Draft
liaoxin01 wants to merge 2 commits into
Draft
[improvement](load) sort memtable with the vectorized ColumnSorter#66588liaoxin01 wants to merge 2 commits into
liaoxin01 wants to merge 2 commits into
Conversation
MemTable keeps one RowInBlock per loaded row in _row_in_blocks, held as a
shared_ptr, so there is one make_shared per row. Measured at 80 bytes per
row: 16 for the pointer in the vector, plus a 64 byte heap chunk holding
the 16 byte control block and the 40 byte struct.
Two of the struct's five fields do not need to be there.
_agg_state_offset held _offsets_of_aggregate_states.data(), the same
pointer for every row, so it belongs on the MemTable. _has_init_agg said
exactly what _agg_mem being non-null already says. That leaves 24 bytes,
small enough to keep in the vector directly and drop the per-row
allocation with it.
For a 7.05M row memtable that is 564 MB down to 169 MB, and building the
array drops from 401 ms to 111 ms.
Dropping the shared_ptr means the rows in _row_in_blocks and the copies
_aggregate() works on are no longer the same object. Two places relied on
that aliasing:
- prev_row now points into temp_row_in_blocks, which is where
_finalize_one_row() will read it from.
- _aggregate() adopts temp_row_in_blocks unconditionally, so the
entries left behind cannot name an aggregate state that
_finalize_one_row() has already released. Without this, a memtable
that aggregates across a shrink_memtable_by_agg() round and then
again in to_block() destroys those states twice -- confirmed by
instrumenting ~MemTable.
memtable_sort_test.cpp only covered class Tie. It now also drives a
MemTable through insert()/to_block() and covers multi-column and nullable
key ordering, the DUP_KEYS tie-break direction, batching independence,
UNIQUE_KEYS last-writer-wins, AGG_KEYS aggregation, and aggregate state
surviving shrink_memtable_by_agg() rounds. The AGG_KEYS schema carries a
BITMAP BITMAP_UNION column so those last cases run over an aggregate
state that owns heap memory rather than a trivially destroyed one.
MemTable::_sort() ran its own multi-key sort: pdqsort over the row array,
with the comparator passed as a std::function that called the virtual
IColumn::compare_at once per comparison. Every comparison paid an
indirect call, a virtual dispatch and two random accesses into the block
before it could look at the key.
The query engine already has the sort this needs. ColumnSorter
(be/src/exec/sort/sort_block.h) implements the same sort-and-tie
algorithm, but keeps an inline copy of the key next to the row id, so a
comparison is a typed, inlinable operation on a compact array.
_sort() and _sort_by_cluster_keys() now build an IColumn::Permutation and
run it through ColumnSorter; _sort_one_column() and class Tie have no
users left and are removed. _sort_by_cluster_keys() no longer needs a row
object per row either -- the LSN sidecar it was carrying is already
indexed by row position, so the permutation can reorder it directly.
Measured on this data shape (7.05M rows, one key column, Release build):
key type current ColumnSorter speedup
int32 13.2 s 0.96 s 13.7x
int64 13.3 s 1.07 s 12.4x
decimal128(20,2) 13.7 s 1.28 s 10.7x
varchar ~4B 19.6 s 3.03 s 6.5x
varchar ~40B 21.9 s 3.75 s 5.8x
nullable int32 16.0 s 1.04 s 15.5x
nullable varchar ~4B 25.4 s 2.81 s 9.0x
Fixed-width keys gain the most because their inline value is the value
itself; string keys still dereference the arena for the memcmp, so the
longer the key the smaller the gain.
The inline permutation costs memory in proportion to the key width: 8
bytes per row for INT32, 16 for INT64, 24 for a StringRef, 32 for
Decimal128 after alignment. It is a std::vector local to
ColumnSorter::_sort_by_inline_permutation, so it is released between key
columns and the peak holds one of them, plus 8 bytes per row for the
permutation and 1 for the equal flags. Against the 24 bytes per row the
memtable now spends on the rows themselves, the peak of the two together
still comes out below what the previous sort needed.
Ordering is unchanged, tie-break included: rows whose whole key is equal
are still stabilised on descending row position for DUP_KEYS and on
ascending row position for everything else.
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
11 tasks
Contributor
Author
|
run buildall |
liaoxin01
marked this pull request as draft
August 7, 2026 16:55
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves BE load-path MemTable sorting performance by replacing the memtable’s bespoke multi-key pdqsort + virtual IColumn::compare_at comparator with the vectorized query-engine ColumnSorter, and updates MemTable’s row bookkeeping to avoid per-row heap allocations. It also significantly expands unit tests to validate ordering, tie-break behavior, and aggregation correctness across shrink rounds.
Changes:
- Switch
MemTable::_sort()/_sort_by_cluster_keys()to build and sort anIColumn::PermutationviaColumnSorter, then apply it in-place (cycle-following) to reorder rows/sidecars. - Store
RowInBlockby value (instead ofshared_ptr) and remove the now-unusedTie/_sort_one_column()machinery. - Replace the previous minimal
Tietest with MemTable-driven tests covering multi-key ordering, DUP/UNIQUE/AGG semantics, NULL ordering, batching independence, and a long-cycle permutation case.
Review Checkpoints (skill Part 1.3)
- Goal & proof: The stated goal (faster memtable sort while preserving byte-identical ordering/tie-break) is implemented; unit tests in
be/test/load/memtable/memtable_sort_test.cppprovide targeted coverage for the ordering invariants. - Change scope: Changes are focused on memtable sorting/row bookkeeping and the associated unit tests.
- Concurrency: No new concurrency primitives/locks introduced in the diffs reviewed; changes appear confined to memtable’s single-threaded build/sort/aggregate lifecycle.
- Lifecycle/memory safety: Row aggregation state ownership changes (by-value rows) are handled by adopting finalized rows back into
_row_in_blocksto avoid double-destroy; however, the newColumnSorterpath introduces index-size constraints that should be asserted (see stored comment). - Configs/compatibility/observability: No new configuration knobs, persistence formats, or protocol-visible changes identified in these diffs.
- Test coverage: BE unit tests were expanded meaningfully for the touched behavior; no regression tests added in this PR.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
be/test/load/memtable/memtable_sort_test.cpp |
Replaces Tie-only test with MemTable end-to-end tests validating ordering/tie-break, batching, and aggregation/shrink behavior. |
be/src/load/memtable/memtable.h |
Stores RowInBlock by value, removes Tie/_sort_one_column, adjusts aggregation APIs to refs, and adds permutation-sort helper declaration. |
be/src/load/memtable/memtable.cpp |
Implements permutation-based sorting via ColumnSorter, applies permutation in-place, updates aggregation paths for by-value rows, and simplifies cluster-key sorting (incl. LSN sidecar reorder). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+403
to
+409
| const size_t num_rows = perm.size(); | ||
| if (num_rows == 0) { | ||
| return 0; | ||
| } | ||
| EqualFlags flags(num_rows, 1); | ||
| EqualRange range {0, static_cast<int>(num_rows)}; | ||
| HybridSorter hybrid_sorter; |
Contributor
TPC-H: Total hot run time: 28841 ms |
Contributor
TPC-DS: Total hot run time: 158531 ms |
Contributor
ClickBench: Total hot run time: 24.01 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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.
What problem does this PR solve?
Problem Summary:
MemTable::_sort()ran its own multi-key sort:pdqsortover the row array,with the comparator passed as a
std::functionthat called the virtualIColumn::compare_atonce per comparison. Every comparison therefore paid anindirect call, a virtual dispatch and two random accesses into the block before
it could even look at the key.
The query engine already has the sort this needs.
ColumnSorter(
be/src/exec/sort/sort_block.h) implements the same sort-and-tie algorithm --sort by a key column, mark equal ranges, refine within them using the next key
column -- but keeps an inline copy of the key next to the row id, so a comparison
becomes a typed, inlinable operation on a compact array instead of a chain of
random accesses.
This PR makes
_sort()and_sort_by_cluster_keys()build anIColumn::Permutationand run it throughColumnSorter._sort_one_column()and
class Tiehave no users left and are removed._sort_by_cluster_keys()no longer needs a row object per row either: the LSN sidecar it was carrying is
already indexed by row position, so the permutation can reorder it directly.
Measured with a micro-benchmark on 7.05M rows, one key column, Release build:
Fixed-width keys gain the most because their inline value is the value itself;
string keys still dereference the arena for the
memcmp, so the longer the keythe smaller the gain. No key type regressed.
The motivating case was a load whose profile showed
MemTableSortTimeat 83 sacross 7 memtables of a single tablet, with the sink blocked in
WaitFlushLimitTimefor 45 s behind it.Memory
Keeping a copy of the key next to each row id costs memory in proportion to the
key width: 8 B/row for
INT32, 16 B forINT64, 24 B for aStringRef, 32 Bfor
Decimal128after alignment. It is astd::vectorlocal toColumnSorter::_sort_by_inline_permutation, so it is released between keycolumns and the peak holds one of them, plus 8 B/row for the permutation and
1 B/row for the equal flags.
That is why #66545 comes first. Against the 24 bytes per row the memtable spends
on the rows themselves after that change, the peak of the two together is below
what the previous sort needed, for every key type:
Merging this one without #66545 would instead take the peak up, to 683 MB for
an int32 key and 853 MB for a decimal128 one, which is why they are ordered.
Tie-break
Ordering is byte-for-byte identical to before, tie-break included: rows whose
whole key is equal are stabilised on descending row position for
DUP_KEYSand on ascending row position for everything else, exactly as the previous
is_dup ? lhs->_row_pos > rhs->_row_pos : lhs->_row_pos < rhs->_row_posdid.That
DUP_KEYSdirection has no semantics behind it -- it reproduces theiteration order of the skip list MemTable used before #18686, where
SkipList::Insertlinked a new node ahead of the existing equal keys, soiterating yielded equal keys in reverse insertion order. The skip list is long
gone and #18686, #19099 and #20392 each carried the ternary along without it
meaning anything.
Dropping it is nevertheless not free: a run of P0 with the tie-break normalised
to ascending fails 39 cases, and only about half of those are a missing
ORDER BY. In the rest the content changes rather than the order -- which rowwins in a
UNIQUEtable fed by an unorderedselect, which element survivescollect_set(k, 1), the element order insidearray_agg, the auto-incrementid-to-row mapping,
first_valueover a window whoseORDER BYhas ties. Thosecan only be "fixed" by rewriting the expected output. So the direction is kept
here, and removing it is left as its own change.
Release note
None
Check List (For Author)
The
MemTable-driven cases inbe/test/load/memtable/memtable_sort_test.cppcome from #66545 and cover the ordering this change has to preserve. This PR
adds one more, for a permutation that is a single long cycle rather than the
short swaps the other cases happen to produce, since the permutation is applied
to the row array in place by following its cycles. The tie-break case was
verified to fail when the direction is flipped, so it does discriminate.
Behavior changed:
Does this need documentation?