Repository navigation
Conversation
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69515, apache#69513 Problem Summary: An AGG_KEYS read that folds the rows of a key (a query without pre-aggregation, a horizontal compaction, the checksum task) goes through two merges and a row-at-a-time fold. BetaRowsetReader merges the overlapping segments of a rowset with VMergeIterator, VCollectIterator merges the rowsets with a pairing heap, and BlockReader::_agg_key_next_block takes one IteratorRowRef at a time, buffers row references until a Block boundary, copies the values into a staging Block by a hash map keyed by source Block, and aggregates each group. Two wrong results come from the seams between these steps, on tablets with a rowset whose segments overlap on keys (one load that flushed several memtables): 1. Level0Iterator applies the same-key bits of a merged rowset to every row of a Block but the first, so the first row of each Block loses its bit: a key group that crosses a Block boundary comes back as two rows. 2. VMergeIterator orders two rows of one key by RowwiseIterator::data_id(), which is 0 for every LazyInitSegmentIterator, so a REPLACE column can take the value of the older segment. This PR moves the AGG_KEYS fold reads, and only them, onto a new read path; every other read keeps VCollectIterator for now: - VHeapMergeIterator (new) merges the rowsets and the segments of the rowsets that overlap on keys in one heap, like StarRocks' HeapMergeIterator: child indexes in a std heap; each round pops the child whose row goes first, extends its run while its rows still go before the next child's current row (one comparison per row), appends the run to the output Block and pushes the child back. It only sorts. Rows of one key go by child order, the newer rowset or the later segment first (fixes 2). The output Block holds block_row_max rows, or, with a byte budget, the budget divided by the average row size of the children's Blocks. It also orders by a sequence column, descending keys and the older row first, which the next reads to move need; those are unit-tested here. - SameKeyFlags (new) tells the rows of one key apart by comparing each row's key with the row before it, the last row of the previous Block for the first row of a Block (fixes 1). Like StarRocks' build_tie_for_column, it compares a key column of a key type (integer, decimal, date and time, IP, string, nullable or not) by one loop over the Block for its type, with the same comparison as IColumn::compare_at(); any other column by compare_at() row by row. - AggKeyBlockIterator (new) folds the merged Blocks group by group: the keys and the REPLACE columns are copied from a group's first row, which the REPLACE reader function keeps; the other value columns are aggregated with one add_batch_range per group and column; a stretch of one-row groups is copied as is when every reader function returns its single input unchanged. A group may continue into the next merged Block; an output Block ends in front of a new key. - BlockReader::_init_agg_key_iter builds the path: when the rowsets overlap (the same rule as VCollectIterator: a rowset with overlapping segments, or rowsets whose key ranges do not ascend), every segment of a rowset whose reader would merge its segments is a child of the heap and every other rowset is a child read through its rowset reader; otherwise the rowsets are read one after another and no two rows share a key. The row-at-a-time fold (_agg_key_next_block's body, _init_agg_state, _append_agg_data, _update_agg_data, _copy_agg_data, _update_agg_value and their state) is removed, and so is _agg_columns_idx, which only kept the AGG_KEYS value columns out of _insert_data_normal. - rowid_read_ahead.cpp includes storage/iterators.h for StorageReadOptions, which it got from a unity-build sibling before the new files moved the unity batches. Observability: the profile timers BlockReaderVcollectIterInitTimer, BlockReaderRsReadersInitTimer and BlockReaderBuildHeapInitTimer time the same steps of the new path; merged rows are counted as before (rows folded into an earlier row of their key). An aggregate function that cannot be resolved still fails init with the tablet id, schema hash, reader type and version. ### Release note Fix wrong results of AGG_KEYS reads of tablets with a rowset whose segments overlap on keys: a key could come back twice, and a REPLACE column could take an older value. ### Check List (For Author) - Test: Unit Test - New: HeapMergeIteratorTest (key order and child order, sequence column, descending keys, row limit, byte budget, empty batches, columns it cannot compare by), SameKeyFlagsTest (every key type, nullable or not, across Blocks, against compare_at() row by row), AggKeyBlockIteratorTest and AggValueAggregatorTest, and BlockReaderOverlappingSegmentsTest (AGG_KEYS over a rowset with overlapping segments, as a query and as a compaction, with Blocks of 1, 2, 3 and 100 rows; its first case failed on master on 2026-10-23). - Removed: block_reader_agg_flush_test.cpp, which drove the removed row-at-a-time fold. - ASAN build: 692 read-path and compaction tests (BlockReader*, VGenericIteratorsTest*, *Changelog*, *Compaction*, SegCompaction*, *RowIdConversion*, TabletReaderTest*, *SchemaChange*, Vertical*, RowTtl*, OrderedDataCompactionTest*, QueryCacheTest*, ...) pass, 7 skip themselves. - Behavior changed: Yes (the two wrong results above are fixed) - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…pping segments ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69515 Problem Summary: test_overlapping_segments_merge loads an AGG_KEYS table with the MemTable.need_flush debug point, so one load writes a rowset whose two segments both hold keys 0..19, the second one with the newer rows. It checks that the load really wrote an OVERLAPPING rowset with several segments and that the scan folds the rows itself (PREAGGREGATION: OFF), then reads with Blocks of 3 rows, which cut key groups at Block boundaries: every key must come back once, with the SUM of both rows and the REPLACE value of the newer segment. The result must stay the same after a full compaction rewrites the rowset. The .out file is generated by the framework. ### Release note None ### Check List (For Author) - Test: Regression test - Passes on a local FE + BE (ASAN) built from this branch. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
csun5285
requested review from
airborne12,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
November 11, 2026 06:03
…ocks' iterators ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69515 Problem Summary: Review follow-ups for the AGG_KEYS read over a heap merge, no behavior change: - VHeapMergeIterator keeps the children that still have rows in a std::priority_queue of child indexes with a named comparator, instead of a std::vector driven by std::ranges::push_heap / pop_heap and a lambda. The average row size of the byte budget is summed over the children, a child that is done holding no rows and no bytes. - Every child holds its current Block directly. Nothing shares the Block any more: the rows are copied out of it, and the std::shared_ptr was what VMergeIterator needed for its row references. - MergeDecision describes the compared columns with the SortDescription of a sort, one {column, direction, NULLs direction} per column, like StarRocks' SortDescs: the key columns (descending for ORDER BY ... DESC) and the sequence column (descending, or ascending for the changelog TSO) are entries of one list, so the comparison is one loop over it and then the child order. MergeDecision keeps how many leading entries are key columns, which the check of issue apache#66390 needs, and `older_first`. - SameKeyFlags is gone. Like StarRocks' ChunkAggregator, AggKeyBlockIterator marks the rows of one key itself: build_equal_flags_for_column() (new, core/column, next to EqualFlags; like StarRocks' build_tie_for_column) compares the adjacent rows of one column by one loop for its type, and is called for every key column; the first row of a merged Block is compared with the last row of the output, the key of the group still open, so no copy of the last key is kept. The value aggregator is a nested class, AggKeyBlockIterator::ValueAggregator. ### Release note None ### Check List (For Author) - Test: Unit Test - ColumnEqualFlagsTest (was SameKeyFlagsTest): every key type, nullable or not, several columns, and the columns compared by compare_at(), against compare_at() row by row. - HeapMergeIteratorTest and AggKeyBlockIteratorTest updated; a merged Block that starts with a new key added. - ASAN build: the 692 read-path and compaction tests of the first commit pass, 7 skip themselves. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…within one Block ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69515 Problem Summary: Like StarRocks, the heap merge hands out the rows of one key in the order they were written, and the fold takes the newest value of a key from its last row: - MergeDecision keeps the key columns and the sequence column apart: `keys`, a SortDescription, and an optional `sequence`, ascending, so that the row with the largest value comes last. Rows equal on both go by child order, the older child first; `older_first` and `num_key_entries` are gone. The check of issue apache#66390 applies to every key entry; the sequence column only has to be in the Blocks. - With `whole_key_groups`, which the AGG_KEYS read turns on, a merged Block ends only in front of a new key: past the row limit only the rows of the last key go on, fewer than there are children, since every child holds a key once. - AggKeyBlockIterator therefore folds every key group within one merged Block, and the state that carried a group from one merged Block to the next is gone: the open group, the comparison of a merged Block's first row with the last output row, and the open state of ValueAggregator. ValueAggregator::fold_group() folds one group and appends its row. The key and REPLACE columns are copied from the group's last row. REPLACE_IF_NOT_NULL takes the last value that is not NULL with its `_load` function, whose pointer to the row stays valid because the merged Block still holds the row when the result is appended. A DORIS_CHECK fails the read when a merged Block goes on with the key of the Block before it, instead of emitting the key twice. Behavior change: the value columns of one key are folded oldest first on the reads of this path: queries that fold, horizontal compactions and checksums. SUM, MIN, MAX, REPLACE, REPLACE_IF_NOT_NULL, HLL_UNION and BITMAP_UNION give the same results as before. An order-sensitive GENERIC aggregate state, such as agg_state<group_concat(...)>, now merges the rows of a key in write order, as the load path does, instead of newest first. Vertical compaction, the default, still merges newest first until it moves to the heap merge too. ### Release note None ### Check List (For Author) - Test: Unit Test - HeapMergeIteratorTest: the older child first, the sequence column ascending, a Block that goes past the row limit with the rows of its last key, a sequence column out of range. - AggKeyBlockIteratorTest and its ValueAggregator tests: groups folded within one merged Block, REPLACE from the last row, REPLACE_IF_NOT_NULL taking the last value that is not NULL. - ASAN build: 834 read-path and compaction tests, 831 pass and 3 skip themselves. - Behavior changed: Yes (the merge order of order-sensitive GENERIC aggregate states, see above) - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eader setup ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69515 Problem Summary: Review follow-ups for readability, no behavior change: - VHeapMergeIterator always ends an output Block in front of a new key: MergeDecision loses `whole_key_groups`, which AGG_KEYS turned on and whose default was the setting the fold rejects. It reads the byte budget as given, 0 for none, and BlockReader passes the one it has resolved against `enable_adaptive_batch_size`, so the switch is read in one place. The compare contract is checked once at init, also when no child has rows, `_eof` and the factory function are gone, and BlockReader and the tests build the class directly. - AggKeyBlockIterator: the input iterator is `_input` and the Block read from it `_input_block`, instead of `_rows` and `_merged`, a name that also meant the merged rows and a non-merged Block. The check that an input Block does not go on with the key of the Block before it moves to `_advance_input_block()`, so reading and marking an input Block no longer take the output columns, and its comment says why the output is empty only in front of the first input Block. - BlockReader forward-declares AggKeyBlockIterator, with its constructor out of line next to the destructor. ### Release note None ### Check List (For Author) - Test: Unit Test - HeapMergeIteratorTest: the byte budget without the global switch, the rows of one key kept in one Block, and the compare contract also checked when no child has rows. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ets that share no key directly ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69515 Problem Summary: - AggKeyBlockIterator folds every input Block as a whole. An input Block from the heap merge holds at most the row limit of keys, so it always fits into an empty output Block; an output Block takes the next input Block only while the rows it folds into fit into the row limit and, at the input Block's average row size, into the byte budget. The next input Block is read ahead after every fold, so no state is carried inside an input Block or between calls besides the Block itself, and the check that an input Block does not go on with the key of the Block before it runs at every Block boundary. The per-group checks, the 64-row byte measurement, the resume point inside an input Block and the deferred copy of the key rows are gone. - A key that has one row is copied as it is stored, for every aggregation, as the reads that need no merge return it. Before, the QUANTILE_UNION and GENERIC values of such keys went through their union function, so the bytes of a key could depend on how the rowsets lie and a checksum could differ between replicas. - AGG_KEYS reads whose rowsets hold every key once (no rowset with overlapping segments, keys ascending from one rowset to the next, the check VCollectIterator already uses) are read as they are, like DUP_KEYS, through `_direct_next_block`, instead of copying every row through the fold. One decision, `_folds_agg_keys()`, picks both the init and the next-block function. AggKeyBlockIterator thus always reads a VHeapMergeIterator and loses its `merged` flag; the rowset reader of a rowset whose segments do not overlap becomes a heap child of one reader, which the heap releases once the rowset is read. Such reads now hand up the Blocks of the segments, as DUP_KEYS reads do, instead of rows gathered into full Blocks, so rows that tie on an ORDER BY can come out in another order. test_rename_column compared such ties in their order; its four `SELECT *` queries now compare sorted rows (order_qt), with the same expected output. - ValueAggregator::fold_group() asks each group's rows whether they hold a NULL, so a nullable function takes its batch path from the rows it adds. start_block() is gone: it had to be called for every refill of the same Block, and a missed call would have added the values under NULLs. ### Release note None ### Check List (For Author) - Test: Unit Test - AggKeyBlockIteratorTest: whole input Blocks taken while they fit the row limit and the byte budget, counted by the rows they fold into; keys that have one row copied as they are, also in front of a folded key. - AggKeyBlockIteratorValueAggregatorTest: nullable MIN over groups whose NULL is the first row, the last row or none, so a wrong NULL range changes the result. - BlockReaderOverlappingSegmentsTest: two one-segment rowsets whose key ranges share a key. - ASAN build: 835 read-path and compaction tests, 832 pass and 3 skip themselves. - Test: Regression test - schema_change_p0/test_rename_column compares sorted rows. - Behavior changed: No (output Block sizes, and with them the order of rows that tie on an ORDER BY, change; the rows do not) - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…d drop its caches ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69515 Problem Summary: Review follow-ups for readability, no behavior change: - `_less(a, a_pos, b)` becomes `_goes_before(child, row, other)`, whose names say which child and which row are compared; the column comparison it ran twice, `_compare()`, is a lambda in it. HeapOrder names its arguments `lhs` and `rhs` and says why it swaps them. `_fill()` becomes `_fill_child()`, and its `exhausted` flag `eof`. - A child keeps its iterator, its Block and its current row only: its row count and byte size were copies of `block.rows()` and `block.bytes()`, which `_max_rows()` now reads, so a merge without a byte budget no longer measures every Block it reads. - The compare contract check is gone. BlockReader builds the MergeDecision from the leading key columns of the same ReadSchema, and the read it guarded against, a changelog scan whose projection lacks key columns (issue apache#66390), never reaches the AGG_KEYS fold, which `_init_agg_key_iter()` checks; VCollectIterator and VMergeIterator keep their own. The tablet id the heap kept for its message goes with it. ### Release note None ### Check List (For Author) - Test: Unit Test - HeapMergeIteratorTest, AggKeyBlockIteratorTest and BlockReader tests pass; the test of the removed check is gone. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…Block in the heap merge ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69515 Problem Summary: Like StarRocks' HeapMergeIterator, VHeapMergeIterator takes a child's Block whole when no row of it has been taken and its last row, and so every row, goes before the next child, which one comparison tells: - An output Block that is still empty takes the Block's columns and gives the child its own empty ones to refill, so the rows are neither compared one by one nor copied. - An output Block that such a Block does not fit into ends in front of it, so that the next one takes it whole. Otherwise a child whose Block was split once stays out of step with the output Blocks and is never handed over again. It does not end there when the Block goes on with the output Block's last key, so the rows of one key still never land in two Blocks. The rows and their order do not change, only where output Blocks end. It pays where the children barely overlap, as a large rowset under a few small loads, or rowsets that share part of their key range; children that interleave row by row are merged as before, with one more comparison per child Block. next_batch() now takes an empty Block, which AggKeyBlockIterator and the tests pass. ### Release note None ### Check List (For Author) - Test: Unit Test - HeapMergeIteratorTest: an empty output Block takes the columns of a Block that goes first whole, an output Block ends in front of such a Block that does not fit, and does not when the Block goes on with its last key. - Behavior changed: No (output Block sizes change; the rows do not) - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ual rows ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69515 Problem Summary: The nullable loop of build_equal_flags_for_column() compared the raw null-map bytes of two rows, while ColumnNullable::is_null_at() takes any byte other than 0 as NULL: two NULLs with the bytes 1 and 255, or with the byte 2 each, were flagged as different keys, although compare_at() finds them equal. The segment readers write only 0 and 1, so the reads of this PR were not affected; the helper now reads each byte as NULL or not first. ### Release note None ### Check List (For Author) - Test: Unit Test - ColumnEqualFlagsTest.TakesEveryNonZeroNullByteAsNull: the null bytes 1, 255, 2, 2 and 254, over different values underneath, give the flags of compare_at(). - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…y across merged Blocks ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69515 Problem Summary: Review follow-ups: - Merge order. VHeapMergeIterator hands out the rows of one key newest first again, the newer child first, as VCollectIterator and vertical compaction do; the fold takes the key and REPLACE columns from a key's first row, and REPLACE_IF_NOT_NULL from its `_reader` function, the first value that is not NULL. With the oldest-first order of the earlier commits, a value whose result depends on the order of the rows (an AGG_STATE<map_agg_v2> or group_concat state, a DOUBLE SUM) read without pre-aggregation could change after a vertical compaction, which still merges newest first, and a horizontal compaction stored another value than a vertical one. - Hot keys. The merge no longer goes past its row limit with the rows of the last key: an output Block ends at the limit, also inside the rows of one key. Like StarRocks' ChunkAggregator, AggKeyBlockIterator keeps the last group of an input Block open (its key and REPLACE values in the output, its other values in the aggregate states) until the next input Block shows whether it goes on with that key, and ends an output Block only in front of a new key, so every key still comes out once. Before, a key with a row in each of N overlapping rowsets put all N rows into one merged Block whatever the byte budget. A key of one row that ends an input Block is kept as it is stored until the next Block shows whether it goes on, so such keys are still copied as they are. - The fold follows StarRocks' order: (1) the groups of an input Block, (2) the key and REPLACE columns from the first row of each group, (3) the other value columns, column by column, runs of one-row keys copied as one range. An output Block fills to its row limit, the byte budget counted as rows like the limit of the merge (the budget divided by the average row size of the input Block), instead of taking whole input Blocks while their estimated size fits. - BlockReader builds the key ordinals once for the merge and the fold, and decides once whether the rowsets overlap. ### Release note None ### Check List (For Author) - Test: Unit Test - HeapMergeIteratorTest: the newer child first, the sequence column in its direction, a Block that ends at the row limit inside the rows of a key. - AggKeyBlockIteratorTest: a key carried through three input Blocks, a one-row key at the end of an input Block copied as it is or folded with the next Block, output Blocks filled to the row limit and ended in front of a new key only, the byte budget counted as rows. - AggKeyBlockIteratorValueAggregatorTest: groups by row count, REPLACE_IF_NOT_NULL taking the first value that is not NULL, an open group keeping its value across input Blocks. - ASAN build: 841 read-path and compaction tests, 838 pass and 3 skip themselves. - Test: Regression test - data_model_p0/aggregate/test_agg_keys_fold_order: map_agg_v2, group_concat and DOUBLE SUM over three rowsets of one key return the same row before and after a vertical and a horizontal full compaction. - Behavior changed: No (master's merge order again; output Block sizes change) - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
### What problem does this PR solve? Issue Number: N/A Related PR: apache#69515 Problem Summary: test_rename_rollup reads its AGG_KEYS table with `SELECT * ... ORDER BY user_id`, and key 2 holds two rows, loaded by two INSERTs, that tie on user_id. Their rowsets hold ascending keys that do not overlap, so since this PR reads them as they are, one Block per rowset, and the sort above may return the tied rows in either order: run alone, the suite passed once and failed once on the same build. Like test_rename_column, the two `SELECT *` queries now compare sorted rows (order_qt), with the same expected output. ### Release note None ### Check List (For Author) - Test: Regression test - schema_change_p0/test_rename_rollup passes three runs in a row. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ocks' and let rowset readers give their merge children ### What problem does this PR solve? Issue Number: N/A Related PR: apache#69515 Problem Summary: Review follow-ups for readability, no behavior change: - AggKeyBlockAggregator (new agg_key_block_aggregator.{h,cpp}) does what StarRocks' ChunkAggregator does, with its interface: update_source(), source_exhausted(), aggregate(), is_finish(), has_aggregate_data() and aggregate_result(). It holds the source and where it stopped, the same-key flags, the open key and the batch, which it hands over by swapping columns. ValueAggregator moves into it. - AggKeyBlockIterator is the loop of StarRocks' AggregateIterator: it reads a merged Block whenever the aggregator has used up the last one and returns a batch once it is full or the input ends. init() no longer reads ahead. - RowsetReader::get_merge_children() gives the children of a merge that reads a rowset, like StarRocks' Rowset::get_segment_iterators: each segment when the segments overlap on keys, otherwise one VUnionIterator that the rowset reader sets up with the read options its segment iterators were built with. BlockReader no longer wraps a rowset reader in RowsetReaderIterator, and the merge times the reads of every child's Blocks into BlockFetchTime, which the rowset reader timed before. - The heap merge's helpers say what they do: _less_than_current_row(), _can_take_whole_block(), _rows_before_next_child(), _copy_rows(), _move_block_to_output(), _push_back_to_heap(), _read_next_block() and _output_row_limit(). BlockReader::_folds_agg_keys() becomes _merges_agg_key_rows(). ### Release note None ### Check List (For Author) - Test: Unit Test - AggKeyBlockAggregatorTest: the last key of a source carried to the next one, and a batch full in front of a new key with rows of the source left. - ASAN build: 843 read-path and compaction tests, 840 pass and 3 skip themselves. - Behavior changed: No - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <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.
Background
What problem does this PR solve?
Issue Number:
Related PR:
PR Type:
Problem Summary:
Details
Testing
Notes for Reviewers
Release note
Check List (For Author)
AI Disclosure