github-actions[bot] commented on code in PR #68648:
URL: https://github.com/apache/doris/pull/68648#discussion_r4266750101
##########
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:
[P1] Keep descriptor-miss protection for mixed PCT refreshes
An external PCT table does not prevent the OLAP table from receiving the new
scope. A supported example is OLAP A with LIST(d,k), p1=((1,1)), joined on d to
Paimon E with LIST(d) and a matching d=1 row; the MV partition {1} is
populated. If A adds p12=((1,2),(2,2)) after alignment releases its base locks
but before buildRefreshContext, the merged descriptor becomes {1,2}. The old
{1} is marked undescribed and mappedBasePartitions gives A an empty set, but
this return skips the check because E is external. The refresh then plans:
InsertOverwrite(MV[{1}])
Join(a.d = e.d)
Filter(FALSE) -> Scan(A)
Filter(e.d IN (1)) -> Scan(E)
A successful COMPLETE refresh therefore erases the stored d=1 row while both
base tables still contain it. This is a distinct mixed-PCT bypass of the
safeguard added for the existing all-OLAP threads. Reject or realign an
undescribed MV partition whenever a scoped OLAP read is involved; unresolved
external metadata should not disable that protection for the whole batch.
##########
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:
[P2] Make the uncovered-query test exercise written-partition grace
This test sets the grace period but never stubs p1.getVisibleVersion(), so
the Mockito partition returns 0. The new eligibility code only adds versions
above PARTITION_INIT_VERSION (1) to writtenPartitionsInGrace, and this fixture
therefore cannot enter the grace continue branch. Moving the coverage check
back below grace, which restores the dropped-row bug this test is meant to
prevent, still passes the zero-partition assertion here. Stub a written version
(for example 2L, as in the neighboring positive grace test) so the uncovered
query must reject a partition that grace would otherwise admit.
--
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]
