github-actions[bot] commented on code in PR #67172: URL: https://github.com/apache/doris/pull/67172#discussion_r3885467045
########## regression-test/suites/mtmv_p0/test_dml_materialized_view_rewrite_global.groovy: ########## @@ -0,0 +1,72 @@ +// 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. + +suite("test_dml_materialized_view_rewrite_global", "p0,mtmv,nonConcurrent") { + String dbName = context.config.getDbNameByFile(context.file) + String originalDmlRewrite = sql_return_maparray( + "show global variables like 'enable_dml_materialized_view_rewrite'")[0].Value.toString() + + sql "drop materialized view if exists dml_rewrite_global_candidate_mv" + sql "drop materialized view if exists dml_rewrite_global_target_mv" + sql "drop table if exists dml_rewrite_global_base" + + sql """ + create table dml_rewrite_global_base ( + k1 int, + v1 int + ) + duplicate key(k1) + distributed by hash(k1) buckets 1 + properties ('replication_num' = '1') + """ + sql "insert into dml_rewrite_global_base values (1, 10), (2, 20)" + + sql """ + create materialized view dml_rewrite_global_candidate_mv + build deferred refresh complete on manual + distributed by random buckets 1 + properties ('replication_num' = '1') + as select k1, sum(v1) as total from dml_rewrite_global_base group by k1 + """ + sql "refresh materialized view dml_rewrite_global_candidate_mv complete" + waitingMTMVTaskFinishedByMvName("dml_rewrite_global_candidate_mv", dbName) + + sql """ + create materialized view dml_rewrite_global_target_mv + build deferred refresh complete on manual + distributed by random buckets 1 + properties ('replication_num' = '1') + as select k1, sum(v1) as total from dml_rewrite_global_base group by k1 + """ + + setGlobalVarTemporary([enable_dml_materialized_view_rewrite: false], { + connect(context.config.jdbcUser, context.config.jdbcPassword, context.config.jdbcUrl) { + def sessionValue = sql_return_maparray( + "show variables like 'enable_dml_materialized_view_rewrite'")[0].Value.toString() + assertEquals("false", sessionValue.toLowerCase()) + sql "set enable_materialized_view_rewrite=true" + sql "refresh materialized view dml_rewrite_global_target_mv complete" + } + }) Review Comment: [P2] Wait for the refresh before restoring the global flag `setGlobalVarTemporary` restores the global as soon as this closure returns, but the wait is outside it. The asynchronous MTMV task creates fresh internal `ConnectContext` instances from the then-current global defaults in both `run` and `exec`, so it can observe the restored DML-rewrite value instead of `false`. Because the candidate and target MVs produce identical rows, the final data assertion cannot detect that. Keep the task wait inside the temporary-global scope and add task-level evidence for the disabled rewrite path. ########## regression-test/suites/mtmv_p0/test_dml_materialized_view_rewrite_global.groovy: ########## @@ -0,0 +1,72 @@ +// 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. + +suite("test_dml_materialized_view_rewrite_global", "p0,mtmv,nonConcurrent") { + String dbName = context.config.getDbNameByFile(context.file) + String originalDmlRewrite = sql_return_maparray( + "show global variables like 'enable_dml_materialized_view_rewrite'")[0].Value.toString() + + sql "drop materialized view if exists dml_rewrite_global_candidate_mv" + sql "drop materialized view if exists dml_rewrite_global_target_mv" + sql "drop table if exists dml_rewrite_global_base" + + sql """ + create table dml_rewrite_global_base ( + k1 int, + v1 int + ) + duplicate key(k1) + distributed by hash(k1) buckets 1 + properties ('replication_num' = '1') + """ + sql "insert into dml_rewrite_global_base values (1, 10), (2, 20)" + + sql """ + create materialized view dml_rewrite_global_candidate_mv + build deferred refresh complete on manual + distributed by random buckets 1 + properties ('replication_num' = '1') + as select k1, sum(v1) as total from dml_rewrite_global_base group by k1 + """ + sql "refresh materialized view dml_rewrite_global_candidate_mv complete" + waitingMTMVTaskFinishedByMvName("dml_rewrite_global_candidate_mv", dbName) + + sql """ + create materialized view dml_rewrite_global_target_mv + build deferred refresh complete on manual + distributed by random buckets 1 + properties ('replication_num' = '1') + as select k1, sum(v1) as total from dml_rewrite_global_base group by k1 + """ + + setGlobalVarTemporary([enable_dml_materialized_view_rewrite: false], { + connect(context.config.jdbcUser, context.config.jdbcPassword, context.config.jdbcUrl) { Review Comment: [P1] Select the suite database on this fresh connection `connect` opens the supplied URL verbatim, while the standard `context.config.jdbcUrl` has no database component. Unlike the suite's normal connection path, this closure never rebuilds the URL with `dbName` or executes `USE`, so the unqualified `REFRESH MATERIALIZED VIEW dml_rewrite_global_target_mv` fails with no database selected before it can exercise the flag. Please connect with a DB-scoped URL or select `${dbName}` first. ########## regression-test/suites/mtmv_p0/test_dml_materialized_view_rewrite_global.groovy: ########## @@ -0,0 +1,72 @@ +// 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. + +suite("test_dml_materialized_view_rewrite_global", "p0,mtmv,nonConcurrent") { + String dbName = context.config.getDbNameByFile(context.file) + String originalDmlRewrite = sql_return_maparray( + "show global variables like 'enable_dml_materialized_view_rewrite'")[0].Value.toString() + + sql "drop materialized view if exists dml_rewrite_global_candidate_mv" + sql "drop materialized view if exists dml_rewrite_global_target_mv" + sql "drop table if exists dml_rewrite_global_base" + + sql """ + create table dml_rewrite_global_base ( + k1 int, + v1 int + ) + duplicate key(k1) + distributed by hash(k1) buckets 1 + properties ('replication_num' = '1') + """ + sql "insert into dml_rewrite_global_base values (1, 10), (2, 20)" + + sql """ + create materialized view dml_rewrite_global_candidate_mv + build deferred refresh complete on manual + distributed by random buckets 1 + properties ('replication_num' = '1') + as select k1, sum(v1) as total from dml_rewrite_global_base group by k1 + """ + sql "refresh materialized view dml_rewrite_global_candidate_mv complete" + waitingMTMVTaskFinishedByMvName("dml_rewrite_global_candidate_mv", dbName) + + sql """ + create materialized view dml_rewrite_global_target_mv + build deferred refresh complete on manual + distributed by random buckets 1 + properties ('replication_num' = '1') + as select k1, sum(v1) as total from dml_rewrite_global_base group by k1 + """ + + setGlobalVarTemporary([enable_dml_materialized_view_rewrite: false], { + connect(context.config.jdbcUser, context.config.jdbcPassword, context.config.jdbcUrl) { + def sessionValue = sql_return_maparray( + "show variables like 'enable_dml_materialized_view_rewrite'")[0].Value.toString() + assertEquals("false", sessionValue.toLowerCase()) + sql "set enable_materialized_view_rewrite=true" + sql "refresh materialized view dml_rewrite_global_target_mv complete" + } + }) + + waitingMTMVTaskFinishedByMvName("dml_rewrite_global_target_mv", dbName) + order_qt_target_data "select k1, total from dml_rewrite_global_target_mv order by k1" Review Comment: [P1] Commit the generated expected output for this query test This new `order_qt_target_data` assertion requires `regression-test/data/mtmv_p0/test_dml_materialized_view_rewrite_global.out`, but that file is absent from both the authoritative changed-file list and the checkout. The regression framework derives that exact path and opens it before comparing rows, so the suite cannot pass normal verification. Please generate the result with the prescribed regression runner and commit the generated `.out` file. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/rules/exploration/mv/AbstractMaterializedViewRule.java: ########## @@ -471,6 +470,60 @@ protected List<Plan> doRewrite(StructInfo queryStructInfo, CascadesContext casca return rewriteResults; } + // Partition compensation only supports one global limit/topN in each plan. When this invariant + // is not met, return null so the caller skips the rewrite instead of changing query semantics. + Plan buildPartitionCompensationPlan(Plan rewrittenPlan, Plan baseTablePlan, Plan queryPlan) { + List<Plan> queryGlobalLimits = queryPlan.collectToList(node -> isGlobalLimitOrTopN((Plan) node)); + List<Plan> rewrittenGlobalLimits = rewrittenPlan.collectToList( + node -> isGlobalLimitOrTopN((Plan) node)); + List<Plan> baseTableGlobalLimits = baseTablePlan.collectToList( + node -> isGlobalLimitOrTopN((Plan) node)); + if (queryGlobalLimits.isEmpty()) { + return rewrittenGlobalLimits.isEmpty() && baseTableGlobalLimits.isEmpty() + ? buildCompensationUnion(queryPlan, Lists.newArrayList(rewrittenPlan, baseTablePlan)) : null; + } + if (queryGlobalLimits.size() != 1 + || rewrittenGlobalLimits.size() != 1 || baseTableGlobalLimits.size() != 1) { + return null; + } + Plan queryGlobalLimit = queryGlobalLimits.get(0); + Plan rewrittenGlobalLimit = rewrittenGlobalLimits.get(0); + Plan baseTableGlobalLimit = baseTableGlobalLimits.get(0); + // Only remove a root global operator. An outer Project can carry expressions that differ + // between the query and MV branches, so unioning below it by output position is unsafe. + if (queryGlobalLimit != queryPlan || rewrittenGlobalLimit != rewrittenPlan + || baseTableGlobalLimit != baseTablePlan + || rewrittenGlobalLimit.getType() != queryGlobalLimit.getType() + || baseTableGlobalLimit.getType() != queryGlobalLimit.getType() + || getOffset(rewrittenGlobalLimit) != getOffset(queryGlobalLimit) + || getOffset(baseTableGlobalLimit) != getOffset(queryGlobalLimit)) { + return null; + } + Plan compensationUnion = buildCompensationUnion(queryGlobalLimit.child(0), Lists.newArrayList( Review Comment: [P1] Do not compensate from individually truncated batch partitions A valid MV partition is not necessarily a sufficient TopN prefix when refreshes batch partitions together. For example, with `refresh_partition_num=2`, `p1={100}`, `p2={90,80}`, and `ORDER BY v DESC LIMIT 2`, one refresh stores `p1={100}` and only `p2={90}`. If p1 is later updated to `1`, only p1 becomes stale, so this code builds: ```text TopN(v DESC, 2) UnionAll MVScan[p2] = {90} BaseScan[p1] = {1} ``` That returns `{90,1}`, while the base query returns `{90,80}`. The root/type/offset checks cannot recover the p2 row discarded by the earlier batched refresh. Please preserve refresh-batch provenance and invalidate/compensate the whole batch together, or materialize the required prefix independently per MV partition; add a stale-sibling regression with `refresh_partition_num=2`. Note that the property is mutable, so checking only its current value does not identify historically co-batched partitions. -- 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]
