[Enhancement] IVM: re-derive the maintenance query at refresh instead of freezing it#74881
Merged
Conversation
…n, no trial) Refresh-time variant of rewrite(): uses the MV's persisted encodeRowIdVersion instead of re-deducing, and skips the CREATE-time trial compile. Shares rewriteInternal with the CREATE path. Signed-off-by: Youngwb <yangwenbo_mailbox@163.com>
…e-derive Move the position-aligned output-vs-schema compatibility check out of MVRefreshSchemaChecker into IvmSchemaCompat so the upcoming refresh-time re-derive shape gate can reuse it. Behavior-preserving. Signed-off-by: Youngwb <yangwenbo_mailbox@163.com>
…ry at refresh deriveRewrittenQuery parses the original query, re-runs the IVM rewrite with the MV's pinned encode version (no trial), re-analyzes, and enforces a row-id strategy gate. derive() adds the shape gate (IvmSchemaCompat) and a diff-WARN against any legacy frozen ivmDefineSql, returning the maintenance SELECT for refresh. Signed-off-by: Youngwb <yangwenbo_mailbox@163.com>
… reading frozen ivmDefineSql MVIVMRefreshProcessor now acquires the metadata lock from a skeleton built off the original query, then inside the lock re-derives the maintenance SELECT via IvmRefreshDefinition.derive and builds the INSERT through the existing formatInsertSql path (parameterized to accept the derived SELECT, preserving the AUTO_INCREMENT column-list handling). Retarget the PCT-fallback test's failure injection to the parameterized getIVMTaskDefinition overload now on the IVM refresh path. Signed-off-by: Youngwb <yangwenbo_mailbox@163.com>
The IVM refresh path now re-derives the maintenance query each refresh, so the frozen rewritten SQL is no longer needed. CREATE still derives the MV column schema from the rewritten tree (the re-analyze stays); only the serialization into ivmDefineSql is dropped. The field is retained for legacy MVs and the refresh-time diff-WARN. MVRefreshSchemaChecker also re-derived its comparison query from the frozen ivmDefineSql via getMVQueryDefinedSql(). With the frozen text gone that fell back to the original user query, whose field count omits the hidden __ROW_ID__/__AGG_STATE_* columns, so the external-base schema check failed with "column count N vs M" and inactivated every IVM MV on refresh. Route the checker through IvmRefreshDefinition.deriveRewrittenQuery for incremental MVs so it compares against the same rewrite refresh runs; PCT still compares viewDefineSql. Signed-off-by: Youngwb <yangwenbo_mailbox@163.com>
…hecker no-false-drift Round-trip test asserts IvmRefreshDefinition.derive is serialization-stable across IVM query shapes (scan, GROUP BY col/ordinal, DISTINCT, all-constant, sum, max) — the load-bearing assumption of the re-derive approach. Retroactivity test proves derive ignores any persisted frozen ivmDefineSql. Checker test pins that a new IVM MV (no frozen text) does not false-trip schema-drift inactivation. Signed-off-by: Youngwb <yangwenbo_mailbox@163.com>
IVM refresh always re-derives the maintenance query and calls the parameterized getIVMTaskDefinition(selectSql). The no-arg form had no production callers and would have produced a semantically-wrong IVM INSERT (the original user query, missing the __ROW_ID__/agg-state columns). Point the remaining tests at the parameterized form. Signed-off-by: Youngwb <yangwenbo_mailbox@163.com>
…path) An IVM MV's first/full refresh runs through the hybrid -> PCT path (MVPCTRefreshProcessor), whose INSERT was built from getTaskDefinition() -> getMVQueryDefinedSql(). Since IVM MVs no longer persist the frozen rewritten ivmDefineSql, that now returns the bare user query, so for aggregate / DISTINCT MVs the positional INSERT had fewer columns than the MV schema (which carries the hidden __ROW_ID__ / __AGG_STATE columns), failing the refresh with "Inserted target column count doesn't match select/value column count". Re-derive the rewritten maintenance query inside the lock for incremental MVs, mirroring MVIVMRefreshProcessor. Scan (AUTO_INCREMENT) MVs are unaffected -- their rewritten query equals the user query; only QUERY_COMPUTED (aggregate / DISTINCT) MVs hit it. The existing IVM refresh tests seed a TVR baseline to force the pure-IVM delta path, so none exercised the first-refresh full rebuild. Add a test that does a real first refresh of an aggregate IVM MV through the PCT path. Signed-off-by: Youngwb <yangwenbo_mailbox@163.com>
…ompat as drift Three issues found by an adversarial review of the re-derive refactor: 1. AUTO-mode gap: the PCT full-rebuild guard (MVPCTRefreshProcessor) and the schema checker (MVRefreshSchemaChecker) gated on isIncremental(), which excludes AUTO. Legacy AUTO-mode IVM MVs also carry the hidden __ROW_ID__/__AGG_STATE schema, so the checker fell to the viewDefineSql branch (fewer columns), tripped the arity check, and falsely inactivated a healthy MV. Widen both guards to isIncrementalOrAuto(). 2. Re-derive incompatibility was misclassified as transient: a row-id strategy flip or a no-longer-IVM-rewritable query threw a SemanticException whose message matched none of isLikelyDriftException's signatures, so the checker retried forever instead of inactivating with a clear "drop and recreate" reason. Add the two markers. 3. deriveRewrittenQuery cast the parse result without an instanceof/empty guard; a corrupt or empty viewDefineSql threw an opaque ClassCastException mid-refresh. Guard it, matching the PCT / old-checker pattern. Add a regression test for the AUTO-mode checker case (validated: it fails without the isIncrementalOrAuto fix -- the MV is falsely inactivated). Signed-off-by: Youngwb <yangwenbo_mailbox@163.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
… mode The re-derive guards in MVPCTRefreshProcessor and MVRefreshSchemaChecker used getCurrentRefreshMode().isIncrementalOrAuto(), assuming AUTO implies an IVM schema. It does not: an AUTO-mode MV whose query is not IVM-eligible has a plain PCT schema (no __ROW_ID__). For such an MV the guard fired derive(), which either throws "is not an IVM query" or trips the row-id strategy gate (QUERY_COMPUTED vs stored null) -- failing the PCT refresh, and in the schema checker falsely inactivating a healthy MV. Gate on the schema instead: getRowIdStrategy() != null (the MV has a __ROW_ID__ column == it was materialized by the IVM rewrite). That is the precise precondition for "must re-derive the rewritten query", independent of refresh mode -- correct for INCREMENTAL and IVM-eligible AUTO, and correctly skips non-IVM AUTO and plain PCT MVs. Add a regression test for the non-IVM AUTO case (validated: false-inactivates without the fix). The IVM-attempt derive in MVIVMRefreshProcessor needs no such guard -- its throw is caught by the hybrid processor, which falls back to PCT. Signed-off-by: Youngwb <yangwenbo_mailbox@163.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ccf0509d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…Codex review) The re-derive guards gated on getRowIdStrategy() != null (has __ROW_ID__) alone. Codex flagged the inverse of the earlier AUTO fix: __ROW_ID__ is not a reserved column name (FORBIDDEN_COLUMN_NAMES is only __op/__row/___count___), and MaterializedViewAnalyzer accepts a column named __ROW_ID__, so a PCT MV can carry one -- the schema-only gate then fires derive() (mode is PCT -> empty -> throws "is not an IVM query"), failing the PCT refresh / inactivating the MV. Neither predicate alone is correct: mode alone (isIncrementalOrAuto) misfires on a non-IVM-eligible AUTO MV (plain PCT schema); the __ROW_ID__ column alone misfires on a PCT MV with a __ROW_ID__ column. Require BOTH -- a real IVM MV is created with the IVM refresh mode AND the __ROW_ID__ schema together. This scopes re-derive to exactly real IVM MVs; any non-IVM MV (incl. a __ROW_ID__-bearing PCT MV) stays on the unchanged viewDefineSql/PCT path. Signed-off-by: Youngwb <yangwenbo_mailbox@163.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 088421d05a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4c717d2ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…dex review) IvmSchemaCompat.compare skipped every hidden column (__ROW_ID__, __AGG_STATE_*). A re-derived rewriter can hold the visible projection and arity while changing a hidden column, and refresh would then INSERT positionally into the old hidden schema -- risking corrupted aggregate state instead of failing loud into drop-and-recreate. Validate hidden columns in place: position-aligned base-type compatibility plus, for hidden columns, a name check -- the __AGG_STATE_* name encodes the agg function/args, so a function-identity drift at an unchanged base type is caught. Comparing the full AggStateDesc was rejected: for an unchanged rewriter the stored Column's descriptor and the re-derived field's descriptor differ benignly, which false-trips every aggregate IVM MV. The name+type check is false-positive free across the iceberg agg IVM suite (joins, unions, multi-key aggregates). Signed-off-by: Youngwb <yangwenbo_mailbox@163.com>
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Contributor
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
Contributor
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
Contributor
[FE Incremental Coverage Report]✅ pass : 85 / 95 (89.47%) file detail
|
HangyuanLiu
reviewed
Jun 22, 2026
HangyuanLiu
approved these changes
Jun 22, 2026
Astralidea
approved these changes
Jun 22, 2026
Contributor
|
Tick the box to add this pull request to the merge queue (same as
|
Contributor
|
@Mergifyio backport branch-4.1 |
Contributor
✅ Backports have been createdDetails
Cherry-pick of 14bf004 has failed: To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally |
23 tasks
2 tasks
Youngwb
added a commit
that referenced
this pull request
Jun 22, 2026
Mergify left conflict markers in IVMBasedMvRefreshProcessorIcebergTest. #74881's hunk that retargets the getIVMTaskDefinition mock signature sits inside testImvSourceRangesNotRecordedOnPctFallback -- part of a TVR/imvSource feature not present on branch-4.1 (no getImvSourceVersionRange), so the hunk could not apply and dragged the surrounding main-only methods into the conflict. Resolve by taking branch-4.1's side: the three main-only testImvSource* methods are dropped (they would not compile here). #74881's own addition, testFirstFullRefreshOfAggregateIvmMvRebuildsRewrittenInsert, stays -- its helpers exist on branch-4.1. The resolved file equals branch-4.1 + that one method. Signed-off-by: Youngwb <yangwenbo_mailbox@163.com>
OliLay
pushed a commit
to OliLay/starrocks
that referenced
this pull request
Jun 30, 2026
… of freezing it (StarRocks#74881) Signed-off-by: Youngwb <yangwenbo_mailbox@163.com> Signed-off-by: Oliver Layer <o.layer@celonis.com>
18 tasks
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
4 participants
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.
Why I'm doing:
IVM (incremental materialized view) maintenance used to freeze the rewritten maintenance query into a persisted
ivmDefineSqlat CREATE time and execute that frozen text on every refresh. The rewritten query is the user query plus the hidden group-identity column__ROW_ID__(from_binary(encode_row_id(keys))) and, for aggregates, the mergeable__AGG_STATE_*columns.Because the rewrite was frozen, bugfixes to the rewriter could not reach already-created MVs. For example the positional-
GROUP BY/SELECT DISTINCT__ROW_ID__fix in #74493 only helped newly created MVs; existing MVs kept executing their stale, buggy frozen text and produced wrong results until dropped and re-created. The encode scheme was also baked into the SQL literal with no room to evolve.What I'm doing:
Re-derive the IVM maintenance query from the MV's original
viewDefineSqlon every refresh (re-running the current, fixed rewriter), instead of executing a frozen string. So rewriter fixes apply retroactively to existing MVs on their next refresh.Fixes #issue: N/A — internal IVM refresh-mechanism change (no separate tracking issue); makes rewriter fixes such as #74493 apply retroactively to existing MVs.
Mechanism
IvmRefreshDefinition.derive(ctx, mv)/deriveRewrittenQuery(ctx, mv): parseviewDefineSql→ analyze → re-run the IVM rewrite with the MV's pinnedencodeRowIdVersion(no trial) → analyze again → validate →buildSimple.ivmDefineSql(MaterializedViewAnalyzer/LocalMetastore); both refresh paths re-derive instead. The field is kept Gson-nullable for old metadata.RowIdStrategymust equal the stored one;IvmSchemaCompat): the re-derived output columns (incl. hidden__ROW_ID__/__AGG_STATE) must match the stored MV schema, reusing the CREATE-time column derivation so type equivalence stays identical.MVIVMRefreshProcessor) and the PCT full-rebuild path (MVPCTRefreshProcessor, used for an MV's first refresh / when no delta baseline exists). The IVM-MV predicate isisIncrementalOrAuto()so legacy AUTO-mode IVM MVs are handled too.MVRefreshSchemaChecker(external-base drift) re-derives the same way so its column comparison matches the stored hidden-column schema.ivmDefineSql, a one-time WARN is logged if the re-derived definition differs (advisory only; refresh always uses the re-derived result).Behavior change — existing IVM MVs created before this change carry a persisted
ivmDefineSql; their refresh now executes the re-derived query instead of that frozen text (this is exactly how rewriter fixes become retroactive). New IVM MVs persist no frozen text. PCT (non-IVM) refresh andSHOW CREATE MATERIALIZED VIEWare unchanged. The persistedTask.definitionof an IVM MV now displays the user query rather than the rewritten INSERT (display only — the executed plan is always re-derived). On downgrade to a pre-change binary, a new IVM MV (emptyivmDefineSql) falls back to the un-rewritten user query: for aggregate /DISTINCTMVs the old binary's own schema check rejects it with a clear error (base table schema check failed … column count N vs M, refresh fails — verified on a dev cluster), and a scan MV's user query equals its maintenance query so it refreshes correctly. Downgrade therefore fails loudly or stays correct — it does not silently produce wrong data. (Upgrade is smooth: an existing MV with a frozenivmDefineSqlre-derives on its next refresh — also verified on a dev cluster.)Verification
GROUP BYcol/ordinal,DISTINCT, all-constant, sum/max); retroactivity (frozen buggy text is ignored); strategy/shape gates; schema-checker no-false-drift for INCREMENTAL and AUTO MVs; the PCT first-refresh full-rebuild of an aggregate MV (validated to fail without the fix). Full IVM + MV suites green (239 tests), checkstyle clean.GROUP BY 1,2andSELECT DISTINCTMV both refresh to data that matches the direct query exactly (0 mismatched rows), through both the PCT full-rebuild and the IVM-delta path.What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: