github-actions[bot] commented on code in PR #68648: URL: https://github.com/apache/doris/pull/68648#discussion_r4142955457
########## regression-test/suites/mtmv_p0/test_mtmv_base_partition_read_scope.groovy: ########## @@ -0,0 +1,224 @@ +// 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_mtmv_base_partition_read_scope") { + String dbName = context.config.getDbNameByFile(context.file) + // A refresh reads the base partitions the MV partition is recorded with, and no others. The window of + // partition_sync_limit below keeps the last two days, so the day before them is recorded nowhere: it + // must not be read either, or its rows would sit in the MV partition -- whose key range does cover + // them -- while the snapshot says the MV does not hold that partition. A base partition dropped after + // such a read would then be invisible to the sync check, and the transparent rewrite would serve the + // rows of a partition the base table no longer has. + // + // The MV partition is a year and the base table's are days, so the range it is read through is wider + // than what it is recorded with. Days are taken relative to today, and the two sides are asserted + // separately rather than as a total, so that where the window's edge falls does not decide the case. + def today = java.time.LocalDate.now() Review Comment: [P2] Use the FE refresh calendar for this date-window fixture. LocalDate.now() uses the regression runner's default time zone, while partition_sync_limit computes its cutoff from DateTimeAcquire.now() in the FE session time zone. If the runner is on September 30 and FE is on October 1 (or setup crosses midnight), p_kept has an upper bound equal to FE's cutoff and is filtered out, so scope_kept_days returns 1 against the fixed golden value 2. Derive and hold the fixture date in the same calendar as the refresh, or recheck the boundary before asserting it. ########## fe/fe-core/src/main/java/org/apache/doris/catalog/MTMV.java: ########## @@ -1608,9 +1608,11 @@ public Map<String, Map<MTMVRelatedTableIf, Set<String>>> calculatePartitionMappi Map<PartitionKeyDesc, Map<MTMVRelatedTableIf, Set<String>>> pctPartitionDescs = MTMVPartitionUtil .generateRelatedPartitionDescs(mvPartitionInfo, mvProperties, getPartitionColumns(), effectiveFilter, pinnedSnapshots); + Map<MTMVRelatedTableIf, String> defaultListPartitions = defaultListPartitionsOf(); for (Entry<String, PartitionItem> entry : mvPartitionItems.entrySet()) { - res.put(entry.getKey(), - pctPartitionDescs.getOrDefault(entry.getValue().toPartitionKeyDesc(), Maps.newHashMap())); + res.put(entry.getKey(), withDefaultListPartitions( Review Comment: [P1] Check every MV partition mapped from a default LIST base partition before allowing rewrite. This fan-out puts p_default in multiple MV mappings, but MTMVRewriteUtil.getPctToMv inverts them into one String and keeps only an arbitrary destination. With explicit LIST tuples (2020,2) and (2021,2) plus default rows for (2020,3) and (2021,3), an insert into p_default dirties both MV partitions. Refresh only the inverse-selected partition, then query the other default-only tuple: Filter(d=other AND k=3) -> Scan(base[p_default]) can rewrite to Filter(d=other AND k=3) -> Scan(MV[other stale partition]) because eligibility checks only the refreshed destination. Setting enable_materialized_view_union_rewrite=false skips the later invalid-partition removal, so this can return stale data. Preserve the full inverse set or reject ambiguous default mappings for rewrite. ########## regression-test/suites/mtmv_p0/test_mtmv_base_partition_read_scope.groovy: ########## @@ -0,0 +1,224 @@ +// 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_mtmv_base_partition_read_scope") { + String dbName = context.config.getDbNameByFile(context.file) + // A refresh reads the base partitions the MV partition is recorded with, and no others. The window of + // partition_sync_limit below keeps the last two days, so the day before them is recorded nowhere: it + // must not be read either, or its rows would sit in the MV partition -- whose key range does cover + // them -- while the snapshot says the MV does not hold that partition. A base partition dropped after + // such a read would then be invisible to the sync check, and the transparent rewrite would serve the + // rows of a partition the base table no longer has. + // + // The MV partition is a year and the base table's are days, so the range it is read through is wider + // than what it is recorded with. Days are taken relative to today, and the two sides are asserted + // separately rather than as a total, so that where the window's edge falls does not decide the case. + def today = java.time.LocalDate.now() + def expiredDay = today.minusDays(5) + def keptDay = today.minusDays(1) + + // The expired day is read by a refresh only while it falls inside a retained MV partition's range, and + // both days are inside the same year only from the sixth of January on. The part below is left out + // where they are not, rather than asserted on days the change cannot be told apart on -- and rather + // than through an assumption, which this runner records as a failure of the suite, taking the calendar + // independent parts of it down with it. + if (expiredDay.getYear() == today.getYear()) { + sql """drop materialized view if exists mv_read_scope""" + sql """drop table if exists base_read_scope""" + sql """ + create table base_read_scope ( + k1 date not null, + value int not null + ) duplicate key(k1) + partition by range(k1) ( + partition p_expired values [("${expiredDay}"), ("${expiredDay.plusDays(1)}")), + partition p_kept values [("${keptDay}"), ("${keptDay.plusDays(1)}")), + partition p_today values [("${today}"), ("${today.plusDays(1)}")) + ) + distributed by hash(k1) buckets 1 + properties("replication_num" = "1") + """ + sql """insert into base_read_scope values ("${expiredDay}", 1), ("${keptDay}", 2), ("${today}", 3)""" + + sql """ + create materialized view mv_read_scope + build immediate refresh complete on manual + partition by (date_trunc(k1,'year')) + distributed by random buckets 1 + properties( + "replication_num" = "1", + "partition_sync_limit" = "2", + "partition_sync_time_unit" = "DAY" + ) + as select k1, sum(value) as total from base_read_scope group by k1 + """ + sql """refresh materialized view mv_read_scope complete""" + waitingMTMVTaskFinishedByMvName("mv_read_scope") + // The expectations below are read from the base table, so they must not be answered from the MV. + sql """set enable_materialized_view_rewrite = false""" + + // The data the refresh had to work with, so that the count below is not read as an empty base table. + order_qt_base_rows """select count(*) from base_read_scope""" + + // The day the window left out is the one missing from the MV, and the days it kept are there: read + // separately rather than as a total, so that the case does not turn on where the window's edge falls. + order_qt_scope_expired_day "select count(*) from mv_read_scope where k1 = '${expiredDay}'" + order_qt_scope_kept_days "select count(*) from mv_read_scope where k1 >= '${keptDay}'" + } + + // A partition of a list partitioned table holds one key per partition column, and a read pinned to one + // of those columns reaches the partitions that differ in the others. The expired partition below shares + // its `d` with a kept one, so a read pinned to `d` alone reads it while the snapshot names only the kept + // partition: a later drop of it would leave its rows in the MV unaccounted for. + sql """drop materialized view if exists list_scope_mv""" + sql """drop table if exists list_scope_base""" + sql """ + CREATE TABLE list_scope_base (d DATE NOT NULL, region VARCHAR(10) NOT NULL, amount BIGINT) + DUPLICATE KEY(d, region) + PARTITION BY LIST(d, region) ( + PARTITION p_expired VALUES IN ((\"2020-01-01\", \"US\")), + PARTITION p_kept VALUES IN ((\"2020-01-01\", \"EU\"), (\"2038-01-01\", \"EU\")) + ) + DISTRIBUTED BY HASH(d) BUCKETS 1 PROPERTIES (\"replication_num\" = \"1\") + """ + sql """INSERT INTO list_scope_base VALUES + (\"2020-01-01\", \"US\", 1), (\"2020-01-01\", \"EU\", 2), (\"2038-01-01\", \"EU\", 3)""" + sql """ + CREATE MATERIALIZED VIEW list_scope_mv + BUILD IMMEDIATE REFRESH COMPLETE ON MANUAL + PARTITION BY (d) + DISTRIBUTED BY HASH(d) BUCKETS 1 PROPERTIES (\"replication_num\" = \"1\", + \"partition_sync_limit\" = \"2\", \"partition_sync_time_unit\" = \"YEAR\") + AS SELECT d, region, SUM(amount) AS total FROM list_scope_base GROUP BY d, region + """ + waitingMTMVTaskFinishedByMvName("list_scope_mv") + order_qt_list_scope "SELECT d, region, total FROM list_scope_mv" + + // Two shapes cannot be pinned to what the base partitions hold, and both are read the way the MV + // partition's own key range reads them -- a reading that can be seen to be too wide, rather than one + // that leaves rows of the MV partition unread. + // + // The first is a list partitioned table's default partition, which takes the rows no other partition of + // it claims: the row below is in it while its `d` puts it in the MV partition the explicit partition is + // read through, so pinning the read to that partition's key would lose it. + sql """drop materialized view if exists list_default_mv""" + sql """drop table if exists list_default_base""" + sql """ + CREATE TABLE list_default_base (d DATE NOT NULL, k INT NOT NULL, amount BIGINT) + DUPLICATE KEY(d, k) + PARTITION BY LIST(d, k) ( + PARTITION p_explicit VALUES IN ((\"2020-01-01\", 2)), + PARTITION p_default + ) + DISTRIBUTED BY HASH(d) BUCKETS 1 PROPERTIES (\"replication_num\" = \"1\") + """ + sql """INSERT INTO list_default_base VALUES (\"2020-01-01\", 2, 2), (\"2020-01-01\", 3, 1)""" + sql """ + CREATE MATERIALIZED VIEW list_default_mv + BUILD IMMEDIATE REFRESH COMPLETE ON MANUAL + PARTITION BY (d) + DISTRIBUTED BY HASH(d) BUCKETS 1 PROPERTIES (\"replication_num\" = \"1\") + AS SELECT d, k, SUM(amount) AS total FROM list_default_base GROUP BY d, k + """ + waitingMTMVTaskFinishedByMvName("list_default_mv") + order_qt_list_default "SELECT d, k, total FROM list_default_mv" + // And the rows of that partition are read for every MV partition they belong to, so the MV partition is + // recorded with the partition they come from: a row inserted into it afterwards is a change the MV + // compares, rather than one it calls itself synchronized through. + sql """INSERT INTO list_default_base VALUES (\"2020-01-01\", 4, 7)""" + order_qt_list_default_tracked "select SyncWithBaseTables from mv_infos('database'='${dbName}') where Name='list_default_mv'" Review Comment: [P2] Assert the default dependency on the 2020 MV partition itself. Before the fan-out change, p_default still mapped to the separate MIN MV partition. This insert makes MIN unsynchronized, so mv_infos.SyncWithBaseTables becomes false even while the 2020 partition still omits p_default from its snapshot. The new golden therefore passes with the tracking fix reverted. Check the 2020 partition's sync state before and after the insert, or refresh again and assert that its new row appears. -- 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]
