enhance: rewrite storage.MergeSort as a proper k-way merge

[bigsheeper]

issue: #51981

What

storage.MergeSort is the default mix-compaction path (dataNode.compaction.useMergeSort defaults to true), reached via mix_compactor.go -> merge_sort.go:135. Three changes:

1. Heap holds k entries, not every in-flight row. enqueueAll pushed every qualifying row of the current record into the queue, so the heap held the in-flight row count rather than the reader count. Each input record is already sorted by the merge key, so the standard form applies: one entry per reader, pop one and push one. At 30 readers x 4096 rows/record the heap goes from ~246k entries to 30.

2. Merge keys resolved once per record. The comparator did recs[x.ri].Column(fid).(*array.Int64).Value(x.i) per side per comparison -- a map lookup plus a type assert whose result is invariant for a record. They are now extracted in advanceRecord (Int64Values() for int64, the *array.String pointer for varchar; both O(1) and copy-free).

3. No per-row allocation. pq.Enqueue(&index{...}) escaped once per row, and container/heap's Push(x any) boxes regardless. Replaced by a small typed value heap over the existing rowIndex{ri, i int32} (added in enhance: speed up sort compaction in storage.Sort #50817). storage.PriorityQueue had no other users repo-wide and is removed, along with the endPositions bookkeeping.

The sibling Sort in the same file already received (2) and (3) in #50817.

Numbers

BenchmarkMergeSort (added here), 200k rows over generateTestSchema() -- 20 fields including 5 vector types. Median of 3, -benchtime 10x -cpu 1:

	before	after
time	523.50 ms	325.96 ms (1.61x)
allocs	302,617	102,599 (-66%)

The gain depends on how much rb.Append dominates. BenchmarkMergeSortVarcharKey, 65k rows over a single varchar field:

	before	after
time	60.83 ms	9.18 ms (6.63x)
allocs	65,642	91
B/op	6.11 MB	4.84 MB (-21%)

The varchar allocation count is the cleanest evidence: 65,642 is approximately the 65,536 rows, i.e. one allocation per row; after is 91 in total, identical across all three runs.

One deliberate behavior change

A k-way merge relies on each input record being sorted by the merge key. That reliance is not new, but the old path did not fail uniformly on disorder, so this is a narrowing and worth stating precisely.

endPositions already depended on sortedness for the general case: an unsorted record could dequeue its smallest key first and trigger advanceRecord while stale entries from the now-invalid record remained queued (RecordReader.Next is borrow-scoped, record_reader.go:20-21), which panics with index out of range -- the [3,1,2] shape.

But it did tolerate a narrower class. Because the old loop enqueued every eligible row of a record, disorder whose maximum key still lands on the last row (a single reader with key order [2,1,3], so endPositions[ri] is the final index) never triggered the early advanceRecord: it was emitted correctly as 1,2,3 and MergeSort returned success. Those inputs now return an error instead. The narrowing is intended -- a bare k-way merge would emit them silently out of order, and trading a loud failure for silent corruption is the one outcome worth avoiding -- and mix_compactor.go:473-484 gates merge-sort to already-sorted segments, so the affected class is vanishingly rare on real data.

To avoid trading a loud failure for silent corruption, the merge now checks that the emitted key never decreases and returns a merr error otherwise. The previous key is held in reusable buffers rather than cloned per row -- cloning would reintroduce exactly the per-row allocation this PR removes.

On #48322 / #48449. An earlier revision of this description named the gate as their root cause and called for a reachability analysis. Both parts have since been resolved, and neither the way that paragraph assumed.

#48322 was an arrow-go use-after-free on STRUCT array deallocation, fixed by milvus-io/arrow#3 and #48775 and verified on master-20260429. #48449 carries the same title; its closure trail is inconclusive and does not point at the gate either. Neither issue supports the attribution.

The reachability analysis has been done, and the scenario it described -- an IsSorted=true segment passing the gate "after namespaceEnabled is turned on" -- is not reachable. EnableNamespace is written only at collection creation (ddl_callbacks_create_collection.go:217); every schema-altering path rebuilds the snapshot through coll.ToCollectionSchemaPB(), which copies the stored value (collection.go:163), so the assignment at collection.go:216 is an identity. And sort_compaction.go:373-374 derives both sorted flags from that same immutable setting (isSorted := !isNamespaceSorted), so a segment's flag and the plan's merge key cannot disagree today.

The gate is nonetheless loose in structure -- it tests whether a segment was sorted, not by which key -- so it is safe by coincidence rather than by construction. This PR now tightens it.

Gate tightened

canMergeSort (extracted from Compact() so it is unit-testable) now requires the sorted flag that corresponds to this plan's merge key, rather than either flag. A mismatch falls through to the existing mergeSplit path before any segment rows are read; no new fallback code.

Two reviewers asked for this. It is defense-in-depth, not a fix for an observed production case -- as established above, the metadata-level mismatch is not reachable today.

The failure that is left loud -- a segment whose flag matches but whose bytes do not -- is now diagnosable. MergeSort reports ErrDataIntegrity (ErrStorage's own doc directs callers here; both are non-retriable SystemError, so retry semantics and classification are unchanged) with a reader N record M row K coordinate, and mergeSortMultipleSegments logs the reader-index to segment-ID mapping alongside it, so the reader index resolves to a segment.

Not done: degrading to mergeSplit when that error fires. Rationale in the review thread -- briefly, the prior behavior for general disorder was a panic, so a deterministic error is not a regression against it; and a metadata/data inconsistency is a should-not-happen class where silent degradation would hide the problem.

Test coverage

MergeSort had no varchar-key test and no multi-field-key test, and TestMergeSort's ordering assertion only checked Value(0) of each output record while the 64MB batchSize yielded a single batch. The first commit adds those baselines and shows them green on the pre-rewrite implementation; the second keeps them green.

• TestMergeSort -- every row checked, small batchSize to force multiple batches
• TestMergeSortVarcharKey, TestMergeSortByMoreThanOneField -- previously uncovered paths
• TestMergeSortPredicateCalledOncePerRow -- pins predicate arity, whose production implementation carries a side effect (segmentTotalRows)
• TestMergeSortUnsortedInputReturnsError -- the ordering check
• TestMergeSortUnsortedInputReportsLaterRecord, TestMergeSortUnsortedInputReportsOffendingReader -- pin the reported coordinate: the first that the record number is computed rather than always 0, the second that the reader index is too
• TestCanMergeSort, TestCanMergeSortMatchesMergeKey -- the gate, both halves of the merge-key truth table plus the segment-count boundary
• TestRowHeap, TestRowHeapSingleElement

Verification

• ./internal/storage/ -run 'TestMergeSort|TestRowHeap|TestSort|TestRadixSort' -- pass
• ./internal/datanode/compactor/... -- pass
• pkg/util/merr guard tests -- pass (an error code changed)
• go build -tags dynamic on both packages -- pass
• make static-check -- not verified locally; the local golangci-lint binary is incompatible with the current Go toolchain (unsupported version: 2). go vet on both packages reports only pre-existing findings in files this PR does not touch. Relying on CI for this one.

Two pre-existing failures in the full ./internal/storage/ package (TestAzureObjectStorage timeout, TestReadFile nil deref) reproduce on unmodified master and are unrelated to this change.

🤖 Generated with Claude Code

[sre-ci-robot]

[ci-v2-notice]
Notice: New ci-v2 system is enabled for this PR.

To rerun ci-v2 checks, comment with:

• /ci-rerun-code-check // for ci-v2/code-check
• /ci-rerun-code-check-macos // for Code Checker MacOS (GitHub Actions)
• /ci-rerun-build // for ci-v2/build
• /ci-rerun-build-all // for ci-v2/build-all (multi-arch builds)
• /ci-rerun-buildenv // for ci-v2/build-env (build milvus-env builder images; update .env after the new tag is ready)
• /ci-rerun-ut-integration // for ci-v2/ut-integration, will rerun ci-v2/build
• /ci-rerun-ut-go // for ci-v2/ut-go, will rerun ci-v2/build
• /ci-rerun-ut-cpp // for ci-v2/ut-cpp
• /ci-rerun-ut // for all ci-v2/ut-integration, ci-v2/ut-go, ci-v2/ut-cpp, will rerun ci-v2/build
• /ci-rerun-e2e-default // for ci-v2/e2e-default
• /ci-rerun-e2e-amd // for ci-v2/e2e-amd (e2e pool dispatcher)
• /ci-rerun-e2e-dist-wp // for ci-v2/e2e-dist-wp (Tencent distributed woodpecker-service boundary)
• /ci-rerun-build-ut-cov // for ci-v2/build-ut-cov (build + unit tests in one pipeline)
• /ci-rerun-gosdk // for ci-v2/go-sdk (Go SDK E2E tests, ARM)
• /ci-rerun-gosdk-std-wp // for ci-v2/go-sdk-std-wp (Go SDK E2E, standalone + embedded Woodpecker)
• /ci-rerun-gosdk-dist-wp // for ci-v2/go-sdk-dist-wp (Go SDK E2E, distributed + Woodpecker service)

If you have any questions or requests, please contact @zhikunyao.

[sre-ci-robot]

✅ CI Loop Results 29cf9ba

Stage	Result	Duration	Tests
✅ Build	SUCCESS	16.0min	-
✅ Code-Check	SUCCESS	8.9min	-
✅ UT-Integration	SUCCESS	25.9min	-
✅ UT-GO	SUCCESS	23.4min	-
✅ UT-CPP-Cov	SUCCESS	59.2min	8499 total, 8499 passed, 0 failed

Total: 83min | Pipeline | Artifacts

Overall Coverage: 73.6%
Diff Coverage: Go 92.1% (140 hit, 12 miss, 152 measurable lines, 77 unmeasured)
Diff Coverage HTML: view changed lines
Total Patch Coverage: 92.1% (140/152 measurable lines, 77 unmeasured)

[czs007]

Adversarial review found no blocking issues; the remaining items are four low-severity and two nitpick-level points on internal/storage/sort.go, all non-blocking.

Low

internal/storage/sort.go:461 — initialization loop drops a record returned alongside io.EOF
The merged init loop does if err := advanceRecord(i); err != nil { if err == io.EOF { continue } ... }, but advanceRecord assigns recs[ri] = rec before returning the error (sort.go:387). If a RecordReader uses the common return rec, io.EOF idiom, recs[i] is non-nil yet seedNext(i) is skipped; since seedNext is afterwards only called for a reader just popped from the heap (sort.go:557), that reader never enters the heap and its entire record is silently dropped from both the output and numRows. The old implementation's second init pass (for i, v := range recs { if v != nil { enqueueAll(i) } }) covered this. Not reachable in production today — every reader on the path (packedRecordReader, ffiPackedRecordReader, ManifestReader, CompositeBinlogRecordReader, materializedRecordReader, timestampOverwriteReader) returns a nil record whenever err != nil — so this is a contract narrowing rather than a live defect. Suggestion: either check recs[i] != nil and call seedNext(i) before continue, or document on the RecordReader interface that the record must be nil when io.EOF is returned. (surfaced during verification)

internal/storage/sort.go:537 — inputs that were previously sorted successfully now fail hard
The new godoc does state the contract (sort.go:336-339 "records already sorted by sortedByFieldIDs"), so the contract itself is declared; what changed is behavior for a narrow input class. The old implementation enqueued every eligible row of a record, so a record whose keys are out of order but whose maximum key happens to land on the last row (e.g. a single reader with key order [2,1,3], endPositions[ri]=2) was emitted correctly as 1,2,3 with no stale heap entries and MergeSort returned success; the new code returns an error on the second pop. The overall direction is right — orders like [3,1,2] previously triggered advanceRecord early and left stale heap entries (the index-out-of-range in the PR body), so replacing silent corruption with an explicit error is an improvement — and mix_compactor.go:473-484 gates merge-sort to already-sorted segments, making the affected subset vanishingly rare on real data. Suggestion: just confirm the narrowing is intended, and consider softening the PR description's claim that the old path tolerated intra-record disorder in general. (raised by xiaocai2333)

internal/storage/sort.go:538 — consider ErrDataIntegrity for the unsorted-input error
The new check returns merr.WrapErrStorageMsg("input record is not sorted by the merge key"), i.e. ErrStorage(1008). Retriability and metrics are unaffected: both codes have retriable == false, and ErrDataIntegrity inherits the zero-value SystemError error type, matching ErrStorage's explicit one. The only argument is code semantics — merr itself asks callers to prefer the more specific code (errors.go:178-180), and ErrDataIntegrity's definition ("don't match the layout we expect … likely permanent corruption; retry won't help") fits a binlog that claims IsSorted but isn't. Counterpoint: per the PR description and #48322, the likely trigger is a mis-routed mix_compactor gate rather than on-disk corruption, and the pre-existing WrapErrStorageMsg("unsupported type for sorting key") at sort.go:379 uses the same code, so the current choice is at least self-consistent. Suggestion: switch to merr.WrapErrDataIntegrity... if you agree with the stricter reading; otherwise leave as is. (raised by xaxys, xiaocai2333)

internal/storage/sort.go:361 — keys lifetime comment states an invariant the code doesn't rely on
The comment says "a reader's keys are only read while that reader has an entry in the heap", but the main loop pops the reader's only heap entry at sort.go:535 and then calls compareWithLast(idx) (537) and saveLast(idx) (540), both of which read keys[x.ri][fp] (sort.go:498, 523) when that reader has no heap entry at all. The code is safe, but for a different reason: keys[ri]/recs[ri] can only be overwritten by advanceRecord via pos[idx.ri]++; seedNext(...) at sort.go:556-557, after both reads. Suggestion: reword to "keys[ri] stays valid until seedNext(ri) advances that reader again", so a future maintainer isn't tempted to move compareWithLast/saveLast after seedNext — which would be a genuine use-after-advance. (raised by xaxys, xiaocai2333)

Nitpick

internal/storage/sort.go:398 — four copies of the key-kind dispatch switch
The int64/varchar dispatch on sortKeyCol.kind appears four times inside MergeSort: extractKeys (sort.go:373-380), compareKeys (401-418), compareWithLast (499-516) and saveLast (524-529). Adding a third key kind (Int32, Bool, multi-type PK) requires four coordinated edits, and each omission fails quietly: a missed case in compareKeys falls through to return 0 (sort.go:418-420) and silently treats the keys as equal, while a missed case in compareWithLast/saveLast disables the unsorted-input detection for that kind entirely. Suggestion: extract a small kind-dispatching helper or hang methods off sortKeyCol; the same duplication exists in Sort (sort.go:137-156) and could be folded in. (raised by xiaocai2333)

internal/storage/sort.go:432 — unreachable x.i < y.i fallback in the heap comparator
After the k-way rewrite, each reader has at most one heap entry — seedNext pushes a single row and returns immediately (sort.go:443-448), and is only called again once that reader's entry has been popped — while less is always invoked on two distinct heap slots (sort.go:249, 266-270). So x.ri != y.ri always holds and the final return x.i < y.i never executes. Suggestion: drop the line or add a comment explaining why it's kept (e.g. for TestRowHeap's custom less), so readers don't infer that a reader can hold multiple heap rows. Related and untested: stability for equal keys previously came from the (ri, i) tie-break and now comes from ri alone, which yields the same order since a reader's equal-key rows are re-seeded in increasing pos. (surfaced during verification)

[bigsheeper]

Thanks — all six verified against the head you reviewed (29cf9ba). Two are fixed in 1cd871b0e9; the branch is also rebased onto latest master, so the commit you reviewed now has a new SHA (49d7fe5051) with unchanged content, and the sort.go line numbers cited below are the ones from 29cf9ba. One is a PR-description correction I have made, and three I am leaving as they are with reasons. One of your suggested fixes is unsafe as written and I want to flag that rather than apply it quietly.

Low — init loop drops a record returned alongside io.EOF (sort.go:461). Confirmed; not fixed, and your first remedy would not be safe.

Confirmed exactly as described: advanceRecord assigns recs[ri] = rec before it inspects the error (sort.go:385-393 — the inline comment even says "assign nil if err"), so a return rec, io.EOF reader leaves recs[i] non-nil while the init loop continues past seedNext(i), and that reader never enters the heap. Agreed it is a contract narrowing rather than a live defect.

Your first suggestion — "check recs[i] != nil and call seedNext(i) before continue" — would introduce a worse bug. On that same path extractKeys(ri) is also skipped (it runs at sort.go:392, reached only after the if err != nil return at :388-390 is passed), so keys[ri] is still the zero value: kind unset, both column pointers nil. Seeding that reader pushes a row whose comparator reads an unpopulated sortKeyCol, compareKeys falls through its switch to return 0 (sort.go:418-420), and every key compares equal — the rows would be emitted in an arbitrary order instead of being dropped. Recovering the record properly would mean calling extractKeys too and tracking "last record, already at EOF" as a separate state, which is real complexity for a case no reader on the path produces.

So I have taken your second option in spirit but not yet in code: the sound fix is to state on the RecordReader interface that the record must be nil whenever a non-nil error is returned. That is record_reader.go, outside the two files this PR touches, and it constrains every implementer — I would rather do it as its own change than smuggle an interface-contract edit into a performance PR. Happy to file it; say the word if you would rather see it here.

Low — inputs that were previously sorted successfully now fail hard (sort.go:537). Confirmed; narrowing is intended; PR description corrected.

Confirmed, and your reading of the old behaviour is right: because the old loop enqueued every eligible row, disorder whose maximum key still lands on the last row ([2,1,3] with endPositions[ri] at the final index) never triggered the early advanceRecord, came out as 1,2,3, and returned success. Those inputs now error.

The narrowing is intended, for the reason you state — the alternative is emitting them silently out of order.

You are also right that the PR description overstated the old behaviour, and I have rewritten that section. It previously read as though the old path depended on sortedness uniformly; it now separates the two classes explicitly: [3,1,2] panicked with index out of range (the #48322 shape), while [2,1,3] succeeded and now does not. Thanks for catching that — it was the description that was wrong, not the code.

Low — consider ErrDataIntegrity for the unsorted-input error (sort.go:538). Confirmed; leaving as ErrStorage.

Agreed on the facts: both codes are retriable == false and both carry SystemError, so this is purely about code semantics, and merr does ask for the more specific code.

I am leaving it, on the counterpoint you raised yourself. The pre-existing WrapErrStorageMsg("unsupported type for sorting key") at sort.go:379 is the sibling failure in the same function, and splitting the two across ErrStorage and ErrDataIntegrity makes the file less predictable than either choice applied consistently. ErrDataIntegrity also carries a "the bytes on disk are corrupt" connotation that points a reader at the wrong layer: per #51981 the likely trigger is the mix_compactor gate routing a [pk]-sorted segment into a [partitionKey, pk] merge, where the binlog is fine and the caller is wrong. If the two are ever unified, I would rather it happen as one pass over the file.

Low — keys lifetime comment states an invariant the code does not rely on (sort.go:361). Confirmed; fixed.

Confirmed by reading the main loop: h.pop() at sort.go:535 removes the reader's only heap entry, and compareWithLast (:537) and saveLast (:540) both read keys[x.ri][fp] afterwards, when that reader has no entry at all. The code is safe for the reason you give — keys[ri] survives until seedNext(ri) at :557 — not for the reason the comment gave.

Reworded to your suggestion, and I added the consequence explicitly so the trap is visible rather than implied: moving compareWithLast or saveLast after seedNext would be a use-after-advance.

Nitpick — four copies of the key-kind dispatch (sort.go:398). Confirmed; not doing it here.

Confirmed, all four sites: extractKeys (373-380), compareKeys (401-418), compareWithLast (499-516), saveLast (524-529). Your point about silent failure modes is the sharp one — a missed case in compareKeys falls through to return 0 and treats keys as equal, and a missed case in the other two disables unsorted-input detection for that kind entirely.

Not folding it in here. As you note, the same duplication exists in Sort (137-156), so doing this properly means one refactor across both functions, and this PR is already 229 lines of rewrite in a default-on compaction path whose correctness argument rests on the merge loop being reviewable. Worth its own change, ideally at the point a third key kind is actually added.

Nitpick — unreachable x.i < y.i in the heap comparator (sort.go:432). Confirmed; fixed.

Confirmed: seedNext pushes one row and returns (443-448) and is called again only after that reader's entry is popped, so x.ri != y.ri always holds and the fallback at :432 never runs.

One correction to the parenthetical: it is not kept for TestRowHeap. That test builds its own rowHeap with its own less including the x.i tie-break (sort_test.go:650-655), and TestRowHeapSingleElement likewise (:672) — both are independent of MergeSort's comparator, and I re-ran them after the change.

So I dropped the branch rather than commenting around it, and stated the invariant plus your stability observation in its place: a reader's equal-key rows are re-seeded in increasing pos, so tie-breaking on ri alone leaves the same output order the (ri, i) tie-break produced.

[czs007]

Summary

Adversarial verification confirmed six issues in this PR — one medium-severity behavior regression where the new hard error rejects input the old code merged correctly with no fallback in the compaction path, plus five low-severity robustness, error-classification, and test-coverage gaps.

---

Medium

internal/storage/sort.go:543 — the new hard error rejects a class of input the old code merged correctly, and the caller has no fallback.

The old enqueueAll pushed every qualifying row of the current record into the priority queue, so the queue re-sorted the whole record; advanceRecord only fired when the popped row was endPositions[ri]. An internally-unsorted record whose maximum key lands on its last row was therefore merged correctly and returned success. For a single reader with record {50, 1, 60}: the old code pops 1, 50, 60 and succeeds; the new code seeds only (0,0)=50, sets saveLast=50, then seeds (0,1)=1, and compareWithLast returns -1 → error. The failure is terminal: mergeSortMultipleSegments errors propagate as return nil, err at internal/datanode/compactor/mix_compactor.go:496-497 with no t.mergeSplit(ctxTimeout) fallback, and the merge-sort decision is deterministic over the same persisted segment flags, so every retry re-enters the same path. The segment is then never compacted again and the delete/TTL entities it carries are never applied. This path is reachable more broadly than the PR comment claims: the gate at mix_compactor.go:481 only checks the upper bound (> MaxSegmentMergeSort, default 30), while the adjacent comment's "only one segment" exclusion is not implemented — preCompact only rejects < 1 (mix_compactor.go:116).

Suggestion: return a distinguishable sentinel (see the ErrDataIntegrity item below) and have the merge-sort branch in mix_compactor.go degrade to mergeSplit when it matches, so a metadata/data mismatch costs one slower compaction instead of a permanently un-compactable segment. Note this is a narrowing of an otherwise net improvement — the wider class of the same metadata inconsistency used to panic (#48322) and now returns an error. (raised by czs007)

---

Low

internal/storage/sort.go:403 — compareKeys / compareWithLast branch only on the x-side kind.

compareKeys switches on cx.kind and then unconditionally reads cy.i64[y.i] / cy.str.Value(...); if the y-side column has a different kind, cy.i64 is nil (index out of range) or cy.str is nil (nil dereference). compareWithLast (sort.go:502-525) has the same shape, and since lastI64/lastStrBuf are shared across all readers, a mixed kind would compare an int64 against a never-written lastI64[fp] and spuriously report "input record is not sorted", landing in the hard-error path above. Not reachable in production: all readers are built from the same plan.GetSchema() and materialized to writerSchema via newMaterializedRecordReader, so a given field has the same arrow type in every reader; extractKeys also rejects unsupported types per reader. The only missing check is cross-reader consistency.

Suggestion: optional hardening — record the first reader's kind in extractKeys and error on mismatch, or add if cx.kind != cy.kind { return error } in both comparators (~3 lines). (raised by xaxys, czs007)

internal/storage/sort.go:467 — a record returned alongside io.EOF is now skipped, and recs[ri] is left non-nil, contradicting the file's own sentinel comment.

The old init loop ran advanceRecord for all readers (continuing on EOF) and then enqueued every non-nil entry, so a record delivered together with io.EOF was consumed; sort.go:467-476 now continues and never calls seedNext(i), dropping that record. This is unreachable in production — materializedRecordReader.Next returns nil, err on any error (record_materializer.go:305-308), as does timestampOverwriteReader.Next (timestamp_overwrite.go:79-82). The more actionable point is the invariant: advanceRecord's comment says // assign nil if err (sort.go:390) but it assigns whatever rec it got, while sort.go:360-361 declares recs[ri] == nil to be the sole exhausted-reader sentinel. In that state keys[ri] is never filled by extractKeys (zero value: kind=keyInt64, i64=nil), so any future path calling seedNext for that reader would push a rowIndex with nil keys and panic in compareKeys. Safety today rests on the implicit fact that such a reader can never obtain a heap entry, not on the documented sentinel.

Suggestion: one line — if err != nil { recs[ri] = nil; return err } — which also makes the behavior match the comment. (raised by xaxys, czs007)

internal/storage/sort.go:544 — unsorted input should use ErrDataIntegrity, not ErrStorage.

The merr sentinel definitions give explicit guidance here: ErrDataIntegrity covers "bytes already on disk don't match the layout we expect from the schema … likely permanent corruption; retry won't help", which is exactly the case of a segment flagged IsSorted whose binlogs are not sorted by the merge key. ErrStorage documents itself as the non-IO / non-serde / non-client-input fallback and says "Prefer ErrIo*/ErrSerializationFailed/ErrDataIntegrity/ErrParameterInvalid when they fit". merr.WrapErrDataIntegrityMsg already exists (pkg/util/merr/utils.go:1089), so this is a one-line change — and it is the prerequisite for the mergeSplit degradation proposed above (errors.Is(err, merr.ErrDataIntegrity)).

Suggestion: switch the wrapper, and replace the substring assertion at internal/storage/sort_test.go:701 with an errors.Is / merr.Code assertion so a message reword cannot silently disable the test. (raised by xaxys, czs007)

internal/storage/sort_test.go:99 — no test reader invalidates the previous record on Next().

The RecordReader contract states a record is "valid until the next Next or Close" (internal/storage/record_reader.go:19-24), and production readers enforce it: materializedRecordReader.Next calls cleanupMaterializedRecord(r.current) and timestampOverwriteReader.Next calls r.last.Release() before fetching. Both test readers (sliceRecordReader, oneShotRecordReader) hold every record until the test ends, so no case exercises use-after-advance. This matters more after the rewrite: keys[ri] is now a long-lived view directly into arrow buffers (Int64Values() slices, *array.String pointers), the ordering constraint that compareWithLast/saveLast must read before seedNext is argued only in the comment at sort.go:359-365, and lastStrBuf relies on per-row copying to survive an advance. None of these three assumptions is pinned by a test.

Suggestion: add a reader that poisons or releases the previous record's buffers on Next() (even just overwriting the backing array with sentinel values) and run the TestMergeSortVarcharKey scenario against it, so an ordering regression fails a test rather than requiring careful reading. (raised by czs007)

internal/storage/sort_test.go:683 — the new unsorted-input test only covers a shape the old code also failed on; the deliberate behavior narrowing has no test.

TestMergeSortUnsortedInputReturnsError uses r0 = [{50,60,1}, {70}] — minimum key on the last row followed by a shorter record — which is precisely the endPositions shape the PR body identifies as the #48322 index-out-of-range panic. The test therefore only demonstrates that previously-crashing input now errors cleanly, which is a net improvement. The behavior change the PR body explicitly calls out in its own section — disorder whose maximum key still lands on the last row (e.g. [2,1,3]), which the old code emitted correctly and returned success for and which now errors — has no test at all. The change most in need of reviewer sign-off is invisible in the suite, leaving no baseline for anyone who later degrades it to mergeSplit or decides to allow such input again.

Suggestion: add a case pinning the current decision (single reader, record {50, 1, 60}, all-true predicate, asserting ErrDataIntegrity) with a comment noting that the old implementation emitted 1, 50, 60 successfully and that erroring is intentional. (raised by czs007)

[sre-ci-robot]

✅ CI Loop Results 1cd871b

Stage	Result	Duration	Tests
✅ Build	SUCCESS	11.8min	-
✅ Code-Check	SUCCESS	8.1min	-
✅ UT-Integration	SUCCESS	24.9min	-
✅ UT-GO	SUCCESS	22.2min	-
✅ UT-CPP-Cov	SUCCESS	39.8min	8499 total, 8499 passed, 0 failed

Total: 80min | Pipeline | Artifacts

Overall Coverage: 73.6%
Diff Coverage: Go 92.8% (141 hit, 11 miss, 152 measurable lines, 83 unmeasured)
Diff Coverage HTML: view changed lines
Total Patch Coverage: 92.8% (141/152 measurable lines, 83 unmeasured)

[xiaofan-luan]

I think one production-safety issue should be addressed before this merges.

The new k-way MergeSort correctly relies on every reader being sorted by sortedByFieldIDs, but the production gate currently checks only:

segment.GetIsSorted() || segment.GetIsSortedByNamespace()

The actual merge key is [pk] or [partitionKey, pk] depending on namespaceEnabled. This means, for example, that a segment marked IsSorted=true (sorted only by PK) can still enter MergeSort after namespace is enabled, even though the merge key is now [partitionKey, pk].

With this PR, such input reaches the monotonicity check and returns an error. mixCompactor.Compact() then returns the error directly without falling back to mergeSplit, so subsequent compaction attempts over the same persisted segment flags will deterministically take the same failing path.

Suggested minimal fix before merge:

• When namespace is enabled, require every input segment to have IsSortedByNamespace=true.
• Otherwise, require every input segment to have IsSorted=true.
• If the required flag does not match, select mergeSplit before opening/writing the merge-sort output.
• Add a targeted test for the mismatched-flag case, including a single input segment since the current gate permits one.

This keeps the optimization unchanged for valid sorted inputs while preventing a metadata/order mismatch from making a segment uncompactionable. The remaining lower-severity robustness and test-coverage items can be handled as follow-ups.

[czs007]

Must-fix issues introduced by this PR: 0
Merge recommendation: Mergeable as-is — this review found no must-fix issue introduced by this PR.

Re-review of 1cd871b0e951

The commit is unchanged since our last comment; below is an adjudication of every earlier finding, followed by this round's findings.

• internal/storage/sort.go:543 — new hard error turns previously-mergeable input into a permanent compaction failure with no fallback: Confirmed. Both reviewers reproduced the behavior change and confirmed the error propagates straight out of mix_compactor.go without falling back to mergeSplit; see the medium finding below for the corrected reachability argument.
• internal/storage/sort.go:403 — compareKeys / compareWithLast branch only on the x-side kind: Confirmed, but narrowed: the old implementation panicked on the same input via an unchecked type assertion, so this is a change of panic shape, not a new defect. Re-stated below.
• internal/storage/sort.go:467 — a record returned alongside io.EOF is skipped and recs[ri] is left non-nil: Confirmed. The init loop continues on EOF and never calls seedNext, dropping that record's rows silently.
• internal/storage/sort.go:544 — unsorted input should use ErrDataIntegrity rather than ErrStorage: Confirmed. ErrStorage's own doc comment asks callers to prefer ErrDataIntegrity when it fits, and it fits here.
• internal/storage/sort_test.go:99 — missing a test where the reader invalidates the previous record on the next Next(): Confirmed, with one correction to our earlier wording: TestMergeSort does use CompositeBinlogRecordReader, which does release the previous record (record_reader.go:416), so "no test reader invalidates" was too strong. The real gap — the varchar path — stands.
• internal/storage/sort_test.go:683 — the new unsorted case only covers a shape the old code also failed on: Confirmed. (Our earlier comment cited line 829, which does not exist; the file is 702 lines and the test is at 683–702.)

---

Medium

internal/storage/sort.go:543 — unsorted-input hard error makes previously-mergeable input a permanent compaction failure with no fallback path.
The new compareWithLast guard rejects a class of input the old implementation merged correctly, and nothing downstream degrades gracefully. Failure scenario: a segment with IsSorted=true whose binlog record is not actually sorted by the merge key but whose maximum key happens to land on the last row (minimal repro: single reader, pk column {2, 1, 3}, sortByFieldIDs=[common.RowIDField]) → MergeSort returns numRows=0 and "input record is not sorted by the merge key"; merge_sort.go:135-139 closes the writer and returns the error, mix_compactor.go:495-497 re-raises it and cannot reach the mergeSplit fallback (the else branch is unreachable when err != nil), so the compaction task fails and fails identically on every retry. Measured: parent commit 422f2e853f returns n=3, output [1 2 3], err=nil; head 1cd871b0e9 returns n=0 with the error.

One correction to our earlier reasoning: we previously claimed the namespace path (services.go:241-247 sets sortFields=[partitionKey, pk] while the gate at mix_compactor.go:475 only checks IsSorted || IsSortedByNamespace) made this "reachable in real deployments." That argument runs the wrong way — when an IsSorted-only segment is mixed in, its record's max (partitionKey, pk) generally does not land on the last row, so the old implementation took the stale-heap branch and panicked ({50,60,1}+{70} on 422f2e853f gives panic: runtime error: index out of range [1] with length 1). In the namespace case this PR replaces a panic with an error, which is an improvement. The genuine regression class is the narrower "record internally unsorted but max key lands on the last row" shape.

Suggestion: this error represents an expected input shape rather than a Milvus bug. Make it a sentinel that mix_compactor.go can detect and fall back to mergeSplit on, rather than failing compaction permanently — at minimum it should be distinguishable by operators. (raised by tinswzy, xaxys)

Low

internal/storage/sort.go:544 — sort-validation failure should use ErrDataIntegrity, not ErrStorage.
This line uses merr.WrapErrStorageMsg. ErrDataIntegrity (code 1009, errors.go:167-173) documents exactly this case — "bytes already on disk … don't match the layout we expect from the schema … Likely permanent corruption; retry won't help" — while ErrStorage (code 1008, errors.go:176-180) says "reach for ErrStorage only when none of those describe the failure." A segment marked IsSorted whose binlog is not sorted by the merge key is on-disk data contradicting its metadata. Both are non-retriable, so the impact is classification and observability: operators keying on the code cannot distinguish data corruption from a storage-internal error. Suggestion: WrapErrDataIntegrityMsg already exists at utils.go:1089 — a one-line change. (Note that extractKeys's "unsupported type for sorting key" at sort.go:382 is also WrapErrStorageMsg, but that classification was carried over unchanged and is not introduced here.) (raised by tinswzy, xaxys)

internal/storage/sort.go:467 — a record returned alongside io.EOF is silently dropped in both the init loop and seedNext, and recs[ri] is left non-nil, breaking the exhausted-reader sentinel.
advanceRecord (sort.go:388-396) assigns recs[ri] = rec before returning the error; the init loop (467-473) continues on io.EOF and never calls seedNext for that reader, and seedNext itself (457-462) returns nil on io.EOF. Failure scenario: any RecordReader returning (non-nil record, io.EOF) on its final batch — the standard io.Reader convention, and not prohibited by the interface comment at record_reader.go:294-298 → every row of that record is discarded and MergeSort returns numRows=0, err=nil, giving the caller no error signal (measured: head returns n=0, got=[]; parent returns n=3, got=[1 2 3]). The current production reader chain does not produce this shape (materializedRecordReader.Next returns nil, err on any error), so this is a contract-level data-loss hazard rather than a live one. Leaving recs[ri] non-nil also contradicts the comment at sort.go:359-365 declaring recs[ri] == nil the sole exhausted-reader sentinel. Suggestion: move the io.EOF check after the rec == nil check in both places. (raised by tinswzy, xaxys)

internal/storage/sort_test.go:99 — no test exercises a reader that invalidates the previous record on the next Next(), and the varchar path has no coverage at all.
Production readers do release the previous record on Next() (materializedRecordReader.Next:301-303 calls cleanupMaterializedRecord, timestampOverwriteReader.Next:75-78 calls r.last.Release(), CompositeBinlogRecordReader.Next:417 calls releaseCurrent), but the PR's new sliceRecordReader (sort_test.go:96-105) and the existing oneShotRecordReader hold their records forever. The only test driving a real reader, TestMergeSort (:276-285), uses an Int64 key (common.RowIDField); TestMergeSortVarcharKey (:535) and BenchmarkMergeSortVarcharKey both use sliceRecordReader. So the lifetime constraint that keys[ri].str points directly into an arrow buffer — the one sort.go:359-365 calls out with "Moving either of those after seedNext would be a use-after-advance" — has no invalidating reader behind it on the varchar path. We verified the current ordering is correct (pop → compareWithLast → saveLast → rb.Append → pos++ → seedNext, sort.go:540-565), so this is a regression-coverage gap, not a live defect. Suggestion: add a varchar merge-sort test driven by a reader that releases the previous record. (raised by tinswzy, xaxys)

internal/storage/sort_test.go:683 — the new unsorted-input case only covers a shape the old code also failed on; the behavior narrowing the PR itself acknowledges is untested.
TestMergeSortUnsortedInputReturnsError uses {50, 60, 1} followed by {70} — the #48322 shape. Running that same input on parent commit 422f2e853f gives panic: runtime error: index out of range [1] with length 1 (the old implementation advanced early after popping the minimum key, leaving a stale heap entry pointing at a replaced record). The case therefore only proves that input which already crashed now returns an error, and gives zero coverage of the deliberate narrowing the PR body devotes a section to: the {2,1,3} shape (old implementation succeeds with 1,2,3; new code errors at sort.go:543 — both measured) has no case at all. That is the one genuinely contentious behavior surface in this change and the one most worth pinning down. Suggestion: add a case asserting {2,1,3} now returns an error, so anyone later touching compareWithLast sees the semantics were chosen deliberately. Separately, compareWithLast's keyString branch has only positive coverage (TestMergeSortVarcharKey inputs are all correctly ordered) — a varchar out-of-order case would close that gap. (raised by tinswzy, xaxys)

(Note: our earlier comment on this item cited sort_test.go:829, which does not exist — the file is 702 lines and the test lives at 683–702.)

---

Disputed — our reviewers disagree

• internal/storage/sort.go:404 — compareKeys / compareWithLast branch only on cx.kind while unconditionally dereferencing the y-side column, so mismatched key types across readers panic. compareKeys (sort.go:401-421) switches on cx.kind but reads cy.i64[y.i] / cy.str.Value(...) regardless: if x is Int64 and y is String, cy.i64 is nil → index out of range; the reverse calls Value on a nil *array.String → nil dereference. compareWithLast (502-525) and saveLast (527-538) have the same shape, and lastI64/lastStrBuf do not record the previous key's kind, so a type change across records compares against a stale buffer. extractKeys (373-386) only validates types within a single record, never across readers. Verification wanted to drop it on provenance grounds: these lines are 100% new in this PR (the old comparator closures were deleted wholesale), but the old implementation had an unchecked recs[y.ri].Column(fid).(*array.Int64) assertion at the same position that panics on identical input, so "caused by this PR" is arguable either way — the panic shape changed, the severity did not, and all production readers share plan.GetSchema(), making the field's arrow type identical across readers with no real trigger path. (raised by tinswzy, xaxys; drop refuted during independent cross-model verification)

This item is flagged for the author to adjudicate, not filed as a required fix.

[bigsheeper]

Thanks — taking the ErrDataIntegrity item, declining the mergeSplit degradation. Reasons for both, and one correction to the record. Head is now 983118dc71.

Taken: ErrDataIntegrity (sort.go:544). Your reading holds. I verified the equivalence rather than assuming it: both codes are retriable == false (errors.go:173,180), both are SystemError (ErrStorage sets it explicitly, ErrDataIntegrity takes the zero value and SystemError ErrorType = 0 at errors.go:35), and neither appears in the oldCode mapping, so old SDKs see no change. A repo-wide grep found no errors.Is(..., merr.ErrStorage) guard anywhere. So the swap changes the code and the message and nothing else.

One caveat worth recording: ErrDataIntegrity's enumerated scope is layout-shaped ("bytes on disk don't match the layout we expect from the schema"), while this failure is bytes contradicting a metadata flag. The category is right and no better sentinel exists, but the sentinel's doc comment could stand widening — I left that alone rather than touch merr in this PR.

Not taken: degrading to mergeSplit when the error fires. Three reasons.

First, this is not a regression against the prior behavior. The old path panicked on general disorder — index out of range from stale heap entries after an early advanceRecord. A panic is also a stall, and a worse one. The only class that genuinely worked before is disorder whose maximum key lands on the last row, and it worked by implementation accident: the old loop enqueued the whole record and the heap re-sorted it. That is not a contract I want to preserve.

Second, the trigger is a segment whose metadata says sorted and whose bytes are not — a should-not-happen class (a sorter bug, storage corruption, memory corruption). Degrading silently to mergeSplit would hide exactly the thing an operator needs to see. Failing loudly is the intent; what was missing was the ability to act on it, which is what the diagnosability work in this PR addresses.

Third, the degradation has a wrinkle the suggestion doesn't cover. merge_sort.go:46-47 and mergeSplit (mix_compactor.go:212-213) each build allocators from the same PreAllocatedSegmentIDs/PreAllocatedLogIDs range, both starting at Begin. If MergeSort has already flushed output — which happens once >= BinLogMaxSize (64 MB default) has been emitted before the inversion — the fallback rewrites the same object keys and leaves unreferenced binlogs behind. Not fatal, but it needs handling that the one-line errors.Is version doesn't have.

Instead, I took the other half. The gate now checks the flag matching the plan's merge key rather than either flag, which closes the metadata-expressible mismatch at zero cost and before any rows are read. And the error is now actionable: reader N record M row K, with mergeSortMultipleSegments logging the reader-index to segment-ID mapping on the same line. Verified end to end — I corrupted one segment's ordering while leaving its IsSorted flag true and confirmed the reported reader index resolved to that segment.

One correction. Your Medium argued the path is "reachable more broadly than the PR comment claims", citing the single-segment gate. That part is right and I fixed the stale comment. But neither of us established why a sorted-flagged segment would carry unsorted data in the first place — and the PR body's answer to that (the namespace toggle) turns out not to hold: EnableNamespace is create-only, so the flag and the merge key cannot disagree today. I have corrected the PR body accordingly. This work is defense-in-depth, and I have stopped it from claiming otherwise.

[bigsheeper]

@xiaofan-luan thanks — implemented, with one correction to the mechanism. Head is now 983118dc71.

Implemented. canMergeSort now requires IsSortedByNamespace when the schema has namespace enabled and IsSorted when it does not, instead of accepting either. A mismatch selects mergeSplit before any segment rows are read, as you asked. Tests cover all four cells of the truth table plus a mixed-flag plan, and the single-segment case you flagged.

Your single-segment observation was right, and it exposed a comment/code disagreement: mix_compactor.go claimed merge sort excludes "only one segment or too many segments", but only the upper bound was ever implemented — preCompact rejects fewer than 1. I fixed the comment rather than adding the lower bound, because single-segment merge sort is the better path: it keeps the output flagged sorted, whereas mergeSplit emits it unsorted and costs a follow-up sort compaction. There is now a test pinning that.

The trigger you described is not reachable, though. You wrote that an IsSorted=true segment "can still enter MergeSort after namespace is enabled". I went looking for that path and could not find it: EnableNamespace is written only at collection creation (ddl_callbacks_create_collection.go:217). All four paths that produce a FieldMaskCollectionSchema update — add_field.go:73, add_struct_field.go:70, alter_collection_field.go:113, alter_collection_properties.go:181 — build the snapshot via coll.ToCollectionSchemaPB(), which copies the stored value (collection.go:163), making the assignment at collection.go:216 an identity. Since sort_compaction.go:373-374 derives both flags from that same setting (isSorted := !isNamespaceSorted), a segment's flag and the plan's merge key cannot disagree.

That does not make the change pointless — the gate was testing the wrong proposition, and it was safe by coincidence rather than by construction — but it does mean this is defense-in-depth rather than a fix for something users are hitting. The PR body claimed otherwise and I have corrected it. For the same reason I did not open a [Bug] issue: there is no reachable bug to report.

Also worth flagging: the PR body attributed #48322 to this gate. That was wrong. #48322 was an arrow-go use-after-free on STRUCT array deallocation, fixed by milvus-io/arrow#3 and #48775, verified on master-20260429.

On the deeper failure mode — a segment whose flag matches but whose bytes are not actually ordered — the gate cannot see that, and I deliberately did not add a mergeSplit fallback for it (czs007 proposed one; reasons in the other thread). It stays a loud, deterministic failure. What I did add is the ability to act on it: the error now reads reader N record M row K and the compactor logs the reader-index to segment-ID mapping, so the offending segment is identifiable from one log line.

[czs007]

Must-fix issues introduced by this PR: 0
Merge recommendation: Mergeable as-is — this review found no must-fix issue introduced by this PR.

Adversarial review found no issues requiring changes.

Verified:

• internal/datanode/compactor/mix_compactor.go:576 — merge-sort eligibility checks each segment's sorted flag against the merge key actually selected by EnableNamespace, so mismatched, unsorted, or over-limit plans fall back to mergeSplit.
• internal/storage/sort.go:434 — the heap comparator plus the seedNext invariant keep at most one live entry per reader, giving a correct k-way merge that neither skips nor duplicates rows.
• internal/storage/sort.go:454 — pos advances monotonically through filtered and emitted rows, so the predicate runs exactly once per input row, including across empty or fully filtered records.
• internal/storage/sort.go:535 — Arrow-backed key views stay valid until their reader advances; compareWithLast/saveLast run before seedNext, and varchar keys needed across advances are copied into reusable owned buffers.
• internal/storage/sort.go:552 — the monotonicity check rejects strictly decreasing output before the row is appended and reports the actual reader, record, row, and merge-key fields; equal keys compare == 0, so duplicate primary keys are not misflagged.
• internal/datanode/compactor/merge_sort.go:136 — reader indices in integrity errors map back to segment IDs without altering reader ordering, and the existing writer-close error path is unchanged.

[sre-ci-robot]

❌ CI Loop Results 983118d

Stage	Result	Duration	Tests
✅ Build	SUCCESS	14.8min	-
✅ Code-Check	SUCCESS	9.2min	-
✅ UT-Integration	SUCCESS	26.4min	-
❌ UT-GO	FAILURE	23.9min	-
✅ UT-CPP-Cov	SUCCESS	56.3min	8629 total, 8629 passed, 0 failed

Total: 79min | Pipeline | Artifacts

Failed Test Logs:

• UT-GO: view log

[bigsheeper]

/ci-rerun-build-ut-cov

[sre-ci-robot]

❌ CI Loop Results 983118d

Stage	Result	Duration	Tests
✅ Build	SUCCESS	17.8min	-
✅ Code-Check	SUCCESS	9.9min	-
✅ UT-Integration	SUCCESS	25.7min	-
❌ UT-GO	FAILURE	23.6min	-
✅ UT-CPP-Cov	SUCCESS	63.0min	8629 total, 8629 passed, 0 failed

Total: 86min | Pipeline | Artifacts

Failed Test Logs:

• UT-GO: view log

[sre-ci-robot]

✅ CI Loop Results ae61298

Stage	Result	Duration	Tests
✅ Build	SUCCESS	11.9min	-
✅ Code-Check	SUCCESS	8.8min	-
✅ UT-Integration	SUCCESS	26.0min	-
✅ UT-GO	SUCCESS	22.5min	-
✅ UT-CPP-Cov	SUCCESS	44.4min	8629 total, 8629 passed, 0 failed

Total: 79min | Pipeline | Artifacts

Overall Coverage: 74.0%
Diff Coverage: Go 84.8% (167 hit, 30 miss, 197 measurable lines, 100 unmeasured)
Diff Coverage HTML: view changed lines
Total Patch Coverage: 84.8% (167/197 measurable lines, 100 unmeasured)

[czs007]

/lgtm

[sre-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bigsheeper, czs007

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• internal/datanode/OWNERS [czs007]
• internal/storage/OWNERS [czs007]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment