yujun777 commented on PR #68648: URL: https://github.com/apache/doris/pull/68648#issuecomment-6380751995
The two P1s the review carries are still open on purpose, and the note's remedy for one of them is the reason I am not taking it here. For `Keep absent IVM PCT inputs inside the recorded batch scope`: an explicit empty reset scope is not a scoped read. `IvmFullRefreshMTMV#rewriteScan` branches on `resetPartitionIds.isPresent()`, so a present-but-empty set takes the `StreamReadMode.RESET` path with no partition selected, and the batch reads none of that table -- the join loses the rows it does produce for the keys the MV partition covers, which is a different wrong answer than the one the finding describes. The scope that would be right is the set the *read* takes, and for IVM that set has to move together with the stream baseline the COMPLETE establishes and with what the next INCREMENTAL reads the delta from (`generateRelatedBasePartitionIds`), so it is not one call site. That is the same fork as `Scope external PCT reads when merging LIST components`, one call site further along: an external PCT table is read through the MV-domain predicate, not the window, because `mappedBasePartitions` seeds only `OlapTable`s. Narrowing that read to the mapping drops the rows the join produces for a key the window kept on the other side -- the D0/D1 merge is exactly a component whose keys span partitions not all tables keep -- while recording what the read took puts a partition the window dropped into the mapping, which is what the sync check then compares and the next refresh then re-reads. Both sides of that fork change what the mapping means, and the mapping is the thing this change is built on. Both are reachable because this change admits a merged LIST component whose key set spans base partitions that not every input keeps; before it the overlapping descriptors were rejected and a batch never read beside a partition it does not record. I would rather land this change with the invariant it implements -- a read scoped to what the MV partition is recorded with, where the mapping can name it -- and take the two exceptions in a following change that decides what the mapping records for a partition the window left out, with a refresh -> remote DROP -> sync check case on the external side and a COMPLETE -> DELETE -> INCREMENTAL case on a merged IVM component to hold the decision down. -- 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]
