github-actions[bot] commented on code in PR #68646:
URL: https://github.com/apache/doris/pull/68646#discussion_r4133689417
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVRelationManager.java:
##########
@@ -502,10 +511,19 @@ private void processBaseTableChange(BaseTableInfo
baseTableInfo, String msgPrefi
LOG.warn(e);
continue;
}
- if (checkQueryUsable && invalidateMvIfQueryUnusable(baseTableInfo,
mvTable)) {
- // Invalidated with the reason, which is the more specific of
the two messages and the one
- // this change is worth recording: the state is the same one
the generic record below
- // would set, so writing it too would only bury the reason.
+ if (checkQueryUsable) {
Review Comment:
[P1] Check bound dependencies, not only whether the stored query analyzes. A
COMPLETE MV can store `SELECT o.id FROM outer_t o WHERE EXISTS (SELECT 1 FROM
inner_t i WHERE i.id=o.id AND flag=1)` with `flag` originally bound to `i.flag`
while `o.flag` also exists. Creation-time SQL rewriting does not enter
`EXISTS`'s subquery plan, so `flag` remains unqualified. After a light DROP of
`i.flag`, Nereids binds it to `o.flag`; output schema still matches, and this
branch leaves the old MV rows eligible for rewrite. Invalidate on changed bound
column identity or qualify slots inside nested subqueries before relying on
query success.
##########
fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java:
##########
@@ -373,8 +373,19 @@ private boolean
processAlterOlapTableInternal(List<AlterOp> alterOps, OlapTable
throw new DdlException("Invalid alter operations: " +
currentAlterOps);
}
if (needChangeMTMVState(alterOps)) {
- Env.getCurrentEnv().getMtmvService()
- .alterTable(oldBaseTableInfo, newBaseTableInfo,
currentAlterOps.hasReplaceTableOp());
+ // Whether the state a dependent MV ends in is decided by
re-analysing its query is the
+ // operation's to say, see AlterOp#needQueryUsabilityCheck -- and
the operation is asked whether
+ // the change it carries has reached the table, because the query
is analysed against the table
+ // as it is now. A schema change that is not a light one is
applied by a job, and where that job
+ // has not run yet the table is the one from before the change:
every query still analyses
+ // against it, and an invalidation decided on that answer would be
about the wrong table. A
+ // change that has not been seen that way keeps invalidating the
MVs that read the table, which
+ // is what it did before the queries were asked at all.
+ boolean judgeStateByQueryUsability =
alterOps.stream().anyMatch(AlterOp::needQueryUsabilityCheck)
+ &&
alterOps.stream().filter(AlterOp::needQueryUsabilityCheck)
+ .allMatch(op -> op.hasReachedTheTable(olapTable));
Review Comment:
[P1] Keep the reached check and MV re-analysis on one schema identity. After
a light `DROP COLUMN a` releases the table lock, this test can see `a` absent;
a concurrent light `ADD COLUMN a INT DEFAULT 0` can then commit before the MV
query is re-analyzed. ADD triggers no MV hook, and `SELECT k, a` now resolves
to the replacement column with the same type, so neither ALTER invalidates the
old MV rows. Serialize the decision with later schema changes or compare the
bound column identity from before the DROP.
##########
fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java:
##########
@@ -373,8 +373,19 @@ private boolean
processAlterOlapTableInternal(List<AlterOp> alterOps, OlapTable
throw new DdlException("Invalid alter operations: " +
currentAlterOps);
}
if (needChangeMTMVState(alterOps)) {
- Env.getCurrentEnv().getMtmvService()
- .alterTable(oldBaseTableInfo, newBaseTableInfo,
currentAlterOps.hasReplaceTableOp());
+ // Whether the state a dependent MV ends in is decided by
re-analysing its query is the
+ // operation's to say, see AlterOp#needQueryUsabilityCheck -- and
the operation is asked whether
+ // the change it carries has reached the table, because the query
is analysed against the table
+ // as it is now. A schema change that is not a light one is
applied by a job, and where that job
+ // has not run yet the table is the one from before the change:
every query still analyses
+ // against it, and an invalidation decided on that answer would be
about the wrong table. A
+ // change that has not been seen that way keeps invalidating the
MVs that read the table, which
+ // is what it did before the queries were asked at all.
+ boolean judgeStateByQueryUsability =
alterOps.stream().anyMatch(AlterOp::needQueryUsabilityCheck)
Review Comment:
[P1] Include non-query-only clauses in the invalidation decision. A legal
light `ALTER TABLE ... DROP COLUMN spare, MODIFY COLUMN payload
STRUCT<a:VARCHAR(10), b:INT>` leaves `spare` absent, so this predicate enables
query-only handling despite `ModifyColumnOp` requiring invalidation. An MV
projecting `CAST(to_json(payload) AS STRING)` still analyzes with the same
output type, but old struct rows now serialize with `b:null`; the new branch
skips invalidation and serves stale values. Require every state-changing clause
to qualify for the query check, or retain generic invalidation for mixed
batches.
##########
regression-test/suites/mtmv_p0/test_drop_unreferenced_column_mtmv.groovy:
##########
@@ -0,0 +1,134 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+import org.junit.Assert;
+
+/**
+ * Which base-table column changes invalidate a materialized view.
+ *
+ * <p>A dropped column is judged by re-analysing the MV's own query -- but
only where that question has an
+ * answer. The query is analysed against the table as it is when the alter
reaches the MV hook, and unless
+ * the change is a light one it has not been applied yet at that point: it was
submitted as a job, the
+ * column is still there, and every query still analyses. An invalidation
decided on that answer would be
+ * about the table from before the change, which is exactly where a dropped
and re-added column leaves the
+ * ABA the invalidation exists for. So a change that is not in place keeps
invalidating, and the two halves
+ * of this suite pin the two answers:
+ * <ol>
+ * <li>a merge-on-write table, where dropping a value column is a light
change: the MV whose query does
+ * not name the column is left alone, and the MV whose query names it is
invalidated;</li>
+ * <li>a duplicate table, where the same drop is not a light one and a job
does the data rewrite: the
+ * column is out of the table's schema before the hook runs all the
same, so the same answer holds.
+ * What the gate is for is the other side of that -- a change a job has
not applied yet, where the
+ * query would be analysed against the table from before it. There the
MV is invalidated, which is
+ * what every column change did before the queries were asked at
all.</li>
+ * </ol>
+ *
+ * <p>The second half is also where the consequence is observable: the rewrite
reaches the MV on that table
+ * and not on a merge-on-write one, so the state the change records is what is
left to report on the first.
+ * The IVM side of the same change, where the state is what escalates the next
refresh to a whole-MV
+ * COMPLETE, is pinned in the ivm directory.
+ */
+suite("test_drop_unreferenced_column_mtmv", "mtmv") {
+ String dbName = context.config.getDbNameByFile(context.file)
+ String suiteName = "test_drop_unreferenced_column_mtmv"
+
+ // ------------------------------------------------- 1. the change is in
place: the query decides
+ String mowTable = "${suiteName}_mow_table"
+ String mowMv = "${suiteName}_mow_mv"
+
+ sql """drop materialized view if exists ${mowMv}"""
+ sql """drop table if exists ${mowTable}"""
+ sql """
+ CREATE TABLE ${mowTable}
+ (
+ k1 INT NOT NULL,
+ amount BIGINT,
+ spare BIGINT
+ )
+ UNIQUE KEY(k1)
+ DISTRIBUTED BY HASH(k1) BUCKETS 2
+ PROPERTIES ("replication_num" = "1",
"enable_unique_key_merge_on_write" = "true")
+ """
+ sql """INSERT INTO ${mowTable} VALUES (1, 100, 7), (2, 200, 8)"""
+ sql """
+ CREATE MATERIALIZED VIEW ${mowMv}
+ BUILD DEFERRED REFRESH COMPLETE ON MANUAL
+ DISTRIBUTED BY HASH(k1) BUCKETS 2
+ PROPERTIES ("replication_num" = "1")
+ AS SELECT k1, SUM(amount) AS total FROM ${mowTable} GROUP BY k1
+ """
+ sql """REFRESH MATERIALIZED VIEW ${mowMv} COMPLETE"""
+ waitingMTMVTaskFinishedByMvName(mowMv)
+ order_qt_mow_baseline "SELECT k1, total FROM ${mowMv}"
+
+ // A column the query does not name. Dropping it gives this MV nothing to
recompute, so it is not
+ // invalidated: it stays a refresh candidate, and the state is where that
shows.
+ sql """ALTER TABLE ${mowTable} DROP COLUMN spare"""
+ assertEquals("FINISHED", getAlterColumnFinalState("${mowTable}"))
+ order_qt_mow_state_after_unreferenced_drop "select
Name,State,RefreshState,SyncWithBaseTables from
mv_infos('database'='${dbName}') where Name='${mowMv}'"
+
+ // A column the query names. The MV cannot be computed from the table any
more, so it is invalidated.
+ sql """ALTER TABLE ${mowTable} DROP COLUMN amount"""
+ assertEquals("FINISHED", getAlterColumnFinalState("${mowTable}"))
+ order_qt_mow_state_after_referenced_drop "select
Name,State,RefreshState,SyncWithBaseTables from
mv_infos('database'='${dbName}') where Name='${mowMv}'"
+ // Neither column was one the rows the MV holds depended on: the
invalidation above is not about the
+ // data being wrong, it is about the query the data stands for.
+ order_qt_mow_rows_after_both_drops "SELECT k1, total FROM ${mowMv}"
+
+ // ------------------------- 2. the same drop on a table whose data a job
rewrites: same answer
+ String dupTable = "${suiteName}_dup_table"
+ String dupMv = "${suiteName}_dup_mv"
+ String dupQuery = "SELECT k1, SUM(amount) AS total FROM ${dupTable} GROUP
BY k1"
+
+ sql """drop materialized view if exists ${dupMv}"""
+ sql """drop table if exists ${dupTable}"""
+ sql """
+ CREATE TABLE ${dupTable}
+ (
+ k1 INT,
+ amount BIGINT,
+ spare BIGINT
+ )
+ DUPLICATE KEY(k1)
+ DISTRIBUTED BY HASH(k1) BUCKETS 2
+ PROPERTIES ("replication_num" = "1")
Review Comment:
[P2] Make this case exercise the schema job it describes.
`light_schema_change` defaults to true, and `spare` is a nonkey value column on
this DUP_KEYS table, so this DROP takes `modifyTableLightSchemaChange`, just
like the first half of the suite. It never checks the fallback where the column
is still present at the MV hook and generic invalidation must occur. Set
`light_schema_change=false` for a separate nonlight case and assert
`SCHEMA_CHANGE`/rewrite exclusion; keep the current assertions in a light case
if desired.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
