test: cover physical primary seqnum range coalescing

[jiangxinmeng1]

What type of PR is this?

• API-change
• BUG
• Improvement
• Documentation
• Feature
• Test and CI
• Code Refactoring

Which issue(s) this PR fixes:

None. The previously linked #27383 concerns a MemCache allocator leak and is unrelated, so it has been removed.

What this PR does / why we need it:

After review, the proposed logical primary-index handoff was withdrawn. PartitionState ingestion and persisted-object normalization both preserve user columns in stable physical sequence-number order; no producer on this path remaps a batch into the current logical catalog order. Range net-effect coalescing must therefore continue to locate the data primary key by primarySeqnum.

The regression now exercises the real producer contracts with divergent ordinals (logical Idx=1, physical SeqNum=3):

• protobuf logtail insert/delete ingestion through PartitionState and NewChangesHandlerWithPartitionStateRange;
• persisted appendable-object special-column normalization through updateDataBatch;
• a preserve-all-versions control that first proves one data row and one tombstone reach the coalescer before normal net-effect filtering cancels them.

The earlier synthetic logical-layout tests and the test-only quickNextWith seam were removed.

Verification

• focused TestRangeCoalescingUsesPhysicalPrimarySeqnum
• controlled-CGo full tests for ./pkg/vm/engine/disttae/logtailreplay and ./pkg/vm/engine/disttae
• go build and go vet for both owning/dependent packages
• scoped git diff --check against current origin/main

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[XuPeng-SH]

Review summary

Reviewed exact head 8e79273a6a1b9994c472e8c2c1931b38f686c2cb. The affected package suites and vet pass, but the new index handoff violates the real PartitionState batch contract and can silently preserve operations that should cancel. A temporary exact-head counterexample reproduces it.

[P1, blocking] primaryIdx indexes the wrong column for partition-state batches

PartitionState.HandleRowsInsert reads the PK from batch.Vecs[2+primarySeqnum] and stores that batch unchanged (partition_state.go:755-779). fillInInsertBatch later removes only the rowid/TS prefix and preserves the remaining column order (change_handle.go:2597-2613). Therefore the data batch passed to filterBatch still has its PK at primarySeqnum, not at the current catalog's logical primaryIdx.

This PR changes the snapshot range path to pass h.primaryIdx and uses it for every net-effect merge (change_handle.go:483-492, logtailreplay/change_handle.go:2174-2179). When schema evolution makes the two values differ, filterBatch groups data rows by an unrelated column while tombstones are grouped by their actual PK. Insert/delete pairs no longer cancel, so branch diff/merge can return silently incorrect changes.

I reproduced this through a real PartitionState row layout: physical PK seqnum 3, current logical index 1, insert (PK=999, column-at-index-1=222) at TS 10, then delete PK=999 at TS 20. The range net effect must be empty; exact head returns one data row and one tombstone. Using seqnum 3 for the same handler produces the correct empty result.

Please make the PK ordinal a property of each emitted batch (or normalize every producer to one documented logical schema before filtering), then cover actual partition-state ingestion and persisted-object batches across schema evolution. The current synthetic tests construct [added-first, logical-PK, TS] directly, so they assume the proposed layout instead of exercising the producer contract.

[P2] The linked issue is unrelated

The PR says it fixes #27383, but #27383 is the TestMemoryFS / MemCache allocator leak. Please link the actual range-net-effects issue and describe the failing schema-evolution scenario so the intended contract is reviewable.

[gouhongshen]

Codex automated review

Revalidated exact head 8e79273. Affected package suites pass, but the pending blocking finding (XuPeng-SH's review, still unaddressed) is independently reconfirmed by a fresh exact-head counterexample driven through the real PartitionState ingestion path; the approving review's premise ('batches emitted on the snapshot range path are in the logical schema') is disproven by the code. Evidence chain: (1) PartitionState.HandleRowsInsert indexes every row by batch.Vecs[2+primarySeqnum] (partition_state.go:761) — that is the ingestion contract; a stored batch whose PK sits elsewhere would corrupt the partition state's own primary index. (2) fillInInsertBatch (logtailreplay/change_handle.go:2602) strips only rowid/TS and preserves stored column order, and the object paths (updateDataBatch/updateCNDataBatch, AObjectHandle/CNObjectHandle) emit the loaded physical layout unchanged — nothing on this path remaps to a logical schema. (3) The normal replay path (NewChangesHandler, change_handle.go:2004) passes primarySeqnum to filterBatch over the identical base handlers and batches, and the same handler's object zonemap pruning addresses the PK by primarySeqnum (change_handle.go:559). (4) The regular read path documents the layout '[0]=rowid, [1]=commitTS, [2+seq]=user columns' (local_disttae_datasource.go:892-896). My exact-head repro: partition state built via HandleRowsInsert/HandleRowsDelete with physical PK seqnum 3 and logical primaryIdx 1; insert (PK=999, 222 at column index 1) at TS 10 plus delete PK=999 at TS 20 must net to empty; the range handler with primaryIdx=1 returns one data row and one tombstone, while ordinal 3 returns empty. The divergence is not hypothetical: TN's catalog shifts logical Idx while SeqNum stays stable (positioned AlterKind_AddColumn/DropColumn in tae/catalog/schema.go), and upgraded clusters carry catalog tables with divergent att_num/att_seqnum after positioned ADD COLUMN upgrades under stable table IDs (v1_2_0 mo_indexes 'after type', v4_0_6 mo_foreign_keys). The PR's new tests synthesize [added-first, logical-PK, TS] batches directly, assuming the proposed layout instead of exercising the producer contract. The quickNextWith split and the constructor split in disttae/change_handle.go are behavior-preserving; no concurrency or resource-lifecycle issues found. Linked issue #27383 is unrelated (TestMemoryFS/MemCache flake), so the intended failing scenario remains undocumented.

P1 - Range net effects filter by logical primaryIdx while partition-state batches carry the PK at the physical seqnum position — insert/delete pairs silently stop cancelling (pkg/vm/engine/disttae/logtailreplay/change_handle.go:2174)

Revalidated the pending blocking finding at exact head 8e79273 with an independent counterexample through the real ingestion path (HandleRowsInsert/HandleRowsDelete with proto batches), not synthetic filterBatch inputs. Contract: PartitionState.HandleRowsInsert reads the PK from batch.Vecs[2+primarySeqnum] (logtailreplay/partition_state.go:761) and indexes every row by it; fillInInsertBatch (change_handle.go:2602) then removes only the rowid/TS vectors and preserves the remaining column order, so in-memory rows are emitted with the PK at primarySeqnum. Object batches keep their loaded physical layout (updateDataBatch/updateCNDataBatch), and this same handler prunes object blocks via the PK zonemap at primarySeqnum (change_handle.go:559). The normal replay path (NewChangesHandler, change_handle.go:2004-2005) feeds primarySeqnum to filterBatch over the identical base handlers and batches, and the regular read path documents '[2+seq]=user columns' (disttae/local_disttae_datasource.go:892-896). This PR switches only the snapshot range path (disttae/change_handle.go:483-492) to h.primaryIdx, so whenever the two ordinals differ, filterBatch (dataPKIdx := primaryIdx, change_handle.go:2174) groups data rows by a non-PK column while tombstones stay grouped by the real PK; pairs no longer cancel and branch diff/merge can return silently incorrect changes. Exact-head repro: physical PK seqnum 3, logical primaryIdx 1, insert (PK=999, 222 at index 1) at TS 10, delete PK=999 at TS 20 -> NewChangesHandlerWithPartitionStateRangeAndPrimaryIdx(..., 3, 1, ...) returns one data row and one tombstone; the same handler with ordinal 3 correctly returns empty. The divergence is reachable: TN's catalog shifts logical Idx while keeping SeqNum stable (AlterKind_AddColumn with InsertPosition and AlterKind_DropColumn in tae/catalog/schema.go), and upgraded clusters hold catalog tables whose att_num/att_seqnum diverge after positioned ADD COLUMN under stable table IDs (v1_2_0 mo_indexes 'after type', v4_0_6 mo_foreign_keys). Please make the PK ordinal a property of each emitted batch (or normalize every producer to one documented schema before filtering) and cover real partition-state ingestion and persisted-object batches across schema evolution; absent a producer that emits a logical layout on this path, the prior seqnum behavior is the correct one.

P2 - Linked issue #27383 is unrelated (TestMemoryFS flake); the failing schema-evolution scenario is undocumented (pkg/vm/engine/disttae/change_handle.go:128)

The PR body says 'issue #27383', but #27383 is '[Bug]: TestMemoryFS flaky — missing free in MemCache bytes allocator', which has no connection to range net effects or primary indexes (verified via the issue body/labels). Without the real issue linked, the intended contract — which producer is supposed to emit a logical-schema batch on the snapshot range path, and in which scenario primaryIdx differs from primarySeqnum — is not reviewable. Please link the actual issue and describe the failing scenario (including how the affected table's att_num/att_seqnum diverge), or state explicitly if no such production scenario exists.