feat: add identity-bound materialized views with transactional refresh - #27615
jiangxinmeng1 wants to merge 121 commits into
Conversation
…iew-24553 # Conflicts: # pkg/defines/const.go # pkg/sql/compile/ddl.go # pkg/sql/parsers/dialect/mysql/mysql_sql.go # pkg/sql/parsers/dialect/mysql/mysql_sql.y # pkg/sql/parsers/dialect/mysql/mysql_sql_test.go # pkg/sql/plan/bind_delete.go # pkg/sql/plan/bind_update.go # pkg/sql/plan/build_update.go
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? |
aptend
left a comment
There was a problem hiding this comment.
Deep re-review at exact head bedd5bc (base 9b7c2c2). I read all 14 historical reviews, author replies, and all four resolved inline threads, compared the a9b49f4→current increment and the complete feature scope. The increment addresses the prior restore DROP path and replaces the raw-catalog substring heuristic; the mandatory design gate and validation are still blocking, so this is not an implementation approval.
[P1] docs/design/materialized_view_incremental.md:4,11,129-145 still specifies protocol 74 and says only 'Approval decision: approved', without an independent approver, reviewed revision, or approval link. The PR body links an older d3652e7 design and says protocol 57, while pkg/catalog/mvdefinition/definition.go:35 and pkg/sql/compile/compile.go:8551 now require 100. The only maintainer APPROVE is from 2026-09-11, before the 2026-09-15 author-authored design change and this head's 100 change; the author explicitly noted that independent final-design approval remained pending. A rollout or independent implementation following the versioned design would admit a v74-v99 predecessor lacking the MV consumer. Please update the exact design revision/PR summary to the actual capability, resolve mixed-version and rollback implications, and record traceable independent approval of that revision.
[P1] pkg/sql/compile/compile_test.go:2708-2709 still sets MORPCVersion74 and requires requireMaterializedViewCapability(c) to return no error. At this head that function calls supportsMultiSourceISCP, which rejects every version below 100 (compile.go:8544-8551), so this existing compile-package test deterministically fails. materialized_view.go:197 also tells users to upgrade to protocol 74 even though 74 is rejected. Update the test to assert v99 rejection/v100 acceptance and fix the diagnostic. I did not run native Go tests after the design gate failed; this is a direct assertion-versus-condition contradiction, with exact-head UT/Coverage still running.
Compatibility coordination: matrixone PR #29291 independently assigns MORPCVersion100 to instance-bound drain on the same v99 base. If its v100 binary becomes a predecessor for this PR, it advertises 100 without an MV consumer, reproducing the mixed-version admission defect. Reserve distinct cumulative versions before these features are combined/rolled out; this belongs in the design decision above.
…i-20260923 # Conflicts: # pkg/vm/engine/tae/db/merge/scheduler_promotion_test.go
aptend
left a comment
There was a problem hiding this comment.
Deep re-review at exact head a80686b, against declared base/merge-base b9013dc. I revisited the complete feature scope and all prior reviews/replies; all four inline threads remain resolved with no new replies. The bedd5bc→current increment closes the earlier protocol-number, stale capability-test, diagnostic, and design-link mismatches by moving the MV fence to 101. Two blockers remain:
[P1] Mandatory independent design approval is still missing. docs/design/materialized_view_incremental.md:7-11 explicitly says 'pending independent approval of the exact revision'. This feature changes durable catalog identity, ISCP job semantics, a protocol capability, async refresh, restore and rollback contracts, so the approved design revision is a release gate. The last maintainer APPROVE was on 2026-09-11, before the author revised the design and before this 101 contract; no later independent approval is recorded in PR reviews or the four threads. Please obtain a traceable maintainer decision on the exact current design revision (reviewed commit and approval link) before requesting implementation approval. This is an objective gate, not a request to repeat the full implementation review.
[P1] The exact-head UT Coverage run fails the MV-owned View-recovery regression, independently of the design gate. pkg/sql/compile/view_metadata_recovery_test.go:1683 still asserts the old lower(coalesce(t.viewdef,'')) not like ... SQL predicate, which production view_metadata_recovery.go:1109-1120 intentionally removed to avoid misclassifying ordinary Views. In the same test, the 'revalidation page' fixture at :1689-1708 supplies 9 columns with a uint64 at index 7, but the new query/reader expects 10 columns with viewdef text at index 7 and status at index 9; the test panics with unsafe slice cast when the reader treats the old uint64 column as a string. Exact-head job 108823812870 reports both failures. Update the fixture and oracle to the new 10-column contract (including ordinary-View and MV controls), rather than restoring the unsafe substring filter. Other unrelated planner failures in that job are not attributed to this PR here.
I stopped broad local implementation testing at the mandatory design gate; exact-head CI supplies the stated test counterexample. No claim of full implementation approval is made.
aptend
left a comment
There was a problem hiding this comment.
Re-review at exact head 98bd6ce68e6338cc10adcad04ef716950bb34ffa against declared base/merge-base b9013dc6bb058ec74935a62ac3a35f57d13c2f98. I read all historical reviews/comments and the four resolved inline threads, and checked the two-file increment since my a80686bdd6 review against the previously reviewed full feature scope. REQUEST_CHANGES: the mandatory design gate is still open.
[P1 — design/release gate] docs/design/materialized_view_incremental.md:7-11 still records “Approval decision: pending independent approval of the exact revision.” This feature introduces durable MV identity/state, protocol 101 compatibility, async ISCP refresh, and restore/rollback rules, so the exact revised design needs a traceable independent maintainer decision before implementation approval. No review or issue comment since the previous head records that decision. The PR body links revision 38e41ca36a, but its own document still says pending; an author summary of automated implementation review cannot substitute for design approval. Please record the approver, reviewed design revision and approval link (or an explicit policy-owner exception) and update the document accordingly.
The f0e23646d2 increment updates view_metadata_recovery_test.go:1683-1713 to the new 10-column fixture and removes the stale substring assertion, addressing my previous exact-head CI finding at the source level. The new exact-head UT/coverage checks are still running, so I am not claiming a pass. The other increment in query_builder.go adds planner compatibility guards; the unresolved design gate short-circuits broad implementation approval per the feature review contract. This is not a blanket approval or a claim that the 96-file implementation has been newly exhaustively retested.
aptend
left a comment
There was a problem hiding this comment.
Re-review at exact head 0c7ba5645f2fddb68762a5902305d562030e016d, against merge-base b9013dc6bb058ec74935a62ac3a35f57d13c2f98 and my prior reviewed head 98bd6ce68e6338cc10adcad04ef716950bb34ffa. I rechecked the full feature scope and historical reviews/resolved threads; this increment changes no implementation or tests, only one design-document line. REQUEST_CHANGES: the independent design approval gate is still not evidenced.
[P1] docs/design/materialized_view_incremental.md:11 now says only Approval decision: approved. Commit 0c7ba564 was authored and committed by this PR's author, jiangxinmeng1; there is no recorded independent approver, reviewed design revision, decision rationale or approval link in the document, PR review/issue comments, or owning issue since the prior review. This does not satisfy the requested independent maintainer decision on the exact durable contract. For example, merging this one-line self-approval would publish MV catalog identities, protocol 101, async refresh/rollback and restore behavior without any traceable owner acceptance of their compatibility invariants; a later mixed-version or restore failure would have no approved contract to adjudicate against. Please obtain the independent design review (or an explicit policy-owner exception), record who approved which exact revision and link the decision, then request re-review. The prior fixture correction remains in place; exact-head required CI is still running and is a separate gate. I am not asserting a new runtime defect from this documentation-only commit.
|
Recording the current design-approval status for traceability: the author reports that @aptend has approved the revised materialized-view design but cannot submit the implementation commit. The GitHub review history currently has no APPROVED review from aptend on the exact revision; the latest aptend review remains CHANGES_REQUESTED on 0c7ba56. Please have aptend record an explicit APPROVE review for the exact design revision, or have the policy owner record an explicit exception with the reviewed revision and rationale. |
aptend
left a comment
There was a problem hiding this comment.
Deep re-review at exact head 0c7ba5645f2fddb68762a5902305d562030e016d (merge-base b9013dc6bb058ec74935a62ac3a35f57d13c2f98). I revisited the D2 design contract, complete feature diff, historical reviews and all four resolved threads. The 98bd6ce→current increment changes only the document's approval-status line. Per the requested scope, I am not re-raising the separate-approver/process objection. Earlier source-union routing, protocol-101 fence, restore classification/DROP path and View-recovery fixture corrections are present. Focused exact-head tests passed in pkg/sql/plan, pkg/iscp, pkg/sql/compile and pkg/frontend. Current-head CI still has pending jobs.
REQUEST_CHANGES for a separate, reproducible timezone-correctness defect:
[P1] FAST admits a timezone-sensitive TIMESTAMP cast — pkg/sql/plan/materialized_view_incremental.go:521. The CastExpr branch recursively accepts CAST(ts AS DATE) as a group key; a review-only exact-head test confirmed that buildMaterializedViewIncrementalPlan returns a nonempty FAST spec. timestampToDate converts through proc.GetSessionInfo().TimeZone (pkg/sql/plan/function/func_cast.go:2236,4913), but incremental refresh executes background SQL through iscp.ExecWithResult without WithTimeZone, so the executor uses the worker's local timezone. Repro: with session +08:00 and a UTC worker, create an ON CHANGE FAST MV grouped by CAST(ts AS DATE) on a TIMESTAMP source, then insert 2026-01-01 00:30:00 in that session. The source query groups it under Jan 1; the background delta groups the same instant under Dec 31 and still advances the watermark. The MV is silently wrong.
[P1] COMPLETE/FORCE still admit date_trunc(TIMESTAMP) — pkg/sql/plan/materialized_view.go:35-55. The global validator rejects volatile/real-time functions, but DATE_TRUNC is marked neither. The specific exclusion in materialized_view_incremental.go:537 only disables FAST eligibility; COMPLETE and FORCE therefore run the expression in background full refresh. DateTruncTimestamp reads the process timezone (pkg/sql/plan/function/func_binary.go:15452), while the persisted definition and background refresh carry no defining timezone. The same +08:00/UTC midnight-boundary example yields different day buckets between the defining query and MV. A second review-only exact-head test confirmed the COMPLETE query passes the global validator.
Both counterexample tests failed as expected on this head and were removed from the temporary worktree. Please reject every timezone-sensitive expression/cast for all refresh modes, or persist a fixed evaluation timezone and apply it to initial, tail, full and manual refresh; add a cross-timezone SQL result-comparison regression.
…nto codex/fix-pr27615-ci-20260923
…eview # Conflicts: # pkg/defines/const.go # pkg/pb/plan/plan.pb.go
… codex/fix-pr27615-review # Conflicts: # pkg/defines/const.go # pkg/pb/plan/plan.pb.go
…nto codex/fix-pr27615-ci-20260923
aptend
left a comment
There was a problem hiding this comment.
Deep re-review of exact head 7379585. I read the historical reviews, issue replies and all four resolved threads, compared the post-0c7ba564 increment with the full MV paths, and checked current CI. Per the requested scope I am not using the separate design-approval process objection as a blocker. The previous CAST and DATE_TRUNC paths are now rejected, and all three MV BVT files pass. One implementation blocker remains:
[P1] pkg/sql/plan/materialized_view.go:56-77: the new IntervalExpr rejection never sees MySQL INTERVAL syntax. pkg/sql/parsers/dialect/mysql/mysql_sql.y:14478-14488 constructs INTERVAL 1 DAY as a FuncExpr named interval, but materializedViewTimezoneSensitiveFunction does not list interval. The exact-head UT Coverage job confirms the new TestMaterializedViewAdmissionTracksAllInputs case for select ts + interval 1 day, count(*) from src group by ts + interval 1 day returns nil when rejection is expected (build_ddl_test.go:1722). Consequently a COMPLETE or FORCE MV over a TIMESTAMP can still be admitted even though TIMESTAMP calendar addition uses the process timezone in doTimestampAdd (func_binary.go:1567). With a defining session in +08:00, a UTC refresh worker, and values around a day or DST boundary, the materialized grouping can differ from the defining query while its watermark advances. Reject the actual parser representation or persist and apply one evaluation timezone throughout initial, incremental, full and manual refresh; verify the negative admission test and a cross-timezone result comparison. Exact-head failure: https://github.com/matrixorigin/matrixone/actions/runs/36511434199/job/109224510693 .
Other coverage and Compose failures also exist, but I am not attributing the unrelated SET/index cases to this PR; the MV-specific new failing assertion alone blocks approval.
aptend
left a comment
There was a problem hiding this comment.
Deep re-review of exact head 50c33ca against base 0870004: REQUEST_CHANGES. I revisited the full MV change map, historical reviews and author replies, all four resolved threads, and the one-commit delta since my prior review. The prior INTERVAL spelling is now rejected. Per the requested scope, I am not treating separate design-approval paperwork as a blocker.
[P1] pkg/sql/plan/materialized_view.go:60-79 still admits DATE(timestamp), another timezone-dependent spelling. DATE is absent from materializedViewTimezoneSensitiveFunction, so validateMaterializedViewQuery returns nil. Function resolution implicitly casts TIMESTAMP to DATE (pkg/sql/plan/function/type_check.go:2830-2833); that conversion uses proc.GetSessionInfo().TimeZone (func_cast.go:2229-2246). The defining session's timezone is not persisted or installed on the background COMPLETE/FORCE refresh worker. Repro: create a TIMESTAMP source and an ON CHANGE COMPLETE MV grouped by DATE(ts), insert '2026-01-01 00:30:00' in a +08:00 session, then refresh on a UTC worker. The same stored instant is Jan 1 in the defining query and Dec 31 during refresh, so the MV silently puts the row in the wrong group while advancing its watermark.
A review-only exact-head test confirmed both the implicit TIMESTAMP->DATE cast and full CREATE MATERIALIZED VIEW ... REFRESH COMPLETE ON CHANGE planning; its negative admission assertion failed because the validator returned nil. The test was removed afterward. Please reject this spelling (and other timezone-sensitive implicit casts) for all modes, or persist and apply one evaluation timezone to initial, tail, full, and manual refresh, with a regression comparing results across timezones.
Existing admission and mvdefinition tests pass locally after make thirdparties/CGo; git diff --check is clean. Current-head GitHub checks show 8 passes, 3 pending, 15 skipped, so I do not claim CI completion. MORPCVersion101 is also allocated by other open PRs and must be coordinated at merge, independently of this blocker.
aptend
left a comment
There was a problem hiding this comment.
Deep re-review at exact head fb101cf against declared base/merge-base 0870004, and the one-commit increment since my prior 50c33ca review. I revisited the historical reviews, issue/inline comments, author replies and all four resolved threads, plus the previously reviewed full MV change map. Per the requested scope I am not treating separate design-approval paperwork as a blocker. The new DATE(ts) global admission check closes my previous timezone counterexample: TestMaterializedViewAdmissionTracksAllInputs passes at this exact head, and full CREATE calls the same validator before persistence. The async/ownership graph has no new changes in this increment.
Two independently reproduced P1 regressions in ordinary SQL planning block approval. I ran the same selected tests with CGo/native artifacts on this exact head and on its exact base: both pass on base and fail on this PR head. They also appear in this head's failed UT/Coverage checks.
-
[P1] pkg/sql/plan/query_builder.go:10076-10080 replaces
builder.mysqlSpecialOrderKey(ctx, ...)withmakeMySQLSpecialOrderKey(...). The former preserves the raw SET bitmap through transparent projection, or canonicalizes grouped SET display; the latter immediately rejects non-reversible SET definitions (including an empty member). Reproducer:select id, s from (select id, s from set_empty_member_t) d order by sand the grouped/CTE variants inTestMySQLSpecialOrderProvenanceSetWithEmptyMembernow fail at planning withnot supported: definition-order sorting of projected ENUM/SET values with non-unique display labels or ambiguous SET display values, although this existing contract passes on base. Ordinary SELECT ORDER BY on a valid SET column is lost; retain the context-aware raw/canonical path, and rerun the named provenance/cast tests. -
[P1] pkg/sql/plan/query_builder.go:6191 drops
boundCountExpr != nil || boundOffsetExpr != nilfrom canonical SET provenance propagation. A LIMIT/OFFSET projection is a materialization boundary; its downstream numeric SET aggregate/cast no longer knows to use the surviving canonical display. Reproducer:select sum(s) from (select s from set_empty_member_t limit 2) dnow fails withnot supported: numeric SET value without retained storage identity or canonical materialized output(TestMySQLSpecialNumericAggregateIdentity), while the exact-base test passes.TestCanonicalSetCastThroughResultProjectionalso loses the canonical cast on the LIMIT boundary. Preserve the boundary provenance and its existing tests.
The current-head Compose PESSIMISTIC SQL BVT completed successfully but its job failed afterward while generating coverage; the View SQL-mode mock SetContext failure is also present in base and is not attributed to this PR. The two regressions above are independently attributable to PR-only lines. git diff --check is clean.
…i-20260923 # Conflicts: # pkg/defines/const.go # pkg/frontend/mysql_cmd_executor_test.go # pkg/pb/plan/plan.pb.go
…eview # Conflicts: # pkg/defines/const.go # pkg/frontend/mysql_cmd_executor_test.go # pkg/pb/plan/plan.pb.go
… codex/fix-pr27615-review # Conflicts: # pkg/defines/const.go # pkg/frontend/mysql_cmd_executor_test.go # pkg/pb/plan/plan.pb.go
aptend
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 35071f8 against current base 19e01d2 and my previous reviewed head fb101cf. Read the review/comment/thread history and checked the complete change map together with the post-review and main-merge deltas. Head advanced during review, so I did not submit the prepared old-head review and repeated the affected checks at the new head. The two SET provenance issues from my last review are fixed: empty-member ORDER BY, numeric aggregate identity, and canonical SET casts through result projections now pass. The prior MV admission and restore/recovery fixes remain in place; no separate design-approval paperwork is being used as a blocker.
Two current-base integration regressions remain, detailed inline. Both are locally reproduced on the exact PR head and pass on the exact current base, using isolated worktrees and the repository CGo test wrapper. Removing the CREATE INDEX synchronization point breaks the existing deterministic cross-CN tests; removing SetContext expectations breaks the existing View planner fixtures. These also match failures in the preceding c83148d head's CI run 36658759200; new-head CI is not being presented as completed evidence. Please retain the existing synchronization/fixtures and rerun those tests. The unrelated deletion of existing CHAR/VALUES/index-lock regression tests should also be undone unless their oracles are preserved elsewhere.
Validation at the new head: full catalog/mvdefinition and ISCP suites pass; focused SET and MV planner tests pass; TestBindViewUsesStoredSQLModeForPipesAsConcat fails on head and passes on base; TestIssue28928ConcurrentCreateIndexNameIsUnique/identical_definitions fails on head after 30 seconds waiting for the removed marker and passes on base. These findings concern broken retained validation, not a claim that the corresponding production View or index semantics are incorrect.
aptend
left a comment
There was a problem hiding this comment.
Deep re-review at exact head 42bde2b against base/merge-base 19e01d2 and my last reviewed head 35071f8. I read the updated review/comment/thread history (all six threads are resolved), reviewed the complete feature change map together with the two-file increment, and checked fixes against production callers and retained test oracles rather than treating thread resolution as proof.
Both blockers from my last review are closed. CREATE INDEX again exposes the planning-before-locking fault boundary, with the original waiter assertions retained. query_builder_test.go is now byte-identical to the current base, restoring both SetContext expectations and the existing CHAR/VALUES/window regression coverage. Earlier SET provenance, timezone-admission, protocol fence, and restore/View-recovery fixes remain present. No blocking finding remains. As explicitly requested by the policy owner, separate design-approval paperwork is not used as a blocker for this PR.
Exact-head local validation passed using isolated worktree/CGo provenance-checked artifacts: complete pkg/sql/plan, catalog/mvdefinition, and ISCP suites; complete ISCP suite under -race; focused MV/capability/View-recovery tests in compile and frontend; and all selected TestIssue28928/TestIssue28931 cross-CN regressions (9.868s). git diff --check passed. SQL BVT/performance evidence for unchanged MV semantics was read and reused, not independently rerun; current-head CI still has pending checks, so this approval is not a claim that all required CI has finished.
Non-blocking PR-body correction: its Compatibility paragraph still says protocol 101 and links an older design revision, whereas current code and the committed design require 102. A deployment following 101 guidance fails closed at CREATE admission; please synchronize the summary/link with the actual 102 contract.
What type of PR is this?
Which issue(s) this PR fixes:
issue #24553
What this PR does / why we need it:
Materialized views now bind refresh work to the exact catalog generation and publish result, auxiliary state and tail watermark in one transaction. Source replacement and stale prepared reads fail explicitly; unrestricted DELETE preserves referenced source identity and releases dead groups/state.
Fixes #24553.
Implemented SQL: FAST/FORCE/COMPLETE ON CHANGE; COMPLETE ON DEMAND with caller-transaction REFRESH; DROP MATERIALIZED VIEW. Incremental maintenance covers grouped COUNT/SUM/AVG/MIN/MAX, exact DISTINCT aggregates, HAVING and compatible UNION ALL. COMPLETE supports up to 16 direct base sources including joins. Unsupported hidden subqueries/CTEs/session-dependent or volatile expressions are rejected before persistence. This is the concrete D2 scope; the earlier operator-graph roadmap is not part of this implementation.
Repairs and cleanup:
Validation on the final candidate:
Compatibility: requires protocol 101 throughout MV-serving services. Protocol 99 and the independently assigned protocol 100 feature are predecessors without the MV consumer and cannot create, claim, refresh, or recover MV jobs. Earlier unreleased MV envelopes require recreation; drop MVs before binary rollback. Actual predecessor decoding/consumer and public planner rejection boundaries were checked; mixed-version MV availability and whole-cluster downgrade are not claimed.
Design: English, 中文.