Repository navigation
feat(sirius): add complete public SQL GPU acceptance campaign - #29862
Conversation
|
mo-bot review · Commit: Assessment: approve Reviewed the complete 37-file delta against the approved D campaign design. Both previous P2 findings are resolved in the current implementation with matching regression evidence. No actionable unresolved defect remains within this scope. Model: Session: Validation:
Limitations:
|
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: 688bb85222b892fca964fb8becdb610131aaaf34
Assessment: approve
Reviewed the complete 37-file delta against the approved D campaign design. Both previous P2 findings are resolved in the current implementation with matching regression evidence. No actionable unresolved defect remains within this scope.
Model: gpt-6-astra / xhigh
Session: 01a12d8f-7dea-77c2-a565-38d45b5564e4 · slot 0 · generation 0
Validation:
-
Verified the assigned HEAD and clean, unchanged source checkout. Separately assessed implementation/tooling, tests and documentation.
-
Traced public SQL execution, cancellation before unread-result close, exact row comparison, schema publication, session reset, independent shared-CTE readers, process cleanup and allocation accounting through their existing owners.
-
All five source/test hashes in preserved review-fixes.json match the current checkout. Reused its 82 passing Linux Python tests, archive cardinality/schema counterexamples, and live finalization/export quota probes with kill-and-join assertions.
-
Verified that the public runner, schema helper, CTE expansion, bridge and Sirius pin are unchanged from the recorded release capture. Reused qualified evidence for 744 timing executions, 264 resource executions, six allocation profiles, ten lifecycle cases, matched sanitizer controls and ten numeric regression gates.
-
Frozen evidence for the assigned HEAD reports successful required CI, Ubuntu unit tests, arm64 static analysis, coverage, PROXY/PESSIMISTIC multi-CN tests and Connector/J reset tests. Skipped checks were not counted as passes.
-
Recorded evidence attribution and source checks in /home/xupeng/.local/state/mo-bot/jobs/33485d3f4eb50886f3039c3f/attempt-1/review-evidence-audit.json.
Limitations:
-
No native/GPU/sanitizer or test-suite reruns were performed. Measurements retain their actual capture revision, 9814347, and the documented source qualifications; they are not measurements of the current HEAD.
-
The native submodule is uninitialized locally. Assessment of the unchanged Sirius pin reused the substantive frozen review and release evidence.
-
The recorded GPU route was slower overall on the RTX 3070. This approval establishes neither a speedup nor absence of ordinary-SQL performance regression.
-
Flight remains unprovisioned. Default cutover, retirement and Docker work are outside D. Full linked-issue bodies were not supplied; scope was checked against the frozen PR and approved design, without claiming closure of the parent issues.
Merge Queue Status
This pull request spent 12 seconds in the queue, including 1 second running CI. Required conditions to merge
|
## What type of PR is this? - [ ] API-change - [ ] BUG - [ ] Improvement - [x] Documentation - [ ] Feature - [x] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: Fixes #29838 Related to #29968 (TopN ANY NULL regression coverage) ## What this PR does / why we need it: Spark 3.5.3's JDBC writer defaults to `READ_UNCOMMITTED`, which MatrixOne rejects before inserting rows. This documents the supported `isolationLevel=READ_COMMITTED` option for Spark JDBC writes, links the guide from both root READMEs, and adds a runnable compatibility probe. MatrixOne's isolation behavior is unchanged. The probe checks the default writer's MatrixOne 20105 error and empty target, then writes from two nonempty Spark writer partitions with `READ_COMMITTED` and verifies readback through a fresh Spark JDBC read and a separate JDBC connection. The TopN test is a separate upstream CI test-contract addition. It keeps unsupported tuple/interval types as negative cases, retains the `ANY` Prepare-success cases already on main in #29862, and exercises nonliteral `ANY NULL` through actual partition TopN execution. `TestPartitionTopNNonLiteralAnyNullOrder` checks output NULLs, payload/group cardinality, flat and constant NULL batches, Reset reuse, and pool cleanup. This consolidates the TopN coverage from #29975; no production code changes. ## Validation - Complete `pkg/sql/colexec/partition` package passed through the repository CGo wrapper on combined head `ca733a4b001b364ab467521b182fdb199d2c40ad` (`2.197s`). - `gofmt` and `git diff --check` passed. - Spark-side build and probe evidence is from the unchanged Spark files and production source head `73e54458d1a4cd209f9d8eda7795242935880551`: build passed; Spark 3.5.3 / Connector/J 8.0.33 / JDK 17 verified READ_UNCOMMITTED rejection (20105), empty target, successful READ_COMMITTED two-partition write, fresh Spark JDBC read and separate SQL readback, and cleanup. The probe was not rerun after the TopN test-only addition. - Focused BVT `test/distributed/cases/set/set_transaction.sql` passed 30/30 on base `035a5b61ba84ffc7dd2193135a7b170af04a4bff`. An attempt on source head `73e54458d1a4cd209f9d8eda7795242935880551` failed during mo-tester's pre-test database cleanup after connection retries, before any case ran; no BVT result is claimed for combined head `ca733a4b001b364ab467521b182fdb199d2c40ad`. - At the post-push snapshot, CI preflight, PR size, coverage eligibility, and Mergify Summary passed. UT Test on Ubuntu/x86, SCA on Linux/arm64, shared build, and UT Coverage on Ubuntu/x86 were pending; other listed jobs were skipped. Full remote CI has not passed. --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
## What type of PR is this? - [ ] API-change - [x] BUG - [ ] Improvement - [ ] Documentation - [ ] Feature - [ ] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: Related to #29965: fixes the repeated catalog-ID scan amplification identified in its statistics preparation path. Keep the incident open for Family acceptance rather than automatically declaring every observed handshake failure resolved by a component experiment. ## What this PR does / why we need it: ### Root Cause `getChangedTableList` resolves each returned table ID separately. The catalog BTree is ordered by account, database, name and descending timestamp, not by table ID. Consequently M requests can traverse N retained name/version entries M times, including full-scope scans for already-dropped IDs. Each traversal holds the BTree read lock. The logtail receiver needs its write lock for each catalog update. Writer preference prevents permanent starvation, but repeated short scans can interleave with many catalog writes and delay completion of a whole update batch. Authentication read barriers behind that work then wait; weakening those barriers would break catalog consistency. Incident evidence in #29965 contains both statistics scans and receiver write lock waits, with authentication blocked on read barriers. Native histogram samples add a discriminating observation: in a 30-second affected interval, 4,482 completed catalog-cache update observations accumulated 29.740 seconds; each individual observation was at most the 45.562ms histogram bucket boundary. This is cumulative contention, not proof of one continuously held 30s lock. ### Scoped Change - Add a batch ID lookup that walks each requested catalog scope once. - Group changed-table requests by account/database at the existing snapshot. - Preserve input order, duplicates, missing IDs, view exclusion and tablePair metadata; preserve tombstones, replacement identities and same-TS ordering. - Keep scalar lookup and singleton behavior, with no persistent index/cache, worker, retry, timeout, protocol, authentication, GC or routing changes. - Scratch memory is O(requested IDs), released with the call. Allocate output tablePair storage only for accepted tables, not every missing ID. ### Validation Base: main `02a2823c2007d5e4fb4ada148e93743df337551a`. Tests run on shanghai50, Linux amd64, Go 1.27.1, repository CGo test wrapper, verified matching native artifacts, four CPU affinity slots. After PR creation, Mergify merged main `7af631bdf7` (#29862, Sirius acceptance campaign) into this branch as `b93970e7e9`. All four delivery files are byte-for-byte unchanged. The local/50 results below are from the authored `d32ae8663d` revision; merged-head GitHub CI is a separate, currently pending check. Focused snapshot and changed-table consumer tests passed. The consumer test uses a real catalog cache and transaction timestamp waiter, exercises both CheckChanged and CollectChanged through the exported callback API, and covers zero-TS requests, ordering, duplicates, views and missing IDs. | Verification | Result | | --- | --- | | Full `disttae/cache` package | PASS, normal 0.112s / race 1.248s | | Full `disttae` package | PASS, normal 7.142s / race 9.186s | | Four new focused tests under race | PASS | | `TestGetTablesByIdAndTimeConcurrentWriterAndGC`, race x100 | PASS, 1.427s package time | | Existing `TestMoTableStatsMoCtl` and `TestHandleGetChangedList` engine tests | PASS, 2.909s package time | | Formatting, diff check, final ownership/visibility/liveness review | PASS | The focused concurrency test's first race run rounded to 0s, so the repository adaptive rule selected 100 repetitions. Full packages were not repeat-stressed. The engine tests use real engine/SQL execution and TN changed-table collection; they are smoke coverage, not a full Proxy/ANLI reproduction. New UTs use tiny catalog fixtures and deterministic start synchronization, not timing assertions. Equal finite-work diagnostic (not a CI timing assertion): one scope with 45,559 names and eight versions, 4,000 requested IDs (90 visible), and a fixed 1,000-row InsertTable plus DeleteTable workload: | Lookup | Full catalog update completion | | --- | ---: | | No-scan control | 1.46ms baseline / 1.66ms candidate | | Existing per-ID path | 18.826s | | Actual production batch API | 12.529ms | The old reader itself took 38.418s versus 10.849ms for the new batch. Returned IDs and visible versions are checked in both arms. No injected lock holds or sleeps are used. The synthetic retained catalog is not claimed to reproduce the exact incident scope distribution or all 512/508 ANLI failures. Permanent microbenchmark, 4,096 names x four versions and 64 missing IDs: 8.425ms / 23,040B / 65 allocations becomes 0.155ms / 3,208B / 5 allocations. Singleton measurements are approximately 132/131us with the same 360B and two allocations. These measure catalog lookup work, not application QPS. ### Risk And Acceptance Boundary The existing snapshot/name visibility algorithm and tree synchronization remain unchanged; no locks are released mid-traversal. Ordinary scalar readers, auth barriers, data caches and vector/SQL execution paths are not modified. This patch does not promise bounded latency for an arbitrarily large single scope, eliminate every other catalog reader, or replace same-workload Daily acceptance. --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Validate canonical public SQL and the complete local release gates for embedded
Sirius GPU execution.
What type of PR is this?
Which issue(s) this PR fixes:
Tracks #28968 and parent #28966; implements D from #29690 after merged C #29757.
What this PR does / why we need it:
The Go runner executes canonical Q1–Q22 over immutable SF1/SF10 inputs, fully
drains public results and compares exact typed schema, values, duplicates and
ordering against independently executed native MatrixOne. The controller owns
separate LOG/TN/CN processes, bounded private spools and joined failure cleanup.
All 744 timing executions are unprofiled. A separate 264-execution campaign
captures actual CUDA allocation/free events. Logical credits/pool tags, physical
allocations, residency, RSS and Go heap retain their source and scope.
Shared CTE occurrences use independent destructive MO readers on the same
statement snapshot. Merged Sirius #30
preserves retained VARCHAR identity casts and rejects reused/unreachable MO
bindings. D pins merged native
955b6881, preserves ABI v1 and uses the verifiedrelease SDK. Optional unprovisioned Flight profiles are N/A.
Early consumer and provisioning failures cancel the unread query before driver
close and preserve error ownership. Deterministic fixtures prove one physical
connection, immutable SQL/config admission, complete-spool publication, late
errors, failed-query cleanup, eight source identities and typed loaded row
multisets. Frontend tests check actual query/account/physical-OID evidence and
heading privacy. No dependency or CI-policy changes are introduced.
Release validation
Preserved release evidence
contains full manifests, safe query records, raw timing repetitions, six allocation
profiles, lifecycle/sanitizer proofs, fresh numeric samples and artifact hashes.
These records are preserved under fork tag
evidence/sirius-d-4230ce3d14andremoved from the current source tree. The validation note documents exporting
them to a temporary workspace; the unchanged CLI reproduces
local_acceptance_complete: true.exact Q1–Q22 SF1/SF10 results and streams 1/2/4. Embedded queries completed GPU
work without fallback, with healthy terminals and zero retained input/result
credit. All owned campaign processes joined before publication.
reconciled logical pool baselines. Actual ordered CUDA spans and successor
windows support post-close proof; lifetime high-water marks keep their scope.
cancellation, writer/numeric error ownership and healthy reuse. Fatal quarantine
closure is scoped to joined test-owned process death.
check passes 34,509 assertions/30 cases with one CUB registration diagnostic,
matched by Native A. The ABI check passes 1,964 assertions/10 cases with one
41,386,248-byte process-global cuDF default pinned allocation, matched by the
no-GPU-work initialization control and all six stable allocation profiles.
Raw counts/statuses are retained; no suppression or waiver is used.
executions, matched operands and 21 samples per case. Maximum ratio 1.026527;
checked-add64 0.843248. Actual historical sources/binaries remain identified.
same-process Sirius/cuVS coexistence and 82 Python contract tests pass. The
final main merge's affected type/function/planner/compiler/frontend suites pass.
The GPU route was slower overall on the recorded RTX 3070. Timing evidence remains
separate from allocation profiling and retains all per-query and Q9 repetitions.
Both complete campaigns use one frozen release provenance record at MO
9814347c1c, Sirius955b6881, SDK29bca11a, with actual rebuilt combined serviceand runner hashes. Native proof reports preserve their original execution
contexts and explicit source qualifications. Historical development and the
failed first release attempt remain separate. The quota monitor's retirement
race was fixed by one stat snapshot, tolerating only removed files; permission
and quota failures remain fatal. Full release schedules restarted from the
beginning. No partial capture contributes to acceptance.
Delivery head
688bb85222normally merges authoritative main02a2823c20.Cleanup commit
70ec74b7d1removes all 54 generated evidence files (about 123,000lines) from the current PR tree. Code, benchmark definitions and regression
fixtures are unchanged by that cleanup. Earlier Git history and capture
provenance are preserved, with the fork tag keeping the snapshot fetchable.
The affected frontend/compiler/planner/vector/comparison suites pass after the
main merge, including applicable race checks; full pre-push SCA passes. Original MO
9814347c1ccapture identities, binaries and measurements remain preserved. Subsequent main
changes address COM_SET_OPTION, prepared/ENUM/window consumers and vector/branch
index filtering; the canonical immutable TAE campaign routes remain qualified.
Required CI passed at
eba41acd67in replacement run38163597900.
The two substantive P2 review findings are corrected in
78d87489da:oracle cells, checking retained cardinality, physical descriptors and schema
identity. The CLI verifies the archive checksums and parses the same bytes it
hashed. Full-row live comparison remains the value oracle; unordered/peer
spool hashes may differ.
worker execution, profiler finalization, owned export and shutdown. Quota or
deadline failure kills and joins the owner; outer adopted-child cleanup still
gates publication. Initial PID-generation publication failure also retains
spawn cleanup. Finalization/export retain their three-hour deadlines.
Review-fix evidence
records six previously accepted parity mutations and two quota-owner red proofs.
All 82 owning Linux Python tests pass. The actual untouched archive passes
with identical acceptance output; native Q1 count 4→5 and OID 61→62 mutations
fail, even after rehashing. Real 1 MiB writers crossing a 512 KiB quota during
finalization/export are killed and joined before parsing/publication. A 1 KiB
export reaches parsing after join. These tests use small process barriers and
reused fixtures, without new GPU measurements or a framework.
Old captures preserve their actual collector source and do not retrospectively
certify the new continuous disk-quota monitor. Fresh CI for this fix head and
reviewer approval still gate merge; no performance or policy waiver is used.
Remaining readiness
The complete implementation and local evidence are ready for review.
Reviewer approval follows that handoff; it is required before final acceptance
and merge. The earlier draft wording incorrectly combined those two stages and
has been corrected.
acceptance_completeremains false in the immutable capturereport; CI and reviewer decisions are external and are not inferred by the local
validator. Both P2 threads are answered and resolved; XuPeng-SH is requested to review the corrections.
After current required CI, reviewer approval and the user merge, E implements
the approved embedded-only design. E is stopped until D merges. The owner
confirmed that no customers use the sidecar; F and its migration/compatibility
requirements are dropped, and the abandoned sidecar repository stays untouched.
Docker remains separate; both tracking issues remain open.
The frozen release tar export has SHA-256
28cfb19a161bc89aa917bf9add42e496b425a3b71f63399670d6daa43cfb269e.After removing checked-in captures, all 82 Python tests pass and external-archive
acceptance is identical. The source tree contains one concise validation note
with export/check commands; raw generated reports remain outside it.
The coverage job in run 38200331638
failed two inherited UT fixtures, both reproduced on clean authoritative main.
Commit
84955f2e93corrects their inputs/synchronization without changingproduction execution:
outstanding signal, all 200 deliveries, and main/subshell PID checks. The
sender is joined before its exit trap is disarmed. All 100 coverage-mode and
100 race-mode focused executions pass; the full optools coverage package passes.
ANYremains a positivecontrol for the domainless NULL comparator added on main. The full partition
and comparator coverage packages pass, including pool cleanup assertions.
Fresh authoritative main also merged TRUNCATE lifecycle regression coverage
(#29827). Commit
435b1a81ceupdates that fixture to provide the transactionsnapshot now inspected by admission. The retained case supplies its held lock;
fresh owners supply an empty snapshot. All wait/cancel/FastFail and zero wait
bookkeeping assertions remain. The failure reproduced on exact clean main;
the corrected full compiler coverage package, 100 focused race executions, and
targeted static checks pass.
No sleeps, retries, skips, longer deadlines, weakened assertions, or GPU changes
are introduced. Targeted static checks pass. Full pre-push SCA passes; fresh required CI and reviewer approval still gate
merge. Main #29919 is normally merged: full compiler/planner/Substrait coverage,
all 22 exact-profile embedded candidates, and the authenticated multi-CN
privilege-cache test pass. Mock optimizer plan serializations vary between runs,
so no whole-inventory byte-equivalence or new GPU/performance claim is made.
The retained D captures preserve their original MO
9814347c1cexecutionidentity, measurements, and existing source qualifications.