fix: support legacy tombstone abort columns - #26889
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
37cb5ce to
5eca11c
Compare
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review — request changes
Reviewed exact head 5eca11c27462f4ae2064638d4f2cd8e8ab96c582.
P1 — Normalize legacy abort-column compatibility at the shared read boundary
The fix only guards IsRowDeletedByLocation and EvalDeleteMaskFromDNCreatedTombstones. Other production consumers of the same ReadDeletes result still do !IsConstNull() followed by raw aborts[i] indexing, including local_disttae_datasource.go and logtailreplay/partition_state.go. With the issue's observed shape (rowids=N, ordinary abort vector length 0), those paths still panic.
Please normalize the supported legacy representation in ReadDeletes/the object decode boundary (or provide one shared validated accessor), so every consumer receives the same contract. Add an end-to-end object-reader regression and exercise at least one datasource/statistics consumer.
P2 — Do not treat every length mismatch as legacy
abortVec.Length() == len(rowids) is used as the only validity check; every other non-null length, including a truncated or oversized abort vector, is silently treated as “no aborts”. For example, rowids=3 and aborts=2 causes an aborted tombstone to be applied as a live delete, producing incorrect snapshot visibility. The object writer currently only logs unmatched column lengths and still persists the object, so this is reachable from an internally malformed object.
Only an explicitly supported legacy shape should mean “no abort metadata”; other mismatches, wrong types, and null abort values should fail/propagate an error. Extend tests beyond a hand-built empty vector to cover the real read path, partial mismatch, const vectors, and both modified entry points.
Focused package tests, build, vet, and diff checks pass locally, but they do not cover these cross-consumer and malformed-layout cases.
|
Addressed the deep-review requests in 6993c2f (rebased/merged with upstream main b992c9e):
Related issue: #26887 |
|
Addressed the legacy-reader compatibility review: the object reader synthesizes a bool const-null abort vector with the block row count when SEQNUM_ABORT is absent. The shared validator now accepts only that exact sentinel; empty non-const, mismatched, wrong-type, and row-level null abort vectors still fail. Updated real ReadDeletes regression to assert the const-null marker, fixed the tombstone writer test layout, and verified locally with focused objectio/ioutil and logtailreplay tests, go vet, and go build. Merged upstream main at 6042ca6. Latest commit: 42572a7 |
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
The prior Codex const-null blocker is fixed: the reader’s legacy marker is now accepted. XuPeng-SH’s shared-boundary and malformed-layout topics are addressed and not duplicated. One distinct blocker remains: merge-based tombstone statistics can silently drop an initial validation error and undercount deletions.
P1 - Propagate initial tombstone iterator errors (pkg/vm/engine/disttae/logtailreplay/partition_state.go:1999)
loadNextBlock now records ReadDeletes/abort-validation errors in it.err and returns false. The initial if it.next() appends the iterator only when it returns true, so an error on the first block discards the iterator before the later it.err check. The merge path then returns success with too few tombstones; CountRows/StarCount can overcount visible rows. This is distinct from the earlier validation-policy finding: the layouts may be rejected, but this consumer must propagate the rejection. Check it.err when the initial next() is false and release any held resources.
|
Fixed the remaining merge-statistics error propagation issue in e863c80: if the first tombstone iterator block fails ReadDeletes or abort validation, countTombstoneStatsWithMerge now releases all acquired iterators and returns the error instead of silently dropping that object. Added a regression test for an initial malformed abort-column read. |
gouhongshen
left a comment
gouhongshen
left a comment
There was a problem hiding this comment.
Codex automated review
Prior const-null compatibility, shared validation, malformed-layout, and initial merge-error blockers are fixed. No current findings; focused tests were locally blocked by missing native headers.
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed exact head 34e7b5a. All prior blockers are closed at the shared ReadDeletes contract: the real legacy const-null sentinel is accepted, malformed persisted layouts fail closed, every consumer uses the validated accessor, and initial merge-iterator errors are propagated with cleanup. Focused objectio/ioutil and logtailreplay regressions pass locally; no blocking finding.
|
Tick the box to add this pull request to the merge queue (same as
|
4 participants
Summary
IsRowDeletedByLocationcompatible with both legacy and current tombstone formats.Fixes #26887
Validation
GOWORK=off go list -mod=readonly ./pkg/objectio/ioutilpassed.go build/go vetand the focused test were blocked locally because the repository CGo/native prerequisites (usearch.h,roaring.h,xxhash.h,cgo/libmo.so) are unavailable; the standard thirdparty build was started but stopped after the onnxruntime download stalled.