yujun777 commented on code in PR #68390:
URL: https://github.com/apache/doris/pull/68390#discussion_r4128484606
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/NormalizeOlapTableStreamScan.java:
##########
@@ -262,6 +263,15 @@ private Plan makeSnapshotScan(LogicalOlapTableStreamScan
scan, CascadesContext c
OlapTable baseTable = streamWrapper.getBaseTable();
List<Slot> originSlots = scan.getOutput();
selectedPartitionIds =
streamWrapper.filterConsumedPartitionIds(selectedPartitionIds);
+ // What this read is about to answer with is recorded here, where the
plan is final and the read states
+ // are in place, and before the read is built: the refresh that reads
it back may not record a
+ // partition it replaced through an image of the table as of the
offset as caught up -- what it wrote
+ // is that image, and the delta that would bring the table up to date
does not apply to it. See
+ // MTMVTask#executePartitionBasedRefresh.
+ if (streamWrapper.readsSnapshotOfAnOlderImage(selectedPartitionIds)) {
Review Comment:
Right, and the reason it kept applying is that the branch I pointed at was
unreachable from where it runs. `answersWithTheCurrentTable` asks about a
partition the read dropped that holds rows, and the caller handed it the list
`filterConsumedPartitionIds` had already filtered -- so the partitions that
branch is about were not in it, and the judgement answered "yes, the table as
it is now" about a read that had just left rows out. Handing it the filtered
list asked a different question than its own contract states.
`e952c29714d` asks it before the filter, with the partitions the scan
selected: the filtering that has to be visible to the question is the wrapper's
own, which sees what it dropped. Nothing else moves -- every read below still
uses the filtered list.
What it costs, measured rather than reasoned, is in the suite added here: a
dimension that is empty when the MV's baseline is built, so its partition is
never given an offset; rows inserted into it afterwards; one fact partition
invalidated; and the strict refresh whose delta fails, which is the case you
asked to have covered. Reverted, that suite fails on the task row of the
refresh after it -- `NONE` with the untouched partition's scope and no rebuild,
because the failed refresh had recorded the rebuilt partition as caught up and
the refresh that follows sees nothing to do. In place, the rebuild is still
owed, its delta reads the dimension without the snapshot's offset and joins its
rows in, and the records the rebuild held back are published with it -- the
third refresh then reports nothing left.
Two things I checked before committing, since the change widens what is
withheld. The direction is one extra rebuild, never a missing row. The other
route it can reach is a whole-MV refresh, whose first batch reads the tables
through RESET rather than the snapshot: measured on a cluster, a `REFRESH
COMPLETE` in the same state lands the joined rows, and the `INCREMENTAL`
refreshes after it report no scope and no rebuild. And the suite carries
`nonConcurrent` (it drives a debug point), which the cloud pipeline skips, so
the sentinel-offset variant of this case is covered by inspection here rather
than end to end.
--
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]
