Skip to content

feat: add experimental metadata-only ADD for nullable columns - #30816

Open
VioletQwQ-0 wants to merge 23 commits into
matrixorigin:mainfrom
VioletQwQ-0:codex/m0-restricted-experimental-draft
Open

VioletQwQ-0 wants to merge 23 commits into
matrixorigin:mainfrom
VioletQwQ-0:codex/m0-restricted-experimental-draft

Conversation

@VioletQwQ-0

@VioletQwQ-0 VioletQwQ-0 commented Oct 22, 2026 •

Copy link
Copy Markdown
Collaborator

What type of PR is this?

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

Which issue(s) this PR fixes:

Related to #30518, the SF100 BranchBench parent tracker, which must remain open.
This PR implements only the restricted metadata-only ADD follow-up. #29798 is
the merged historical comparison baseline. #30958 is the independent BOOLEAN
DEFAULT false extension, not part of this PR or its acceptance claim.

What this PR does / why we need it:

Current Architecture Decision

Documentation head: 667fde44fb1aaae6247b64a151bd41ae2c4e3042; product source remains a44bc4b37cc221938dd29da63cf965185c75ab5c. The updated R3 and public evidence package provide the original diagnostic test patch, commands, scoped results and source/native identities. This publishes existing local executions, with no new product change or runtime qualification. CI for this documentation head is pending.

The latest architecture CR remains open: semantic ownership, trustworthy domain classification across clone/recovery, and ordinary historical reads under shutdown upgrade and backup/restore require a maintainer decision. Mixed-version operation and downgrade are not added as approval requirements. The earlier R2 model decision and historical evidence below do not constitute acceptance of R3.

Restricted Experiment And Review Gates

Ordinary builds select COPY. Only mo_m0 enables a restricted nullable,
no-backfill trailing ADD, using same-version participants. This is not production
enablement, mixed-version/multi-CN qualification, QA acceptance or merge approval.
The PR remains Ready; reviewer approval and QA are separate pending decisions.

Changes:

  • Admit one trailing nullable ADD per pessimistic RC autocommit in the proven
    hidden-composite-PK domain, preserving old objects instead of copying payload.
  • Preserve clone layout/identity and nonzero catalog/TN versions; prove complete
    typed prefixes before missing suffixes can be interpreted as NULL.
  • Align CN/TN writers, flush, merge, transaction rollback and recovery; stripe
    complete unordered M0 block descriptors without changing ordered/default splits.
  • Keep conservative M0 filtering and COUNT restrictions as explicit costs.
  • Review shared default behavior separately: fail-closed missing columns and
    narrow legitimate proofs, authoritative catalog nullability, name-based
    PRE_INSERT, COUNT identity/nullability guards, dense public ordinals and
    derived-View metadata filtering.
  • No new SQL syntax, catalog fields, wire fields or object-format fields.

Distinct Design Approval Phase

In response to XuPeng-SH's design-gate CR,
the complete feature has a dedicated versioned design:
R2 design at the repair commit, SHA-256
444e7bfc9f0a5b333435b90a7f9b93a70109aeb5232749a0ab018bcc3324eeed.
Fresh isolated Astra medium rejected R1, then returned PASS (design only) on
that exact R2 after the source/geometry and migration boundary was decided.
The separate decision record
names the model, session, reviewed hashes, rejected R1 and approved R2.
Earlier PASS_CODE_REVIEW, green CI and feasibility were not design
approval. This is a retrospective delivery-gate repair, not a claim that design
approval preceded the existing implementation. Full implementation review must
resume only after the distinct design decision; maintainer approval remains open.

The design covers admission/proof provenance, CN/TN commit/rollback/cancellation,
clone/merge/recovery ownership, shared defaults, resource lifetime/cost and removal
boundaries. The resulting narrow implementation alignment binds the legacy proof
to the persisted caller's real DB/table IDs and canonical column seqnum/type,
not names alone. Wrong-object/column checks remain; no NULL fallback is restored.
Any further implementation deviation needs affected design re-review.

gouhongshen's automated review
was formally APPROVED with two non-blocking follow-ups, retained explicitly:

  • Fail-closed ordinary reads may reject historical objects previously NULL-filled.
    Relevant historical upgrade sources and ordinary/system layouts require QA;
    unknown geometry is not automatically corruption or safe NULL. A legitimate
    case needs a verified narrow proof, not a restored blanket fallback.
  • Proof metadata work remains a current-head capacity/performance risk, including
    pristine eligible roots. Reader-local same-object/context reuse exists; no
    cross-scan cache is claimed. The core range/reader proof chain was already in
    CF9, so later repairs are not evidence that the whole cost is new. No speculative
    cache or new performance result is included.

Evidence Identity And Limits

CR-repair head: a329de964e8b7738790c98d26bdf24c0d446a589; its newly
triggered CI must be read under that SHA, not inferred from predecessor PASS.
CI is pending at publication. Ready is preserved; formal CHANGES_REQUESTED
remains until the blocking reviewer submits a new decision.

Implementation reference: 4c9092203a49f5bba5fcd8e1d2f2a8c6bd0c4982.
Kernel: b3c86e3f3375f6441d3326d08ac23ac6598d4d1d.
The latest CR repair adds the design/decision and narrows only the legacy-proof
issuer, with direct regressions. Native and other kernel closures are unchanged;
their evidence is reused by content, not relabelled as new exact-head executions.

Area Evidence and limit
Design and corrective delta R1 REQUEST_CHANGES; frozen R2 PASS (design only), then separate canonical legacy-proof delta PASS_CODE_REVIEW by isolated Astra medium. Exact hashes and session in the versioned review record; not maintainer approval.
CR-repair local execution Canonical/negative legacy proof, default/M0 owning-package execution with -vet=all, and real persisted TestBlockRead/TestBlockRead2 PASS. Two-mode incremental lint (0 issues), molint, err-check, gofmt/diff checks PASS. No new shared state or race obligation; prior concurrency evidence is not relabelled.
Implementation model reviews Isolated Astra medium complete candidate 8f9c5046..22f01a2c plus 13-file corrective Q1 delta: PASS_CODE_REVIEW; ordinal/View deltas through b3c86e3f had no blockers. Not prior design approval or human maintainer approval.
Default regressions Legitimate reader/TopN/BlockRead/publication proofs, catalog/PRE_INSERT/COUNT, ordinary COPY and ordinal/View passed in bound local/CI evidence. COPY selection alone is not compatibility proof.
Explicit tagged execution Formal ALTER, NULL, partial/full UPDATE, old snapshots, clone/committed merge, rollback, cancel/deadline, ADD/DML ordering and recovery have local/Q1 Linux receipts; focused race x3 passed. Ordinary CI does not select all mo_m0 files.
Q1 Linux e1d1ea82 used fresh matching native and default/M0 normal binaries for success, default COPY, TN-applied failure/retry, cancel/deadline/interleave, restart, content/isolation and merge. Unchanged closures are reused, not called exact-b3 binaries.
Shared Linux supplement D3 64aa85d3 / C3 0343d680: tiny-data ordinal/View/customer/content/isolation/restart. Only matching shared closures are reused; D3's extra admission change and C3's DEFAULT false are excluded. Not official mo-tester or whole-product qualification for this head.
Kernel CI 39876195531 at b3c86e3f: UT/SCA/Coverage/BVT/CI Required SUCCESS.
Implementation-reference CI 39987278483 and automatic Ready-event 39994658132 both SUCCESS at 4c9092203a, including UT/SCA/Coverage/BVT/CI Required. Neither is relabelled as documentation-successor CI.
Actual selected BVT PROXY 120534512275 executed customer ADD/clone 48/48 and View metadata 44/44, zero failed/ignored/abnormal. Other green jobs do not add unverified case selection.

BVT: test/distributed/cases/ddl/m0_customer_add_clone.sql and .result.
For this legacy-proof issuer delta, BVT: N/A for forged catalog identity and old
missing system-column geometry, neither constructible through supported current
SQL. The focused proof UT and persisted-read controls cover the internal seam;
historical upgrade/migration acceptance remains QA, not certified by fresh BVT.
The seven-file Q1-to-kernel ordinal/View delta did not change native/modules.
Full provenance and retained failed/HOLD generations are in M0_STATUS.md.
The 4c9092203a test-only repair waits for the post-flush CN logtail frontier
before DUMP; no sleeps/retries, timeout relaxation or product guard changes.
Its normal -vet=all, race x3, lint, gofmt/diff checks and exact-head CI passed.
Earlier failed run 39932171577 attempts remain failures, not overwritten.

Historical Performance Only

CF9 cf9f5ff0 normal-binary SF100 screen: 5 workers x 20 steps, one first/warm,
driver 231.04 s / 215.29 s, 100/100 steps per cohort, zero SQL errors and
benchmark_sanity PASS; 400 committed logical ALTERs total, zero COPY attempts.
Existing B 68e642fd first-run driver reference: 319.88 s, a -27.77% historical
candidate-package observation, not a current-head matched pair, isolated
COPY causality, statistical significance or equivalent process-wall speedup.
B retains its original observer FAIL; only the bound driver reference is reused.
All-branch full content, final CPU/write/debt acceptance and host swap-out WARN
remain explicit limits. This CR repair runs no experiment or new optimization.

QA And Release Non-Goals

QA required: yes. Automated evidence is not QA approval. Remaining QA:

  • Exact source/native/tag products, default COPY and M0 zero-COPY/object identity.
  • Nullable suffix/COUNT/content, old snapshots, parent/child isolation, actual
    merge commit, TN-applied failure/cancel, ADD/DML ordering and restart.
  • Ordinary/subscription View permissions/filtering and historical default-build
    object compatibility, including supported upgrade/backup/restore sources.
  • Correct transaction/resource terminal and diagnostic handling of proof failures.

Unknown legacy shapes block release acceptance; no blanket NULL fallback is
proposed. Do not mix ordinary/M0 participants or remove the tag on an M0 data
directory and call it a downgrade. Production enablement, mixed-version/multi-CN,
power-loss matrices and final maintenance-debt acceptance require separate work.
No DEFAULT false, full-delete skip, shuffle, proof cache, fast-filter restoration,
object replacement, P1/P2, stability matrix or new SF100 result is included.
Parent tracker #30518 must remain open; this partial feature does not resolve it.

@qodo-code-review

Copy link
Copy Markdown

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

XuPeng-SH commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

mo-bot review · mo-bot-local · done

Commit: 4c9092203a49f5bba5fcd8e1d2f2a8c6bd0c4982

Review completed. Read the assessment.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mo-bot review · mo-bot-local · review

Commit: 4c9092203a49f5bba5fcd8e1d2f2a8c6bd0c4982

Assessment: request_changes

The mandatory design-review gate is unmet in the supplied evidence. The restricted append invariant is a coherent starting point, but implementation approval requires a traceable approved design revision. Full implementation review remains outstanding.

Model: gpt-6-astra / xhigh

Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1

  • P2 M0_FEASIBILITY.md:7: Design validation requirement: this feature changes planner, CN/TN, object-reader and recovery contracts, with over 2,500 changed implementation lines across multiple owners.
    Impact: The repository requires a versioned design and a distinct design-review approval before implementation approval. Link the reviewed revision and decision covering admission, commit/rollback/recovery ownership, shared default-build effects, compatibility boundaries and performance tradeoffs. The existing document may be completed and reviewed; an RFC is not required.
    Evidence: M0_FEASIBILITY.md records feasibility and implemented boundaries. M0_STATUS.md:144 records PASS_CODE_REVIEW, but neither document nor the supplied PR evidence binds an approved design decision to a specific design revision. The requirement is explicit in .agents/skills/mo-dev/references/feature-design-review.md, sections 1–2.

Validation:

  • Verified frozen HEAD, merge base, unchanged source, complete 98-file inventory and clean diff whitespace checks.

  • Read both feature documents and inspected admission, object-proof, writer-version and CN rollback boundaries to establish the design-gate trigger.

  • Acknowledged successful exact-head CI statuses; no runtime tests were rerun.

  • Reconciled the static audit receipt and retained log in /home/xupeng/.local/state/mo-bot/jobs/07d0caff00117708bfb2ae79/attempt-1/review-evidence.json.

Limitations:

  • Full implementation, test-quality and tagged validation review was deferred under mo-self-review/SKILL.md: “Missing/unapproved or failed design makes the decision REQUEST_CHANGES and short-circuits full implementation polishing.”

  • No runtime defect is asserted. Author-reported tagged/native/race/recovery evidence has not yet been independently reconciled.

@gouhongshen gouhongshen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex automated review

I reviewed the full diff of #30816 (98 files, about 7970 insertions) at head 4c90922 against origin/main. The PR has no human reviews, no inline review comments, and only a bot status note in issue comments, so there are no prior findings to duplicate or withdraw. The design is consistent and sound: the metadata-only ADD path is admitted only in mo_m0 builds behind inert default-build hook stubs, proofs validate the complete typed column prefix of every block before interpreting an absent suffix as NULL, and every unsupported schema, transaction mode, or layout hole fails closed with an error instead of silent NULL filling. In the default build all hook points resolve to the previous behavior (COPY for ADD COLUMN, ordinary splits, fault injection inert), and the shared changes I traced (name-based PRE_INSERT not-null checks, authoritative catalog nullability via attnotnull, the count(col)-versus-starcount nullability guard, VersionDef propagation through catalog replay, dense information_schema.COLUMNS ordinals for trusted system templates) are bugfix-grade and covered by the added default-build tests, with the well-tested trust boundary for view-template rewrites. I verified the clone, flush, merge, rollback, recovery, writer-version derivation, and reader-proof flows line by line and found no correctness, concurrency, or resource defect that meets a blocking threshold; the two notes below remain after tracing (the mo_tables logical-id legacy proof covers the known ordinary-build missing-column case, and mo_table_col_max_min on appended columns degrades only to an empty answer in a corner of the experimental build). I could not run package tests locally: this checkout has no thirdparties/install (usearch.h, jemalloc.h, xxhash.h, roaring.h are missing) and producing it requires the documented make config dependency build, which the review rules forbid installing here; CGO_ENABLED=0 fails because mpool requires the C allocator. As substitute verification I confirmed the repo's MatrixOne ALL CI run 39987278483 completed with conclusion success at the exact reviewed head sha, which covers UT, SCA, coverage, shared build, and both BVT jobs including the 48/48 customer ADD/clone case. Both reported findings are non-blocking and are recorded so the declared QA and performance acceptance gates keep them in scope.

P2 - Fail-closed missing-column reads turn legacy NULL-fill data into errors in default builds (pkg/objectio/funcs.go:231)

readOneBlockWithMeta previously returned a NULL-filled vector for any requested normal column whose seqnum exceeded the block max or whose column meta had DataType 0. This change returns ErrMissingColumnWithoutProof unless the context carries an M0 proven-object marker or a legacy marker. The only legacy admission added is withRelLogicalIDLegacyProof in pkg/vm/engine/tae/tables/pnode.go, scoped to mo_tables rel_logical_id on TN persisted scans. Persistent data written by historical versions where ADD COLUMN was metadata-only (before commit 03b3f9c/#11429 switched it to COPY in Aug 2023), or any other unanticipated missing-column geometry, will now fail reads with an internal error rather than silently NULL-fill after an upgrade. The change is a deliberate, documented fail-closed direction (M0_FEASIBILITY.md lists fail-closed missing-column reads as an included default-build fix), and matrix version qualification is explicitly deferred to QA, so this is not blocking; but the follow-up QA should exercise an upgrade from any still-relevant older version, and the team should consider a discoverable proof or a stable, user-facing error code so operators are not left decoding 'objectio: missing normal column without proof' from an old object.

P2 - mo_m0 read proofs re-validate full object metadata per scan with no caching (pkg/vm/engine/disttae/m0_hook_mo.go:80)

In mo_m0 builds, proveM0ReadObject runs ReadHeader, ReadAllMeta, and ValidateM0Object (a per-block, per-seqnum column-meta loop) for every visible object on every range scan (called unconditionally from rangesOnePart in txn_table.go, again per new object in readutil reader.read, and in collectTableStats/GetColumMetadataScanInfo for domain tables). The layout predicate passes for every pristine hidden-composite-PK table (version 0 with dense seqnums), so composite-key tables that have never been metadata-only altered also pay the proof on each scan, not just M0-altered tables. The fileservice cache absorbs repeated metadata reads within a query and results are fail-closed correct, and M0_STATUS.md explicitly defers a proof cache and the final performance acceptance, so this is not blocking. Given that the SF100 evidence cited in the PR predates parts of this proof chain, the performance sign-off should measure this cost at scale, including for large ordinary composite-primary-key tables in mo_m0 builds, and treat the 'no proof cache' deferral as a tracked item rather than an implicit none.

@VioletQwQ-0

Copy link
Copy Markdown
Collaborator Author

Addressing the design-gate CR:

I added a complete versioned design under docs/design/, with a separate
design decision record. This is a retrospective approval-stage repair; the
previous feasibility, implementation reviews and green CI were not design
approval, and I am not claiming the implementation originally followed a
design-first sequence.

The fresh isolated gpt-6-astra / medium reviewer first requested changes
on R1's undecided ordinary-build historical compatibility boundary. R2 fixes
the source/geometry acceptance matrix, explicit unsupported inputs and
pre-upgrade migration contract. It then received a distinct design PASS
at SHA-256 444e7bfc9f0a5b333435b90a7f9b93a70109aeb5232749a0ab018bcc3324eeed.
Repair: a329de964e8b7738790c98d26bdf24c0d446a589.
Immutable R2 design
and independent decision record
cover the complete feature, including default-build compatibility and cost.

The approved design also exposed one implementation alignment: the existing
legacy rel_logical_id proof checked names but not the canonical relation.
The repair uses the persisted-node caller's existing catalog-owned DB/table ID,
canonical column seqnum/type and exact object/column binding. The old helper's
wrong-type admission was reproduced before correction. After the design phase,
the separate corrective implementation delta received PASS_CODE_REVIEW.
Local validation: focused canonical/negative proof; default and mo_m0 owning
tables package with -count=1 -vet=all; real persisted TestBlockRead and
TestBlockRead2; both-mode incremental lint (0 issues) and molint; repository
err-check, gofmt and diff checks all PASS. No new shared state is added.
Forged relation IDs and old missing physical system-column geometry cannot be
created via supported current SQL, so the focused delta uses UT/persisted-read
controls; existing whole-feature customer BVT remains, while historical upgrade
acceptance remains QA. The new head owns its pending CI, not predecessor PASS.

This does not relax missing-column checks or modify a persistent/wire format,
introduce an API, add a proof cache, or run new experiments. Model design/delta
review is not a GitHub maintainer approval, deployed QA or production qualification.
Please resume implementation review against this approved design and the
content-bound prior evidence; your formal CR remains open until you review it.

For the two non-blocking notes:

  • Ordinary fail-closed compatibility is now an explicit geometry/migration
    contract, not merely an inventory task deferred to QA. No blanket NULL fallback
    is restored. Historical upgrade/restore execution remains a separate QA gate.
  • Repeated metadata proof cost remains a documented performance/capacity risk,
    including pristine eligible composite-PK roots. There is already reader-local
    same-object/context reuse; there is no cross-scan cache. The core range/reader
    proof implementation is unchanged from CF9, so I do not attribute its whole
    cost to post-pilot changes or claim current-head performance qualification.

The PR remains Ready, not merged. Parent tracker #30518 must remain open.

@XuPeng-SH

XuPeng-SH commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

mo-bot review · mo-bot-local · done

Commit: a329de964e8b7738790c98d26bdf24c0d446a589

Review completed. Read the assessment.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mo-bot review · mo-bot-local · review

Commit: a329de964e8b7738790c98d26bdf24c0d446a589

Assessment: request_changes

Reviewed the complete 100-file delta, callers, tests, design records and supplied comments. The restricted append invariant and reuse of existing transaction ownership are coherent, and the prior design-review requirement is closed. One confirmed admission defect remains: partitioned tables enter the M0 path, after which a successful ALTER breaks subsequent INSERTs.

Model: gpt-6-astra / xhigh

Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1

  • P1 pkg/sql/plan/m0_hook_mo.go:40: With mo_m0 enabled, execute: CREATE TABLE t(a int not null,b int not null,primary key(a,b)) PARTITION BY HASH(a) PARTITIONS 2; INSERT INTO t VALUES(1,1); ALTER TABLE t ADD COLUMN x int; INSERT INTO t(a,b,x) VALUES(2,2,42);
    Impact: ALTER succeeds through M0 on the root, while partition-child ALTERs use COPY and replace their table IDs. Partition routing retains old child IDs, so the subsequent INSERT fails with Error 1146, 'can not find table by id'. Reject partitioned roots and backing partitions using authoritative feature flags and partition metadata in planner/CN admission, retaining the existing COPY path. Add a regression through the actual SQL entry point.
    Evidence: The guard checks TableType == catalog.SystemPartitionRel, but current partitioned roots are represented by features.Partitioned and TableDef.Partition. Scope.AlterTable then replays ALTER against the children. Two independent exact-head probes reproduced M0 admission, an unchanged root ID with version 0→1, replaced child IDs, and INSERT failure referencing an old child ID. Explicit ALGORITHM=COPY controls passed. Existing rows remained readable. Logs: attempt-1/partition-probe.log and partition-readback.log; matching check receipts: 1792992153429546523-6.json and 1792992225817530884-4.json.

Validation:

  • Verified unchanged HEAD and 11,912 tracked files matching the build snapshot; reconciled the frozen design and implementation-alignment hashes.

  • Isolation preflight passed. An initial missing-native-library setup failure was recovered through the repository build owner using the exact source snapshot and assigned caches.

  • Focused mo_m0 validation passed across 13 package invocations; two contained no selected tests. Default-build validation passed 83 root tests across 15 packages.

  • Race-enabled targeted stress passed: TN-applied rollback ×100, concurrent ADD ×3, cancellation ×18, deadline-after-TN-apply ×4, and ADD/INSERT interleave ×12.

  • Reconciled 15 command/source/exit receipts against retained logs. Evidence inventory and hashes: /home/xupeng/.local/state/mo-bot/jobs/3396e36efdf032477bf6f121/attempt-1/review-evidence.json. Final reconciliation passed under receipt 1792992639058624107-3.json.

Limitations:

  • Validation was focused; full repository and whole-package race suites were not run. local_validation supplied no reusable author artifacts.

  • Avoiding table rewrites is the intended gain; repeated metadata proofs remain proportional to blocks × columns. No current-head comparative benchmark was performed, so historical timings do not establish current performance.

  • Multi-CN, mixed-version and historical migration qualification remain outside the stated experimental scope. The partition finding demonstrates write failure, not data loss.

@fengttt fengttt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this PR contains on disk object format change? This is a huge decision and it is not going to work as it is now.

I am not sure why we should touch anything in objectio just to fix metadata nullable stuff.

Please go to @aptend and @LeftHandCold for a total re-design.

@VioletQwQ-0

Copy link
Copy Markdown
Collaborator Author

The partition-admission repair is published in c547c94. Planner and authoritative CN admission now reject partition roots/backing partitions (feature flags, current metadata and legacy representations), retaining COPY fallback. The direct CN regression also checks that rejection precedes table/workspace publication.

Validation on macOS arm64 with Go 1.27.1, against this exact candidate content:

  • The published a329 production source reproduces error 1146 on the first INSERT after ADD COLUMN; explicit COPY succeeds. The repaired complete TestM0MetadataOnlyAddColumnSQL passes, including both default/explicit-COPY paths, old-row NULLs, new rows, replaced root/child IDs, and routing IDs matching live backing relations.
  • Five focused roots (39 named tests), four default-build roots (27 named tests), and the complete tagged planner/disttae packages (20,548 named tests) pass. The focused/default/owning commands use mo-cgo-test -count=1; tagged runs use -tags=mo_m0. Default/tagged vet, configured lint and repository-pinned custom lint completed.
  • The actual canonical ddl/m0_customer_add_clone.sql comparator passes 64/64 twice on the same tagged instance. Database/table/snapshot absence is checked before fallback cleanup after each run; final catalog absence and listener closure pass. No expected failure was skipped or replaced with generated-output acceptance.

The first new SQL test run exposed an unstable physical-name assumption in its oracle after successful writes in both COPY paths. The corrected test matches logical partition names while retaining all ID-replacement/routing assertions; that initial failure is preserved in local evidence.

This increment only repairs partition admission. The on-disk-format decision, ObjectIO justification and requested redesign with the responsible maintainers remain open. This is not whole-PR approval, deployed QA, or a claim that the new head's CI has completed.

@XuPeng-SH

XuPeng-SH commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

mo-bot review · mo-bot-local · done

Commit: c547c949bc97025464926c48755ed8fb45fee65e

Review completed. Read the assessment.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mo-bot review · mo-bot-local · review

Commit: c547c949bc97025464926c48755ed8fb45fee65e

Assessment: request_changes

The partition-admission follow-up addresses the previously identified guards, but the later maintainer request for architectural redesign remains explicitly unresolved. The existing model design approval predates that objection. Full implementation approval is deferred under the repository’s design gate.

Model: gpt-6-astra / xhigh

Session: 01a13dcf-1586-7a01-9000-d45e20995cf8 · slot 1 · generation 1

  • P2 pkg/objectio/funcs.go:231: Design validation requirement: the PR changes ordinary-build missing-column reads and adds persisted-object proof semantics, while the latest maintainer review requests redesign of the storage/ObjectIO approach.
    Impact: The experimental build tag does not isolate the shared reader compatibility change. Resolve the ObjectIO justification, persisted-data semantics and upgrade/migration boundary with the requested owners, aptend and LeftHandCold, and record the accepted design revision before implementation approval. Absence of new serialized fields alone does not close this decision.
    Evidence: Review 5647967218 requests redesign; author comment 6411760792 explicitly states that the on-disk-format decision, ObjectIO justification and requested redesign remain open. Current HEAD changes only partition admission and tests relative to a329de9; the design and ObjectIO implementation are unchanged. feature-design-review.md requires every design blocker to be resolved before implementation approval.

Validation:

  • Verified frozen HEAD, merge base, unchanged source, the complete 100-file inventory and git diff --check. The delta includes 2,537 changed non-test production Go lines.

  • Verified the R2 design hash and reviewed its ownership, compatibility, migration and performance decisions against the subsequent maintainer objection.

  • Inspected the nine-file partition follow-up, including planner/CN rejection, publication guards and SQL routing/content regression assertions; no runtime pass is claimed.

  • Audit passed with receipt checks/1793007436755491741-2.json. Matching review-audit.log and review-evidence.json are retained under /home/xupeng/.local/state/mo-bot/jobs/71957bd5c26e7d6a5884f30a/attempt-1.

Limitations:

  • Complete implementation, lifecycle, test-quality and performance qualification remain outstanding because the architectural decision is unresolved.

  • No runtime tests were rerun. local_validation is empty; author-reported tagged, native, race and BVT results were not independently certified.

@VioletQwQ-0

Copy link
Copy Markdown
Collaborator Author

Main-conflict maintenance only: merge main 2465dae7a4ada1b277113cca5afff6fe10149b96
into the existing PR branch without rewriting reviewed history.
Merge commit: a44bc4b37cc221938dd29da63cf965185c75ab5c.

The sole textual conflict was in mergeObjectsTask.LoadNextBatch. The resolution
keeps upstream WithPreservePhysicalRows for rowid/transfer-map stability AND
the existing M0 object-proof qualification/injection before scans. ObjectIO,
partition-admission repair, the design and its compatibility decisions are not
changed by this conflict resolution.

Matching local CPU native dependencies were regenerated for main's usearch
update; older native/binary and Linux evidence is not relabelled as this merged
head's qualification. Focused validation passed: three M0 merge/writer unit tests;
TestMergeBlocks; TestV9FlushPreservesRowIDAcrossAbortHole; and actual SQL
TestM0MetadataOnlyAddColumnSQL, TestM0CloneMergeProofUsesDerivedVersion,
TestM0MergeReadsOldObjectsWithoutCallerProof. Vet, incremental static checks
and PR-relative whitespace checks are recorded separately from successor CI.
No new experiment or performance result is claimed.

This removes merge conflicts, not the architecture CR. The requested ObjectIO/
persisted-data/upgrade decision with aptend and LeftHandCold remains open;
no maintainer approval, QA acceptance or merge is claimed. Ready state is
preserved, and no redundant architecture re-review is requested here.

@XuPeng-SH

XuPeng-SH commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

mo-bot review · mo-bot-local · done

Commit: a44bc4b37cc221938dd29da63cf965185c75ab5c

Review completed. Read the assessment.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mo-bot review · mo-bot-local · review

Commit: a44bc4b37cc221938dd29da63cf965185c75ab5c

Assessment: request_changes

The original design-document gap and partition-admission defect are addressed, but the subsequent maintainer request for architectural redesign remains unresolved. Current design and ObjectIO sources are unchanged, and the latest author comment explicitly preserves that blocker. Full implementation approval remains deferred under the repository’s design gate.

Model: gpt-6-astra / xhigh

Session: 01a13dcf-1586-7a01-9000-d45e20995cf8 · slot 1 · generation 1

  • P2 pkg/objectio/funcs.go:231: The candidate retains ordinary-build missing-column rejection and persisted-object proof semantics while maintainer review 5647967218 requests redesign of the storage/ObjectIO approach.
    Impact: Design validation requirement: resolve ObjectIO responsibility, persisted-data semantics and the supported upgrade/migration boundary with the requested owners, aptend and LeftHandCold, and record their accepted design revision before implementation approval. The earlier model approval and absence of new serialized fields do not close this later decision.
    Evidence: Author comments 6411760792 and 6432901299 explicitly leave the architecture request open. Independent comparison confirms that ObjectIO sources and the R2 design/decision remain unchanged from a329de9. feature-design-review.md requires every design blocker to be resolved before implementation approval. Source hashes and comparisons are retained in review-evidence.json.

Validation:

  • Verified frozen HEAD, merge base, clean source, complete 100-file inventory and diff whitespace. The delta includes 2,537 changed production Go lines.

  • Read all 13 discussion items and raw comment/review lists; saved discussion-assessment.json with complete, unique key coverage.

  • Verified the R2 design hash, inspected current partition guards and regression assertions, and confirmed the merge resolution preserves both physical-row handling and M0 proof injection.

  • Acknowledged successful frozen current-head UT, SCA, coverage, BVT and CI Required statuses; these do not resolve the architectural decision.

  • Static audit passed with receipt checks/1793075143736948142-5.json. Matching review-audit.log, review-evidence.json, check-ledger.json and review-map.json are retained under /home/xupeng/.local/state/mo-bot/jobs/e1f3d1ac15b0db462b0b4a93/attempt-1.

Limitations:

  • Full implementation, lifecycle, test-quality and performance qualification remain deferred under the mandatory design gate. No new runtime defect or serialized-format change is asserted.

  • No runtime tests or native builds were rerun. local_validation is empty; author-reported native, tagged, race and BVT execution was not independently certified.

  • Avoiding ADD payload copying is the intended gain. Repeated metadata proofs remain a scaling cost; reader-local reuse exists, and the core proof functions already existed in CF9. Historical timings do not establish current-head performance.

VioletQwQ-0 commented Oct 27, 2026 •

Copy link
Copy Markdown
Collaborator Author

@fengttt @XuPeng-SH @aptend @LeftHandCold The updated R3 now includes an inspectable evidence package: exact test patch, reproduction commands, result summary and source/native identities.

Local results on product source a44bc4b3:

  • A two-row CREATE → COPY ADD → clone → checkpoint/restart → child M0 ADD passed. COPY preceded M0 ADD; this covers one controlled lifecycle.
  • Real reader tests verified legal suffix NULLs and rejection of bad prefixes/types and wrong-object proofs. An inconsistent schema was ultimately rejected by shared ObjectIO; production reachability is unproved.
  • The same synthetic bytes read as NULL in the pre-PR default reader and were rejected by the candidate. Historical-release provenance remains unverified.

We favor A: engine/reader-owned evolution and NULL mapping, with physical reads in ObjectIO. Please decide the semantic owner, trustworthy classification across clone/recovery, and ordinary historical-read compatibility for shutdown upgrade and backup/restore. Those design questions remain open.

Head 667fde44fb publishes documentation and existing diagnostic sources only; it adds no product change or new test execution. Mixed-version operation and downgrade are outside this review's approval requirements.

@XuPeng-SH

XuPeng-SH commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

mo-bot review · mo-bot-local · done

Commit: b40841dc5968a127fb37d3f5467e5f8ce0b3a2be

Review completed. Read the assessment.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mo-bot review · mo-bot-local · review

Commit: b40841dc5968a127fb37d3f5467e5f8ce0b3a2be

Assessment: request_changes

R3 provides a reasonable engine-owned direction, but the explicitly requested redesign decision remains unresolved. The original missing-design-document and partition-admission findings are addressed. The remaining P2 concerns domain authority and supported historical-read semantics, not a demonstrated corruption bug or serialized-format change.

Model: gpt-6-astra / xhigh

Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1

  • P2 docs/design/20261027-m0-read-boundary-r3.md:206: Design requirement: the candidate retains default-build missing-column rejection while R3 leaves ordinary/M0/inconsistent classification and historical-read policy undecided. The maintainer's explicit redesign request remains open.
    Impact: Before implementation approval, resolve the requested decision with aptend and LeftHandCold: establish the semantic owner, classification authority across the supported clone/recovery lifecycle, and ordinary persisted-data compatibility under shutdown upgrade and backup/restore. Record the decision against the existing proposal and align the implementation. A new document format or persistent marker is not inherently required.
    Evidence: Review 5647967218 explicitly requests redesign with these owners. Current pkg/objectio/funcs.go:230 rejects unproved missing columns in ordinary builds; m0_context.go:41 carries object name and widths while callers own semantic binding. R3:88–103 and 179–211 leave that contract unresolved. Author comment 6438585016 supplies useful SQL lifecycle and negative-reader observations, but explicitly leaves classification blocked; its synthetic baseline comparison lacks historical-release provenance. The current commit changes only documentation relative to a44bc4b.

Validation:

  • Reconciled all 16 PR discussion items and complete raw lists; saved discussion-assessment.json. Read linked issue 30518 and its three comments. Its clock-error concern is unsupported: Clock.Now returns two timestamps.

  • Verified partition admission guards and regression assertions for routing IDs, COPY replacement and post-ALTER inserts. Confirmed the merge preserves both physical-row handling and M0 proofs.

  • Static audit passed for frozen head/base, clean source, matching patch, complete 101-file inventory, R2 design hash and discussion coverage. Receipt: /home/xupeng/.local/state/mo-bot/jobs/2db1ade05d6255a0bebe995e/attempt-1/checks/1793096425789110216-3.json. Matching review-audit.log, review-evidence.json and check-ledger.json are retained in that directory.

  • Acknowledged successful frozen-head build, UT, SCA, coverage and BVT statuses. These statuses do not establish tagged mo_m0 execution.

Limitations:

  • Full implementation, lifecycle, test-quality and performance qualification remains deferred at the material design gate; mo-self-review permits an upstream design blocker to stop downstream review.

  • No runtime tests or native builds were run. local_validation is empty; author-reported executions were considered but not independently certified.

  • Avoiding ADD payload rewriting is the intended gain. Repeated metadata qualification remains a documented cost with reader-local reuse; historical CF9 timings do not establish current-head performance. No separate performance blocker is asserted.

  • Mixed-version cluster operation and downgrade are outside the repository's supported upgrade contract and are not approval requirements here.

@XuPeng-SH

XuPeng-SH commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

mo-bot review · mo-bot-local · done

Commit: 667fde44fb1aaae6247b64a151bd41ae2c4e3042

Review completed. Read the assessment.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mo-bot review · mo-bot-local · review

Commit: 667fde44fb1aaae6247b64a151bd41ae2c4e3042

Assessment: request_changes

The published diagnostics improve R3's evidence but do not close the existing maintainer architecture requirement. The original design-document gap and partition-admission defect are addressed. One P2 design requirement remains; no new production defect is asserted.

Model: gpt-6-astra / xhigh

Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1

  • P2 docs/design/20261027-m0-read-boundary-r3.md:270: The candidate retains shared default-build missing-column rejection while ordinary/M0/unknown classification and historical-read compatibility remain undecided under the explicit maintainer redesign request.
    Impact: Design requirement: resolve semantic ownership, trustworthy classification across supported clone/recovery paths, and ordinary persisted-data compatibility under shutdown upgrade and backup/restore with the requested owners, aptend and LeftHandCold. Record the decision against the existing proposal and align implementation before approval. No new document format or persistent marker is inherently required.
    Evidence: Review 5647967218 explicitly requests this redesign. Current pkg/objectio/funcs.go:230 rejects unproved missing columns; m0_context.go:41 carries object name and widths, leaving semantic binding to callers. R3:255–275 and author comment 6438585016 retain the unresolved decision. The published tests cover one controlled CREATE/COPY/clone/restart sequence and synthetic reader cases; they neither establish historical-release provenance nor close classification across the supported lifecycle. Product code remains unchanged from a44bc4b.

Validation:

  • Read all 18 indexed discussion items and complete raw lists, linked issue 30518 and its three comments; saved discussion-assessment.json with unique, complete coverage.

  • Inspected current partition guards, routing/write regression assertions, canonical legacy-proof binding and merge-context preservation. The linked clock-error allegation is unsupported: Clock.Now returns two timestamps.

  • Static audit passed: frozen source/base, complete 103-file inventory, patch consistency, whitespace, unchanged product code, R2 hash and all three published diagnostic-source hashes/applicability.

  • Matched checks/1793103174386350804-3.json to review-audit.log and check-ledger.json in the assigned diagnostic directory.

  • Acknowledged successful exact-head build, UT, SCA, coverage, BVT and CI Required statuses; these do not establish tagged execution or historical-upgrade compatibility.

Limitations:

  • Full implementation, lifecycle, test-quality and performance approval remains deferred at the material design requirement.

  • No runtime tests or native builds were rerun. Original diagnostic execution logs were not supplied; published results remain author-reported.

  • Avoiding COPY is the intended gain; repeated metadata qualification remains a documented cost. Historical timings do not establish current-head performance. Mixed-version operation and downgrade are not approval requirements.

This branch was previously deployed

1 inactive deployment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/feature size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants