yujun777 commented on code in PR #68648:
URL: https://github.com/apache/doris/pull/68648#discussion_r4235991500
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVRewriteUtil.java:
##########
@@ -94,6 +111,18 @@ public static Collection<Partition>
getMTMVCanRewritePartitions(MTMV mtmv, Conne
return res;
}
}
+ if (mtmvNeedComparePartitions.isEmpty()) {
+ // A base partition the query reads is mapped from no MV
partition of this one -- a partition
+ // the mapping left out, an expired one for instance -- so no
MV partition holds its rows.
+ // None of them may answer this query, whatever the state of
the partitions in grace below:
+ // the answer is the base table.
+ return Lists.newArrayList();
+ }
+ if (gracePeriodMills > 0 && currentTimeMills <=
(partition.getVisibleVersionTime()
+ + gracePeriodMills) && !forceConsistent) {
Review Comment:
Fixed in 2da8d83c2cc. You are right that this is not the key-order question:
the keys genuinely expand, the alignment replaces the MV partition with an
empty one of the wider keys, and its creation time is as fresh as the partition
is empty.
Grace now needs the MV's refresh snapshot to describe the partition -- a
refresh to have recorded it at least once -- so a partition alignment has just
added is not answered for by it; it falls through to the sync check, which has
nothing to compare it with either. `MTMVRewriteUtilTest` pins both sides: a
partition in its grace period whose snapshot records it is still answered
usable, and one with no snapshot entry (the newly aligned replacement) is not.
A query over the keys the new partition is to hold is therefore answered
from the base table while the refresh runs, which is the safe direction; after
a successful refresh the partition has a snapshot and grace applies again as
before.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/UpdateMvByPartitionCommand.java:
##########
@@ -130,17 +142,69 @@ 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;
+ }
+ OlapTable olapTable = (OlapTable) table;
+ Set<PartitionItem> items = Sets.newHashSet();
+ for (String partitionName : readable) {
+
items.add(olapTable.getPartitionItemOrAnalysisException(partitionName));
+ }
+ if (items.stream().anyMatch(PartitionItem::isDefaultPartition)) {
+ // One of the partitions this MV partition is recorded with is
a list partitioned table's
+ // default partition, which takes the rows no other partition
of it claims. Those rows are
+ // the ones the MV partition's own key range names, wherever
the base table put them, and a
+ // partition of the MV takes them by that key rather than by
the partition they were placed
+ // in. So a table whose mapped partitions include one is read
the way an unscoped one is:
+ // the MV partition's key range, at the partition column's own
type. Which of the rows that
+ // range reaches sit in an explicit partition and which in the
default one is not something a
+ // predicate on the partition columns can say -- ADD PARTITION
claims a key without moving the
+ // rows a default partition already holds under it -- so this
table is not scoped at all, and
+ // the mapping is kept whole for it for the same reason: see
+ // MTMVRelatedPartitionDescSyncLimitGenerator, which leaves
such a table out of the window.
+ // The mapping names the default partition in every MV
partition that reads the table, so this
+ // is reached for each of them and not only for the one the
sentinel key maps to.
+ builder.put(table, constructPredicates(mvItems, colName,
+ Optional.of(partitionColumnType(olapTable, colName))));
+ continue;
+ }
+ if (readable.isEmpty()) {
Review Comment:
Fixed in e03d5690539, in the direction you point at: the batch now checks
that every MV partition it is to refresh is one the mapping describes, and
refuses to read it when it is not -- rather than reading it as nothing, which
is what wrote no rows over the rows the partition holds. The refusal makes the
task try again, and trying again realigns, so the partition and the mapping
built after it come from the same metadata state the alignment read.
That is the difference you name: a table the mapping gives no partition of
is read as nothing because it feeds nothing to the partition being refreshed,
while a partition no table describes at all is one the alignment has not caught
up with.
One thing I want to be straight about: the interleaving itself is a
lock-window race, and I did not reproduce it locally. What I ran for it is the
window-driven partition suites and the `mtmv_p0` set that refreshes by
partition (`test_limit_partition_mtmv`, `test_partition_limit_mtmv`, the
read-scope suite, the multi-pct ones and the multi-level replace one) -- all
pass, which says the check does not fire for a legitimate mapping, not that the
race is closed end to end. If you have a way to interleave an `ADD PARTITION`
between the alignment and the context build, that would be the end-to-end case
to add.
--
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]
