yujun777 commented on code in PR #68390:
URL: https://github.com/apache/doris/pull/68390#discussion_r4083012340
##########
fe/fe-core/src/main/java/org/apache/doris/job/extensions/mtmv/MTMVTask.java:
##########
@@ -768,6 +749,9 @@ private IvmIncrRefreshResult
executeSingleIvmAttempt(MTMVRefreshContext refreshC
} catch (Exception e) {
throw new JobException("IVM snapshot generation failed for mv=" +
mtmv.getName(), e);
}
+ // The requirement these partitions are read under, captured before
the read inside doRefresh and
+ // recorded only if the refresh commits; see captureLatestEpochs.
+ Map<String, Long> capturedEpochs =
captureLatestEpochs(Sets.newHashSet(needRefreshPartitions));
Review Comment:
Implemented. The states are read once, and that one read decides both what
has to be rebuilt and the value each batch may write back:
`MTMVTask#executeIvmAttempt` keeps the plan-time `latestEpoch` per partition,
and `#plannedCeiling` clamps every captured epoch to it. An invalidation
landing between the routing decision and a batch's read therefore stays above
the value that batch records, so the partition is still rebuilt by the next
refresh instead of being written back as satisfied.
What is still missing is a test that pins the clamp: the window is a race,
so a deterministic one has to drive `commitCapturedEpochs` with a planned map
and a higher captured map. I will add that.
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVPartitionState.java:
##########
@@ -61,6 +61,43 @@ public MTMVPartitionState(MTMVPartitionState other) {
this.latestEpoch = other.latestEpoch;
}
+ /**
+ * The state a partition gets when it is first aligned: never refreshed,
one generation required.
+ *
+ * <p>Alignment only ever creates this pair, so "no entry yet" and "this
pair" say the same thing
+ * about the past -- the partition was never marked and holds no rows.
+ */
+ public static MTMVPartitionState initial() {
+ return new MTMVPartitionState(0, 1);
+ }
+
+ /**
+ * Whether this partition holds rows that a metadata-only change of a base
table has made unusable,
+ * so it has to be rebuilt rather than caught up incrementally.
+ *
+ * <p>{@code latestEpoch > refreshEpoch} alone does not say that: {@code
refreshEpoch == 0} means the
+ * partition was never refreshed, so it holds no rows and its first
refresh reads the current base
+ * tables anyway -- it cannot carry in rows that no longer exist. Calling
that dirty would rebuild
+ * every partition of a fresh MV for nothing.
+ *
+ * <p>The rejected alternative asked the partition itself instead ({@code
visibleVersion == 1} means
+ * "no data", and a rebuild would put {@code refreshEpoch} back to 0 for
such a partition). It is
+ * better in one way -- it does not rebuild a partition that is empty
because its base partitions are
+ * empty -- but it leaks silently as soon as an anti-join, {@code NOT IN}
or {@code NOT EXISTS}
+ * incremental refresh exists: removing input rows does not always remove
output rows (a left join
+ * filtered by {@code b.v IS NULL} becomes non-empty when the matching
rows go away), and that change
+ * emits no row binlog. Betting the criterion on today's operator support
is exactly what this state
+ * exists to avoid, so the conservative reading wins.
+ */
+ public boolean isDirty() {
+ return latestEpoch > refreshEpoch && refreshEpoch != 0;
Review Comment:
Implemented, and taken further than the finding asks: the exemption is gone
rather than guarded, so `isDirty()` is `latestEpoch > refreshEpoch`. The crash
cut closes on its own -- `{0, 2}` is dirty because `2 > 0` -- and the
in-progress flag that was briefly proposed for it is not needed.
`MTMVTest#testPartitionStateIsDirtyWhenItIsBehindItsRequirement` pins the
pairs, `(0, 1)` and `(0, 2)` among them, and
`MTMVTaskTest#testBuildAttemptsEscalatesToCompleteWhenEveryPartitionNeedsARebuild`
pins that `(0, 1)` counts as needing a rebuild -- under the old reading that
case kept the incremental chain instead.
--
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]
