yujun777 commented on code in PR #68648:
URL: https://github.com/apache/doris/pull/68648#discussion_r4268302758
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVRewriteUtil.java:
##########
@@ -88,23 +135,49 @@ public static Collection<Partition>
getMTMVCanRewritePartitions(MTMV mtmv, Conne
if (mtmvNeedComparePartitions == null) {
try {
mtmvNeedComparePartitions =
getMtmvPartitionsByRelatedPartitions(mtmv, refreshContext,
- queryUsedPartitions);
+ queryUsedPartitions,
+
ctx.getSessionVariable().isEnableMaterializedViewUnionRewrite());
} catch (AnalysisException e) {
LOG.warn(e);
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();
+ }
+ // A partition that alignment has just added is as fresh as it is
empty: a base ADD PARTITION can
+ // widen the keys an MV partition covers, which drops the
populated one and adds an empty
+ // replacement, and its creation time would let the grace period
admit it before any refresh has
+ // read it -- a query rewritten to it then reads no rows at all,
including the ones the base
+ // partitions it is to hold carry. Grace answers for a partition a
refresh has written, which the
+ // version read above says, and for no other: neither the refresh
snapshot nor `hasData` can be
+ // asked -- the snapshot is keyed by the partition's name, and
distinct keys can generate the same
+ // name, while on cloud `hasData` answers true for a session that
turned empty-partition pruning
+ // off, whatever the partition's version is.
+ if (gracePeriodMills > 0 && currentTimeMills <=
(partition.getVisibleVersionTime()
+ + gracePeriodMills) && !forceConsistent
+ && writtenPartitionsInGrace.contains(partition.getName()))
{
+ res.add(partition);
+ continue;
+ }
// if the partition which query not used, should not compare
partition version
if (!mtmvNeedComparePartitions.contains(partition.getName())) {
continue;
}
if (partitionSnapshots == null) {
+ // Every partition that reaches the sync check needs its
snapshots prepared, and what keeps a
+ // partition from reaching it is grace -- read from the same
set the check above is made of,
+ // rather than from a second reading of the grace period. A
partition inside its window that no
+ // refresh has written falls through to be compared (that is
what the version read above is for),
+ // so leaving it out here would send that comparison to
snapshots nobody prepared.
Set<String> partitionsToPreload = Sets.newHashSet();
for (Partition candidate : allPartitions) {
- boolean withinGracePeriod = gracePeriodMills > 0
- && currentTimeMills <=
candidate.getVisibleVersionTime() + gracePeriodMills
- && !forceConsistent;
- if (!withinGracePeriod &&
mtmvNeedComparePartitions.contains(candidate.getName())) {
+ if (!writtenPartitionsInGrace.contains(candidate.getName())
Review Comment:
Fixed in 98cddbf9e16. You are right on both halves: the read is what writes
the cached mtime back (0 where the meta service has none), and the preload was
built from a second reading of the window rather than from what that read
reported.
The final grace set is now computed once, after the version read
(`writtenPartitionsInGrace` -> `answeredByGrace`), and it is the single set the
`continue` branch and the preload both read from. The loop no longer re-tests
the grace window at all, so a partition the read moved out of the window falls
through to the comparison and its snapshots are prepared along with the rest of
the compared set, rather than reaching `PreparedPartitionSnapshots.get` with
nothing prepared.
Covered by
`MTMVRewriteUtilTest#testGetMTMVCanRewritePartitionsWhenTheVersionReadMovesThePartitionOutOfGrace`:
`Partition.getVisibleVersions` both reports the partition written (2) and
rewrites its timestamp to 0 inside the same call, and the case asserts the
partition is compared --
`prepareComparablePartitionSnapshots(Sets.newHashSet("p1"))` is verified and
one partition is offered. The case fails with the decision taken before the
read. `org.apache.doris.mtmv.MTMVRewriteUtilTest` is 19 tests, all pass, and an
earlier case (`...InGracePeriodStillPreloadsWhatItCompares`) still covers the
partition within its window that no refresh has written.
--
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]
