github-actions[bot] commented on code in PR #68648:
URL: https://github.com/apache/doris/pull/68648#discussion_r4134420545
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/UpdateMvByPartitionCommand.java:
##########
@@ -130,17 +136,55 @@ private static List<String>
constructPartsForMv(Set<String> partitionNames) {
return Lists.newArrayList(partitionNames);
}
+ /**
+ * The predicate every base table of the MV definition is read through.
+ *
+ * <p>A table the caller scopes is read from exactly the base partitions
it named. Those are the ones
+ * the refresh is about to record as this MV partition's, and the read is
what has to match the record:
+ * reading the MV partition's own key range instead also reads base
partitions no snapshot describes,
+ * and a later silent change to one of them -- dropped, with the base
partition set back to what it
+ * was -- leaves the rows it put in this MV partition behind while the
partition is still judged
+ * synchronized, so the transparent rewrite serves them and no refresh
plans it again.
+ *
+ * <p>Every other table keeps the MV partition's own key range, which is
what the tables the caller
+ * does not scope were always read through. Scoped tables are olap ones;
the partition names are
+ * looked up on one, see the caller.
+ */
private static Map<TableIf, Set<Expression>>
constructTableWithPredicates(MTMV mv,
- Set<String> partitionNames, Map<TableIf, String> tableWithPartKey)
throws AnalysisException {
- Set<PartitionItem> items = Sets.newHashSet();
+ Set<String> partitionNames, Map<TableIf, String> tableWithPartKey,
+ Map<BaseTableInfo, Set<String>> readableBasePartitions) throws
AnalysisException {
+ Set<PartitionItem> mvItems = Sets.newHashSet();
for (String partitionName : partitionNames) {
- PartitionItem partitionItem =
mv.getPartitionItemOrAnalysisException(partitionName);
- items.add(partitionItem);
+ mvItems.add(mv.getPartitionItemOrAnalysisException(partitionName));
}
ImmutableMap.Builder<TableIf, Set<Expression>> builder = new
ImmutableMap.Builder<>();
- tableWithPartKey.forEach((table, colName) ->
- builder.put(table, constructPredicates(items, colName))
- );
+ for (Map.Entry<TableIf, String> entry : tableWithPartKey.entrySet()) {
+ TableIf table = entry.getKey();
+ String colName = entry.getValue();
+ Set<String> readable = readableBasePartitions == null ? null
+ : readableBasePartitions.get(new BaseTableInfo(table));
+ if (readable == null) {
+ builder.put(table, constructPredicates(mvItems, colName));
+ continue;
+ }
+ if (readable.isEmpty()) {
+ // No partition of this table feeds the MV partitions being
refreshed, which is "no row"
+ // rather than "every row": constructPredicates answers the
other way for an empty set,
+ // and that answer would put every row of the table into each
of them.
+ builder.put(table, Sets.newHashSet(BooleanLiteral.FALSE));
+ continue;
+ }
+ OlapTable olapTable = (OlapTable) table;
+ Set<PartitionItem> items = Sets.newHashSet();
+ for (String partitionName : readable) {
+
items.add(olapTable.getPartitionItemOrAnalysisException(partitionName));
+ }
+ // Built from the key at the position the MV's partition column
has in this table, which is
+ // what the mapping is keyed by; a partition of a list partitioned
table can hold more than
+ // one key, and the MV's column is not necessarily the first of
them.
+ builder.put(table, constructPredicates(items, new
UnboundSlot(colName),
Review Comment:
[P1] Restrict the scan to the mapped base partitions, not just their
projected partition-column values. For a valid `LIST(d, region)` table, let
expired `p_old` contain `(old, 'US')` and retained `p_kept` contain `(old,
'EU')` and `(new, 'EU')`. With `partition_sync_limit=2` and an MV partitioned
by `d`, the mapping and snapshot name only `p_kept`, but this predicate becomes
`d IN (old, new)` and still reads the `p_old` row. Dropping `p_old` later
leaves that row in the MV while the snapshot comparison sees only unchanged
`p_kept`, so rewrite can serve stale data. The old predicate had this hole too;
the new scope does not close it. Use exact base partition IDs or full LIST
tuples for the scoped read, and cover this overlap in a regression test.
##########
regression-test/suites/mtmv_p0/test_mtmv_base_partition_read_scope.groovy:
##########
@@ -0,0 +1,82 @@
+// 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
+
+suite("test_mtmv_base_partition_read_scope") {
+ // 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 assertion is against what
+ // the window keeps rather than a fixed number of rows: when today is
early enough in January that the
+ // kept days fall in the new year, the MV partition that covers the
expired day does not exist and both
+ // the MV and the expectation lose it -- the case is then not exercised,
but it is still asserted.
+ def today = java.time.LocalDate.now()
Review Comment:
[P2] Keep the expired and retained days in one MV year for every test run.
On January 2-5, `expiredDay` falls in the previous year while both retained
days are in the current year. The sync limit therefore creates only a
current-year MV partition, whose old key-range predicate already excludes
`expiredDay`; both assertions pass even if this PR's scoped-read change is
reverted. Please use a fixture or controlled clock that always leaves an
expired base partition inside a retained MV partition's year, so this
regression consistently fails without the fix.
--
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]
