yujun777 commented on code in PR #68648:
URL: https://github.com/apache/doris/pull/68648#discussion_r4267923868
##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/mtmv/MTMVTask.java:
##########
@@ -1493,6 +1501,34 @@ private void reconcileIvmStreams(ConnectContext ctx)
throws JobException {
}
}
+ /**
+ * Whether each MV partition of this batch is one the mapping describes,
i.e. one that some base partition's
+ * keys name. A partition nothing describes is one the alignment has not
caught up with: a base `ADD
+ * PARTITION` widens the keys an MV partition covers, which replaces it
with an empty partition of the new
+ * keys, and if that lands between the alignment and this mapping the
partition is read as no table feeding
+ * it and the batch writes no rows over the rows it holds. The batch is
not read at all then: the task tries
+ * again, and trying again realigns.
+ */
+ private void requireDescribedPartitions(MTMVRefreshContext context,
Set<String> execPartitionNames)
+ throws AnalysisException {
+ // Only when every table the MV is partitioned on is an olap one:
those are the tables a refresh is
+ // scoped to, and the ones whose descriptors are read from the tables
themselves. An external table's
+ // are read from a snapshot, and a refresh that cannot resolve one
would look like a partition nothing
+ // describes -- which is a state this guard has nothing to say about.
+ for (MTMVRelatedTableIf pctTable :
mtmv.getMvPartitionInfo().getPctTables()) {
+ if (!(pctTable instanceof OlapTable)) {
Review Comment:
Fixed in 29ab4ac42ca. The check is taken for every batch again: the skip is
removed rather than narrowed, because a non-olap PCT table does not make the
olap side of a mixed batch any less scoped, and you are right that
`mappedBasePartitions` scopes the olap tables whatever the other tables are.
Measured before dropping it: the refresh the skip had been added for,
`mtmv_p0/test_iceberg_mtmv`, passes with the check in place -- the state it was
avoiding does not reproduce -- and
`mtmv_p0/test_mtmv_base_partition_read_scope`,
`nereids_rules_p0/mv/union_rewrite_grace_big` and
`nereids_rules_p0/mv/partition_union_rewrite/partition_mv_rewrite` pass as well.
##########
fe/fe-core/src/test/java/org/apache/doris/mtmv/MTMVRewriteUtilTest.java:
##########
@@ -164,6 +169,156 @@ public void
testGetMTMVCanRewritePartitionsInGracePeriod() throws AnalysisExcept
Assertions.assertEquals(1, mtmvCanRewritePartitions.size());
}
+ @Test
+ public void
testGetMTMVCanRewritePartitionsInGracePeriodWithDataButNoSnapshot() throws
AnalysisException {
+ // A partition a refresh wrote is one grace may answer for even where
the snapshot has been lost with
+ // the table -- a rename recreates it: its version is one it was
written at, which is what grace reads.
+ Mockito.when(mtmv.getGracePeriod()).thenReturn(2L);
+
Mockito.when(refreshSnapshot.getPartitionSnapshots()).thenReturn(Maps.newHashMap());
+ Mockito.when(p1.getVisibleVersion()).thenReturn(2L);
+
+ mtmvPartitionUtilStatic.when(() ->
MTMVPartitionUtil.isMTMVPartitionSync(
+ Mockito.any(MTMVRefreshContext.class),
+
Mockito.any(MTMVRefreshContext.PreparedPartitionSnapshots.class),
Mockito.anyString(),
+ Mockito.any(Set.class),
+ Mockito.any(Set.class))).thenReturn(false);
+
+ Collection<Partition> mtmvCanRewritePartitions = MTMVRewriteUtil
+ .getMTMVCanRewritePartitions(mtmv, ctx, currentTimeMills,
false, null);
+ Assertions.assertEquals(1, mtmvCanRewritePartitions.size());
+ }
+
+ @Test
+ public void
testGetMTMVCanRewritePartitionsInGracePeriodForAPartitionNothingWrote() throws
AnalysisException {
+ // A partition alignment has just added is empty and no refresh has
written it, so its creation time
+ // says nothing about what it holds: grace must not answer for it, or
a query rewritten to it reads no
+ // rows at all. A snapshot entry under the same name must not either
-- distinct keys can generate the
+ // same partition name, so a drop and add can leave the replaced
partition's entry behind -- which is
+ // why grace reads the partition's version rather than the snapshot.
It falls through to the sync
+ // check, which has nothing to compare it with.
+ Mockito.when(mtmv.getGracePeriod()).thenReturn(2L);
+ Map<String, MTMVRefreshPartitionSnapshot> staleSnapshot =
Maps.newHashMap();
+ staleSnapshot.put("p1",
Mockito.mock(MTMVRefreshPartitionSnapshot.class));
+
Mockito.when(refreshSnapshot.getPartitionSnapshots()).thenReturn(staleSnapshot);
+
+ mtmvPartitionUtilStatic.when(() ->
MTMVPartitionUtil.isMTMVPartitionSync(
+ Mockito.any(MTMVRefreshContext.class),
+
Mockito.any(MTMVRefreshContext.PreparedPartitionSnapshots.class),
Mockito.anyString(),
+ Mockito.any(Set.class),
+ Mockito.any(Set.class))).thenReturn(false);
+
+ Collection<Partition> mtmvCanRewritePartitions = MTMVRewriteUtil
+ .getMTMVCanRewritePartitions(mtmv, ctx, currentTimeMills,
false, null);
+ Assertions.assertEquals(0, mtmvCanRewritePartitions.size());
+ }
+
+ @Test
+ public void
testGetMTMVCanRewritePartitionsInGracePeriodStillPreloadsWhatItCompares()
throws AnalysisException {
+ // The partition above falls through to the sync check, so its
snapshots have to be preloaded: the
+ // preload set and the set grace answers for have to come from one
reading of the grace period, or a
+ // partition inside the window that no refresh has written is compared
against snapshots nobody
+ // prepared -- which the sync check reports as "not synchronized",
dropping the partition and, with
+ // every partition in that state, the MV itself.
+ try (MockedStatic<MTMVRefreshContext> refreshContextStatic =
+ Mockito.mockStatic(MTMVRefreshContext.class)) {
+ MTMVRefreshContext context =
Mockito.mock(MTMVRefreshContext.class);
+ refreshContextStatic.when(() -> MTMVRefreshContext.buildContext(
+ Mockito.any(MTMV.class),
Mockito.anyMap())).thenReturn(context);
+
Mockito.when(context.prepareComparablePartitionSnapshots(Mockito.anySet()))
+
.thenReturn(Mockito.mock(MTMVRefreshContext.PreparedPartitionSnapshots.class));
+
+ Mockito.when(mtmv.getGracePeriod()).thenReturn(2L);
+ // Inside its window (creation 1 + grace 2 >= now 3) and written
by no refresh, so grace does not
+ // answer for it -- the version read is what tells the two apart.
+ Mockito.when(p1.getVisibleVersion()).thenReturn(0L);
+
+ Collection<Partition> mtmvCanRewritePartitions = MTMVRewriteUtil
+ .getMTMVCanRewritePartitions(mtmv, ctx, currentTimeMills,
false, null);
+
+ Assertions.assertEquals(1, mtmvCanRewritePartitions.size());
+
Mockito.verify(context).prepareComparablePartitionSnapshots(Sets.newHashSet("p1"));
+ }
+ }
+
+ @Test
+ public void
testGetMTMVCanRewritePartitionsInGracePeriodStillNeedsTheQueryCovered() throws
AnalysisException {
+ // A partition inside its grace period is answered usable without
being compared, but that says nothing
+ // about the base partitions the query reads. One that no MV partition
is mapped from is one whose rows
+ // the MV does not hold, so the coverage has to be read before the
grace period answers: taken after it,
+ // the query would be answered from the MV and that partition's rows
dropped.
+ try (MockedStatic<MTMVRefreshContext> refreshContextStatic =
+ Mockito.mockStatic(MTMVRefreshContext.class)) {
+ MTMVRefreshContext context =
Mockito.mock(MTMVRefreshContext.class);
+ refreshContextStatic.when(() -> MTMVRefreshContext.buildContext(
+ Mockito.any(MTMV.class),
Mockito.anyMap())).thenReturn(context);
+
Mockito.when(context.getPartitionMappings()).thenReturn(Maps.newHashMap());
+
+ MTMVRelatedTableIf pctTable =
Mockito.mock(MTMVRelatedTableIf.class);
+ List<String> qualifier = Lists.newArrayList("internal", "test",
"base");
+ Mockito.when(pctTable.getFullQualifiers()).thenReturn(qualifier);
+
Mockito.when(mvPartitionInfo.getPartitionType()).thenReturn(MTMVPartitionType.EXPR);
+
Mockito.when(mvPartitionInfo.getPctTables()).thenReturn(Sets.newHashSet(pctTable));
+ mtmvUtilStatic.when(() ->
MTMVUtil.getTable(qualifier)).thenReturn(pctTable);
+
+ Mockito.when(mtmv.getGracePeriod()).thenReturn(2L);
Review Comment:
Fixed in 25b82d97ff5. The case stubs a written version (2L), so grace admits
the partition on its own and the coverage check is what rejects it -- the
ordering it is about, and the one the fixture did not reach before.
Checked that it pins it: with the coverage check moved back below the grace
branch the case now fails (`expected: <0> but was: <1>`), where that change
passed it before. `org.apache.doris.mtmv.MTMVRewriteUtilTest` is 18 tests, all
pass.
--
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]
