yujun777 commented on code in PR #68648:
URL: https://github.com/apache/doris/pull/68648#discussion_r4140362815
##########
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:
Fixed in 43be1b3ba23. The scope is now built from the partitions themselves
rather than from the value they take at one column: a list partition is pinned
to the whole of each key it holds (each key a conjunction, `IS NULL` for a
value a row does not have), and a range partition to its bounds, which is the
same thing since a range partitioned base table has a single partition column.
The `LIST(d, region)` shape you describe is a regression case now. With the
change the MV holds only the kept partition's rows; with the predicate reverted
to the projected one it reads the expired partition's row, measured as
`RealRow: [2020-01-01, US, 1]` against a golden that expects only the `EU` rows.
One shape is deliberately left as it was: a list partitioned table's default
partition, which holds no key at all. What it holds cannot be said with a
predicate on the partition columns, so it is read in full as before -- a read
that can be seen to be too wide, rather than one that loses its rows quietly.
##########
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:
Fixed in 43be1b3ba23. You are right, and the fixture turned out to rest on
an assumption of mine rather than on a measurement: while fixing it I measured
the property, and `partition_sync_limit=1 DAY` expires nothing at all (six days
of partitions all kept), while `limit=2 DAY` puts its edge at today-2. So both
the number of retained days and the identity of the expired one were guesses.
The two sides are now asserted separately through `order_qt` -- the expired
day must be missing from the MV, the days the window kept must be there -- so a
total cannot let them cancel out, and the case is skipped with `assumeTrue`
where the calendar does not leave the expired day inside a retained MV
partition. Skipped rather than asserted, so that a run which exercises the case
is a run that can fail without the change.
--
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]
