[eldenmoon]

Background

Variant V2 persists each row's sparse leaves as a dotted-path map, and every reader merges or
binary-searches those keys by raw path bytes. The writer sorts the sparse plan (#70001), and
vertical compaction merges the source sparse row with the materialized subcolumns, so byte order is
a producer obligation all the way to the compacted segment. Nothing checked it, so a regression
would only surface much later as a query failure (Duplicate Variant object key).

What problem does this PR solve?

Issue Number: None

Problem Summary:

Without a check, a writer or compaction merge that breaks the order produces segments whose reads
fail, far from the cause. This change asserts the invariant where it is relied on and keeps the
assertion out of release builds.

Details

• Debug-only checks: DCHECK(is_variant_sparse_row_sorted(...)) in
  • StorageMapRowCursor::bind() (V2 row cursor used by every hierarchical read),
  • the exact sparse-path extractor,
  • SparseColumnMergeIterator::_merge_to for both the source row and the merged destination row,
    because the merge interleaves sparse cells with materialized subcolumn paths.
• Why not repair: query-time and compaction-time tolerance is deliberately not added on this
  branch. Segments must be byte-ordered; a violation is a producer bug.
• Performance: every check is a DCHECK, compiled out in release; queries and compaction keep
  the same scan, sort, and allocation behavior as before.
• Tests: VariantAssemblerTest.DebugCheckRejectsUnsortedSparseRows (death test) feeds a
  two-cell unsorted row and expects the debug check to fire;
  SparseColumnMergeIteratorTest.KeepsByteOrderWhenMergingMaterializedSubcolumn merges a
  byte-sorted source row with a materialized a.d column and asserts the merged row is
  a-b, a.c, a.d. The import side is already covered by
  VariantShredderTest.SparseRowsUseByteOrderWithoutChangingStatisticsSelection.

Testing

• ./run-be-ut.sh --run --filter='VariantAssemblerTest.*:BinaryColumnExtractIteratorV2Test.*:VariantShredderTest.*'
  on dev-x (ASAN): 17 tests passed.
• ./run-be-ut.sh --run --filter='VariantColumnWriterReaderTest.*:IndexStorageVariantCompactionReadTest.*:SparseColumnMergeIteratorTest.*'
  on dev-x (ASAN): 75 passed, 4 pre-existing skips.
• ./run-be-ut.sh --run --filter='SparseColumnMergeIteratorTest.*:VariantAssemblerTest.*'
  on dev-x (ASAN): 2 passed (merge interleaving + death test).
• Negative check: temporarily removed the writer-side std::ranges::sort from VariantShredder
  ([fix](variant) Write Variant V2 sparse keys in byte order #70001). VariantShredderTest.SparseRowsUseByteOrderWithoutChangingStatisticsSelection failed
  its byte-order expectation and the new check aborted in StorageMapRowCursor::bind
  (Check failed: is_variant_sparse_row_sorted), so the assertion does catch a producer that stops
  sorting. The sort was restored afterwards.
• build-support/check-build-hygiene.sh passed.

Release note

None.

Check List (For Author)

• Behavior changed: No (assertions only, active in debug builds)
• Does this need documentation: No
• Pre-submission status: Ready

AI Disclosure

• Tool: DeepSeek Harness
• Generated: code, tests, commit message
• Human review: pending

[eldenmoon]

run buildall

[eldenmoon]

/review

[github-actions]

Codex automated review failed and did not complete.

Error: Review setup stopped before context preparation ran; inspect the first failed or cancelled setup step.
Workflow run: https://github.com/apache/doris/actions/runs/42746524413

Please inspect the workflow logs and rerun the review after the underlying issue is resolved.

[eldenmoon]

run buildall

[eldenmoon]

run buildall

[eldenmoon]

/review

[github-actions]

Codex automated review failed and did not complete.

Error: Review setup stopped before context preparation ran; inspect the first failed or cancelled setup step.
Workflow run: https://github.com/apache/doris/actions/runs/42759347828

Please inspect the workflow logs and rerun the review after the underlying issue is resolved.

[eldenmoon]

run buildall

[eldenmoon]

run buildall

[eldenmoon]

run buildall

[eldenmoon]

/review

[github-actions]

Codex automated review failed and did not complete.

Error: Review setup stopped before context preparation ran; inspect the first failed or cancelled setup step.
Workflow run: https://github.com/apache/doris/actions/runs/42803854926

Please inspect the workflow logs and rerun the review after the underlying issue is resolved.

[eldenmoon]

run buildall

[eldenmoon]

run buildall

[eldenmoon]

/review

[github-actions]

Codex automated review failed and did not complete.

Error: Review setup stopped before context preparation ran; inspect the first failed or cancelled setup step.
Workflow run: https://github.com/apache/doris/actions/runs/42820588081

Please inspect the workflow logs and rerun the review after the underlying issue is resolved.

[eldenmoon]

run buildall

[eldenmoon]

run buildall

[eldenmoon]

/review

[github-actions]

Static review of all six changed files at head 7df2a08. One verified P2 finding: the new compaction destination assertion can compare keys across independent rows when a subsequent sub-batch appends to a nonempty destination, aborting a debug/ASAN BE on valid data. No P0/P1 findings or existing blocking inline comments were identified.

Critical checkpoint conclusions:

• Goal and tests: shared debug assertions implement the byte-order check; the death test covers descending sparse keys. The new merge test checks an initially empty one-row destination and misses the repeated-append case in the inline finding.
• Scope and reuse: the six-file change is focused, with one allocation-free helper shared by exact extraction, hierarchical assembly, and compaction.
• Concurrency/locks: no new thread, shared mutation, lock, or lock-order change.
• Lifetimes/static initialization: checks borrow existing columns; no new production ownership, COW mutation, or global initialization dependency.
• Configuration: no configuration additions or dynamic-change requirements.
• Compatibility: no persisted layout, encoding, or removed symbol changes. SPARSE and DOC producers sort the same raw bytes used by StringRef; bucket reconstruction preserves order. Debug rejection of already unsorted persisted maps matches the stated invariant.
• Thrift/protobuf: no definition changes; field-ID compatibility review is not applicable.
• Parallel paths: reviewed exact/hierarchical reads, SPARSE/DOC, single/multiple buckets, sparse/no-sparse merge, and sequential/rowid calls. The destination-append issue applies to compaction chunking.
• Conditional checks: empty/singleton rows and equal keys remain valid; padded -1 offsets are intentional. The new destination range assumption is the verified defect.
• Test coverage: inspected both new unit tests and relevant existing producer, assembler, bucket, and boundary tests. Repeated appends need coverage. No builds, tests, benchmarks, or build-hygiene checks were executed under this static-only contract.
• Expected results: no golden files changed; the death expectation and a-b,a.c,a.d merge expectation match the constructed input.
• Observability: the failing DCHECK identifies the invariant; no new logging or metrics are needed for this scope.
• Transactions/persistence: no EditLog, version visibility, rowset lifecycle, failover, or delete-bitmap update changes. Existing bitmap filtering supplies a valid trigger for the finding.
• Writes/atomicity/crashes: the diff adds no encoding or commit-protocol changes; the verified assertion can terminate valid compaction in debug/ASAN builds.
• FE-BE variables: none added or changed.
• Performance/memory: helper checks allocate nothing and run only with DCHECK enabled. Release uses -O3 -DNDEBUG; disabled expressions introduce no helper scan, and the empty outer loop is optimizable away. No release regression established by source inspection.
• Other module checks: no hub-header/PCH, extern-template, unity source inclusion, glibc/system-library, or physical segment-ID changes. Existing nullability and COW handling are unchanged.

User focus: no additional focus was supplied. All candidates and risk items have explicit dispositions; no unresolved static coverage gaps remain. Review converged after two rounds. Runtime behavior and the author's reported test runs were not independently executed.

Review converged after 2 round(s).

[github-actions]

[P2] Limit the destination assertion to the row just appended

row restarts at zero for each source batch, but _merge_to appends to dst. SegmentIterator::_read_columns_by_index can call this iterator repeatedly on the same destination for 256-row chunks of discontinuous row IDs, for example after delete-bitmap filtering. On the second call, row == 0 makes this start offset dst_sparse_column_offsets[-1] == 0, so valid rows containing z followed by a new row containing a are checked as one sequence and abort a debug/ASAN BE. Capture the destination key count before appending each row (or use its actual last offset) and check only that row's range; add coverage that appends a second batch to a nonempty destination.

[eldenmoon]

/review

[github-actions]

Static review of all six changed files at head 46c236b. No additional inline findings. The existing P2 destination-append assertion issue still applies and should be fixed: a subsequent batch can check keys across independent destination rows and abort a debug/ASAN BE on valid data. It was independently confirmed and suppressed as a duplicate. No existing P0/P1 inline findings were identified.

Critical checkpoint conclusions:

• Goal and evidence: the shared checks detect descending sparse keys at read/merge boundaries. The new death test reaches the intended assertion; the positive merge test checks byte order. The known destination-range defect prevents the intended invariant check from being correct for all valid batch sequences.
• Scope and reuse: the change is focused on one allocation-free helper, its reader/compaction assertions, and two unit tests.
• Concurrency and locks: the checks read existing batch columns and add no thread, shared mutation, lock, lock ordering, or work under a new lock.
• Lifecycle and static initialization: no new production ownership, COW mutation, resources, cycles, or global initializers. The new test's input columns, options, stats and path set outlive their uses.
• Configuration: no added configuration or dynamic-update behavior.
• Compatibility: no persisted layout, encoding, removed symbol, or wire-protocol change. Current SPARSE and DOC producers sort the full raw path bytes, matching StringRef and existing consumers; bucket combination sorts the same way. Deliberately unsorted legacy producer data already violates the consumer ordering contract.
• Thrift/protobuf IDs: no gensrc changes; cross-branch field-ID comparison is not applicable.
• Parallel paths: inspected exact/hierarchical reads, SPARSE/DOC storage, single/multiple buckets, sparse/no-sparse merge and sequential/rowid calls. Source caches reset between batches; destination columns can remain populated, which confirms the existing P2.
• Conditional checks: empty/singleton rows and equal adjacent keys satisfy nondecreasing order. Padded offset[-1] access is valid. Exact extraction validates offsets before the new predicate; the destination check's batch-local range is the known defect.
• Test coverage: inspected the new negative and interleaving tests plus relevant existing producer, legacy assembler and boundary coverage. The new positive test uses one row into an empty destination and misses repeated appends, as the existing thread describes. No builds, tests, benchmarks or hygiene checks were executed under this static-only contract.
• Expected test results: no result/golden files changed. The death diagnostic and expected a-b,a.c,a.d merge order match the constructed inputs.
• Observability: the DCHECK diagnostic identifies the violated expression; no additional logging or metrics are necessary for this narrow assertion change.
• Transactions and persistence: no changes to EditLog/replay, commit/version visibility, rowset lifecycle, delete-bitmap updates or failover handling.
• Writes, atomicity and crash behavior: existing append/encoding protocols are retained. The existing P2 can terminate valid compaction in debug/ASAN builds; no separate write atomicity or cleanup defect was found.
• FE-BE variables: none added or changed.
• Performance and memory: the helper allocates nothing and does a linear scan only when its DCHECK expression is enabled. Disabled expressions introduce no helper scan; the assembler's otherwise empty loop has no independent side effects. No optimized binary or performance measurement was inspected.
• Other applicable module checks: no new hub-header/PCH dependencies, explicit template instantiations, source .cpp test inclusions, glibc API dependencies, physical segment-ID assumptions or nullable/COW handling changes were introduced. No distinct additional issue was substantiated.

User focus: no additional focus was supplied. Review converged after 1 round: all three reviewers returned NO_NEW_VALUABLE_FINDINGS, all six changed paths were covered, and all candidates and risk items were verified, deduplicated or dismissed with evidence. No unresolved static coverage gaps remain. Compile/runtime behavior and the author's reported test runs were not independently executed.

Review converged after 1 round(s).

[eldenmoon]

run p0

[eldenmoon]

run vault_p0

[eldenmoon]

run external