Conversation
7 tasks
XuPeng-SH
added a commit
to matrixorigin/matrixone
that referenced
this pull request
Sep 26, 2026
## What type of PR is this? - [ ] API-change - [x] BUG - [ ] Improvement - [ ] Documentation - [ ] Feature - [ ] Test and CI - [ ] Code Refactoring ## Which issue(s) this PR fixes: Fixes #28594 ## What this PR does / why we need it: - Applies the session `div_precision_increment` setting to exact DECIMAL division type derivation and evaluation, including prepared statements and CTAS metadata. - Derives DATE, YEAR, TIME, DATETIME, and TIMESTAMP division precision from their packed numeric values. Ordinary DATETIME(6) division now returns DECIMAL(24,10) instead of an unnecessary DECIMAL256(42,10). - Widens Decimal64 operands to Decimal128 when a temporal or integer operand can exceed Decimal64's 18-digit range; retains Decimal256 when the declared result or input requires it. - Bounds Decimal256 shifts and validates declared precision after rounding. Handles negative and 64-bit divisor edges without overflow or divide-by-zero panics. - Expands unit and BVT coverage for all temporal types, increments 0/4/10/30, reversed operands, full-range TIME(6), prepared execution, CTAS, overflow, and metadata. Validation on rebased head `aafa3abeac` (base `71c415705c`): - Full `pkg/sql/plan/function` and `pkg/sql/plan` UT: pass. Focused frontend prepared-plan and plan-cache UT: pass. - `make build-with-prebuilt-native`: pass. Strict metadata BVT on the rebased service: `issue_28594_div_precision.test` 70/70 and `TimeToInt.sql` 267/267, both with zero failures. - Before rebase, strict metadata BVT passed twice for each case (70/70 and 267/267 per run); `time.test` passed 82/82, `datetime.test` 149/149, and `timestamp.test` 126/126. `go vet` passed; golangci-lint v2.6.2 reported 0 issues. - A 1,024-row expression benchmark reduced retained bytes for implicit DATETIME(6) division from 81,952 to 32,784, matching the explicit DECIMAL(20,6) path. On an idle host, the new path measured 373–393 µs versus the earlier 394–402 µs; performance did not regress in this local probe. - Independent final review found no remaining blocker. New-head remote CI is tracked in the PR checks. ## Mixed-version and direct execution follow-up (`9d0519ba54`) - Prevent new decimal DIV/0 plans from running on v96 CNs: placement falls back to one CN during rollout, and the sender rechecks the actual destination before transmission. v97 receivers continue to accept legacy v96 plans, whose stored scale the new executor honors. - Require the v97 catalog admission floor when authoring persisted decimal division expressions, including views whose division folds during planning. - Rebuild the direct-execution Decimal128 return type so its physical Size is 16 bytes; serialized result vectors now round-trip. - Added focused regression tests for feature detection, v96/v97 placement and send behavior, unknown workers, legacy-plan execution, folded view admission, and direct-result serialization. Validation on this commit: full `pkg/pb/plan`, `pkg/sql/plan/function`, `pkg/sql/plan`, and `pkg/sql/compile` unit tests pass; focused mixed-version tests and `go vet` pass. `make build-with-prebuilt-native` passes. Strict #28594 BVT on the new service passes twice on the same instance, 70/70 each run, with empty error reports. Independent review found no remaining confirmed blocker. New-head CI is pending. Rolling-upgrade note: legacy views executed locally on a v96 CN keep their previous decimal division semantics until that CN is upgraded. Newly authored views using the revised semantics wait for the v97 catalog admission floor. ## DDL expression and QA follow-up (`3903e9cbf5`) - Apply the session `div_precision_increment` setting to CREATE/ALTER generated columns, defaults, ON UPDATE, CHECK constraints, and CTAS defaults. Internal COPY ALTER SQL inherits the session variable resolver. - Preserve already-bound expressions during COPY ALTER and CREATE TABLE LIKE when their definitions and referenced types are unchanged. New expressions bind under the current setting; expressions that depend on changed types rebind. Existing column references are remapped across inserted, reordered, and hidden columns. - Added unit tests for DDL binders, replay remapping, type/enum/auto-increment incompatibility, and internal variable resolution. Expanded #28594 BVT to cover old/new precision, CHECK success and failure after COPY ALTER, defaults, LIKE, and type-change rebinding. Validation on this commit: full `pkg/sql/plan` and `pkg/sql/compile` UT passed; `go vet` on both packages passed; golangci-lint v2.6.2 reported 0 issues; native `mo-service` build passed. Strict metadata BVT on the rebuilt service passed twice, 117/117 statements per run. Existing `generated_column.sql` passed 334/334 with historical metadata differences ignored; `check_constraints.sql` passed 20/20 and `create_table_like.sql` 25/25 under the same comparison mode. The new case used strict metadata comparison. The local SQL probe independently confirmed retained 12-digit values after column reorder while current direct division used 6 digits. Root-cause lesson: earlier tests covered operator inputs and precision values but omitted the DDL consumers and persisted-expression lifecycle. This change adds a consumer × setting-change × schema-change matrix, including negative CHECK cases, so rebinding mistakes surface as value or metadata mismatches. ## CTAS inherited DEFAULT precision repair (`cb7025e3a3`) - Keep the source's already-bound row-reference DEFAULT when CTAS only changes the session precision. Check assignment type and every local operand against the final target schema; rebind when an actual operand or assignment type changes. Explicit target defaults still bind under the target session. - Reconcile nullability annotations when an operand becomes nullable. Continue dependency validation and persisted-expression protocol admission for the new table, even when reusing a source binding. - Added planner UT and strict metadata BVT for inherited rows and future inserts, alias and reorder mapping, changed types, explicit target defaults, nullable operands, source-catalog immutability, and protocol admission. The 19-case manual matrix records pre-fix 9/19 versus post-fix 19/19 twice. Validation on this commit: full `pkg/sql/plan` UT passed; `go vet ./pkg/sql/plan` passed with repository CGo paths; golangci-lint v2.6.2 reported 0 issues. Rebuilt `mo-service`; strict `issue_28594_div_precision.test` BVT passed twice on one service, 140/140 per run with zero failed, ignored, or abnormal statements. Existing `expression_default_column_reference.sql` BVT passed 107/107 using its historical metadata-ignored mode. All manual probe databases and BVT database were removed. New-head CI is pending. Why repeated review was needed: the first planner mock omitted the catalog `Type.Table` lineage marker. That made a type comparison appear correct in UT while the SQL service still rebound inherited defaults. The regression test now models that marker, and public SQL verifies the persisted behavior. The new reuse path also required an explicit protocol-admission test; adding it caught a second omission before submission. The case inventory in `test/manual/issue28594/README.md` records 74 coverage/gap entries and 10 remaining separate gaps. ## Table DUMP/LOAD binding repair (rebased head `08495b29d8`) A table can contain persistent expressions authored under different `div_precision_increment` values. The v1 DUMP manifest carries text and a schema hash, but no per-expression binding. LOAD could accept a matching target created under another setting, then evaluate newly inserted rows at the target's precision rather than the source's. A real mixed DEFAULT=10/GENERATED=4 table reproduced this mismatch. - Emit manifest v2 only when a persisted expression is precision-sensitive. Carry bounded bound-expression metadata, and accept it at LOAD only if ordinary DDL binding of the target's own SQL origin under a legal increment 0..30 reproduces each executable tree. Match each expression independently, so mixed values survive. A SHA256 digest detects corruption but does not authorize executable data. - Apply the same protobuf wire preflight at DUMP and LOAD before `Unmarshal`: at most 16,384 columns/checks, 100,000 fields, depth 64, and a 32 MiB payload. Reject duplicate JSON fields and incompatible declarations. Validate relation/object/auto-increment metadata before one guarded catalog replacement, preserving the target's owner and creation time. An ambiguous v1 dump fails with a new-dump remedy before mutation. - `SHOW CREATE` copied independently into a new session is a separate SQL contract: the current CREATE syntax cannot express different increments per expression. Snapshot/PITR ordinary clone already carries bound expressions and is unaffected by this manifest change. Design: `docs/design/20260925-table-dump-expression-binding.md`, revision 10, independent review PASS (SHA256 `89d3587ff02d50cee59a442cd7eff6ba311e24ea045598550d67b6537a570868`). Final independent implementation review: PASS on clean head `08495b29d8`, with no remaining blocker. Validation on rebased production commit `9c18076b98` (base `3f0a68bd80`) plus test-only commit `08495b29d8`: - `make build`: PASS. Full `pkg/sql/plan` and `pkg/frontend` UT: PASS (17.211 s and 98.044 s). New exporter-cap test targeted: PASS. Incremental `go vet` on both changed packages with repository CGo flags: PASS. - Strict `table_dump_load.sql` BVT on the rebuilt service: 83/83 twice, zero failures, ignored, or abnormal cases. Covers both mixed-precision directions, old and new rows, and generated-column UPDATE results. White-box tests cover 0/4/10/30, nested/folded division, tampered executable tree, parser mode differences, producer/reader resource limits, and immutable target-definition preparation. - Failure-path SQL probe: a v2 full dump with a deliberately altered object size failed after `ReplaceDef`; after an explicit `COMMIT`, the target still bound `(1,3)` at its original 4-digit setting. A downgraded v1 manifest was rejected before mutation. A service restart preserved the correctly loaded source precision and the rolled-back target precision. - Comparable one-relation/one-object metadata manifests: v1 1,096 B; v2 1,584 B (277 B raw bound metadata). Ten LOADs per round on one service, alternating v1/v2/v2/v1, took 1.250/1.421/1.797/1.985 s respectively; ranges overlap under background service load. This is an administrative-path probe, not a throughput benchmark. No row-evaluation hot path changed. Compatibility: older LOAD readers reject v2; upgrade readers before producing v2 dumps. v1 remains for insensitive tables. The dump source must be kept when reverting a reader that cannot load v2. Remote CI on the new head is pending. ## DUMP binding and resolver repair (`414c2d8f49`) - Keep complete frontend compiler-context delegation, but query a partial internal process resolver only for session `div_precision_increment`. This restores the old nil/default behavior for unrelated variables such as `foreign_key_checks`, which caused 27 exact-head UT failures. - Rebind generated expressions with every visible column in declaration order, then reject self-reference explicitly. CREATE and DUMP now agree when a generated column appears first, middle, or last. - Rebind each CHECK using a shallow, read-only table header instead of deep-copying every CHECK on every candidate. Stop trying increments after both a legal match and table sensitivity are known. Remove the redundant frontend nil check flagged by SCA. Validation: full `pkg/sql/compile`, `pkg/sql/plan`, and `pkg/frontend` unit suites passed locally; golangci-lint v2.6.2 incremental analysis of these packages reported 0 issues. The rebuilt service passed strict `table_dump_load.sql` twice on one instance (96/96 each), including old rows and future inserts with generated columns at first and middle positions; strict `issue_28594_div_precision.test` passed 140/140. The 100-CHECK local probe measured about 856 MB cumulative allocation and 430 ms on the previous head; the final benchmark measured 18.4 MB/op and 36.7 ms/op on the same host. These are local administrative-path measurements, not a peak-RSS or throughput claim. Independent final review: PASS. New-head remote CI is pending. Why this needed another pass: earlier tests put generated columns only after their operands, so a shifted column coordinate escaped. The work budget counted AST visits but missed full-schema copies. Plain `go vet` did not cover the CI `nilness` analyzer. This repair adds first/middle/last position cases, a count-scaling benchmark, an explicit schema-immutability assertion, and the CI-configured linter check. ## CN shutdown and adversarial QA follow-up (`5ef74a001e`) - Fixed a shared shutdown bug that made both BVT jobs lose their SQL-serving CN coverage counters. The cgroup watcher now uses a nonblocking inotify descriptor through the Go poller; cancellation closes it, and setup/admission failures release it. The exact old CI binary hung even with a 120-second grace period. This also repairs ordinary service shutdown, independent of coverage. - Added correctly hashed malformed DUMP metadata cases and exact CHECK-count/field-count boundaries. Rejected input produces no replacement and leaves the target definition unchanged. A valid round trip is the positive control. - Shared collection integrity checks are in matrixorigin/CI#463. That PR documents its dependency on the runtime fix; it does not lower the coverage threshold. Local validation: system package normal/race tests pass; focused watcher race tests pass 100 repetitions; new frontend QA normal/race tests pass; changed-package configured SCA reports 0 issues. Strict source-built two-CN BVT passes DUMP/LOAD 96/96 twice, TimeToInt 267/267 twice, and #28594 division 140/140 twice after normal catalog admission. Both CNs exit 0 and write counters in the validated teardown. The latest production binary is `ec6aa1a1f6`; `5ef74a001e` and `58058cf406` add tests only. Coverage update at `58058cf406`: the prior local 551/725 (76.00%) calculation included a locally controlled BVT teardown. The actual remote CI run for `5ef74a001e` reported **532/725 (73.38%)**, although DUMP/LOAD BVT ran 96/96; its collected CN profiles recorded zero hits in the DUMP binding files. The new frontend declaration-matrix UT directly executes the restore and rejection paths. Merging its focused UT coverage with that exact failed CI merged profile using the unchanged parser, filters, and strict >75% gate yields **550/725 (75.86%)**, an 18-block gain with the same 725 changed blocks. This is a local replay of the CI gate, pending the new-head remote result. The separate collector integrity fix remains in matrixorigin/CI#463. Performance evidence from the unchanged SQL repair remains: 100 CHECKs measured 36.7 ms / 18.4 MB allocated per operation versus the previous reproducer's 430 ms / 856 MB; the 1,024-row temporal-division probe reduced retained bytes from 81,952 to 32,784. The watcher adds bounded process-level state with no per-row work. These are scoped local measurements, not universal throughput or peak-RSS guarantees. --------- Co-authored-by: XuPeng-SH <xupeng3112@163.com>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Require BVT coverage to contain counters from SQL-serving CNs that exited normally. The current workflows can publish a syntactically valid profile containing only the other service roles, which then appears as low PR test coverage.
Observed in matrixorigin/matrixone#29241, run 36205017094: both BVT profiles have zero hits in
mysql_cmd_executor.godespite successful SQL cases. The coverage parser's union is correct. Reproduction with the exact CI binary showed that both a 10-second stop and a 120-second stop ended at exit 137 with no CN counters: the global Stopper waited on the cgroup watcher's uncancellableunix.Read.The runtime fix is included in matrixorigin/matrixone#29241. Rollout dependency: land the runtime repair before enabling these stricter checks for branches that still contain the old watcher. A longer stop timeout alone does not fix the hang.
Changes
Validation
ec6aa1a1f66bc36ac0b83400ee7e07d107cd28ff: stricttable_dump_load.sql96/96 twice andTimeToInt.sql267/267 twice. Each fresh run used new counters; both CNs exited 0 in about 7.4–7.5 seconds and wrote separate counter files. The helper then performed cleanup and produced a valid profile with SQL executor hits.The local integration used a Debian 13 runtime matching the local native build; the failing baseline used the downloaded CI binary and its original image. These are lifecycle/collection proofs, not a claim that every remote BVT job or #29241's full coverage gate has already passed. Independent review found no code blocker in this collector.
Risk and rollback
This intentionally turns missing CN counters into a producer failure. Healthy jobs do not sleep for the 120-second limit; measured CN stop was about 7.5 seconds. No database query path or coverage mode changes. Revert the workflow/action change to restore the previous collection behavior.