yujun777 commented on code in PR #68648:
URL: https://github.com/apache/doris/pull/68648#discussion_r4236111894
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/MTMV.java:
##########
@@ -1608,9 +1608,11 @@ public Map<String, Map<MTMVRelatedTableIf, Set<String>>>
calculatePartitionMappi
Map<PartitionKeyDesc, Map<MTMVRelatedTableIf, Set<String>>>
pctPartitionDescs = MTMVPartitionUtil
.generateRelatedPartitionDescs(mvPartitionInfo, mvProperties,
getPartitionColumns(),
effectiveFilter, pinnedSnapshots);
+ Map<MTMVRelatedTableIf, String> defaultListPartitions =
defaultListPartitionsOf();
for (Entry<String, PartitionItem> entry : mvPartitionItems.entrySet())
{
- res.put(entry.getKey(),
-
pctPartitionDescs.getOrDefault(entry.getValue().toPartitionKeyDesc(),
Maps.newHashMap()));
+ res.put(entry.getKey(), withDefaultListPartitions(
+
pctPartitionDescs.getOrDefault(entry.getValue().toPartitionKeyDesc(),
Maps.newHashMap()),
Review Comment:
Fixed in 04a397b5d72. You are right that the fan-out made the check pass:
`withDefaultListPartitions` names a default list partition in every MV
partition's mapping, so on the two-table shape the mapping of the MV partition
whose descriptor the alignment has not caught up with is non-empty -- B.default
is in it -- while A, whose keys it holds, maps it to nothing, and the batch
read A as nothing.
The check now asks the narrower question: whether the MV partition's own
descriptor is one the base partitions produce at all.
`MTMV#calculatePartitionMappings` reports the partitions no descriptor matches,
before the fan-out, the refresh context carries that set, and the refresh
refuses to read those partitions. That separates the state you describe from a
table that really feeds nothing, which is what the earlier thread protected.
The descriptor-miss state itself is the interleaving you name and I did not
reproduce it locally; what the change is verified against is that a legitimate
mapping -- including the empty ones from `mtmv_p0`'s two-table and
default-partition suites and `partition_mv_rewrite` -- does not trip it.
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVRewriteUtil.java:
##########
@@ -166,25 +231,46 @@ private static Set<String>
getMtmvPartitionsByRelatedPartitions(MTMV mtmv, MTMVR
}
Set<String> pctPartitions = entry.getValue();
for (String pctPartition : pctPartitions) {
- String mvPartition = relatedToMv.get(Pair.of(tableIf,
pctPartition));
- if (mvPartition != null) {
- res.add(mvPartition);
+ Set<String> mvPartitions = relatedToMv.get(Pair.of(tableIf,
pctPartition));
+ if (mvPartitions == null) {
+ // A partition the query reads that no MV partition is
mapped from -- one the partition
+ // mapping left out, an expired base partition for
instance -- is one the MV's rows say
+ // nothing about. With the union rewrite on it is taken
out of the MV plan and read from
+ // the base table, so the MV partitions the other base
partitions map to still answer the
+ // query and this one is passed over. Without it the MV
alone would answer, so no partition
+ // is answered for at all: the MV is not a candidate for
this query and the query is
+ // answered from the base table.
+ if (unionRewrite) {
Review Comment:
Confirmed and reproduced, but not fixed: I reverted the attempt rather than
land it half right.
Reproduced on this shape -- A and B each `LIST(d,k)` with `p_kept` holding a
2038 key, B additionally `p_old=((2020-01-01,2))`, a 2 YEAR window and a grace
period, an MV joining on d: the MV holds `2020-01-01 -> 1` and `select a.d,
count(*) from a join b on a.d = b.d group by a.d` came back as two rows of 1
for 2020 against the base table's single 2, so the committed `p_old` row is not
in the answer the union path gives.
What I tried was the first option you give: when a query-used partition of a
table is not named by any MV partition and not named by the MV's own snapshots
for the partitions the plan uses, take the plan's MV partitions out and read
the query's partitions from the base. It fixed the shape above (the query then
returned exactly the base's rows, `2020-01-01 -> 2`), but it also fired on
`partition_mv_rewrite`'s ordinary union case, whose answer the existing
compensation already gets right, and narrowing the trigger to the tables that
pass produced no union brought the original shape back. So the piece that
matters is not the trigger, it is that the pass above works from the mapping
and a table can therefore drop out of the compensation mapping while its
query-used partitions remain in the plan -- which needs to be handled where
that mapping is built for the compensation, not beside it.
I have left it as it was rather than push a version that trades one of the
two shapes for the other; the reproduction above is the case to check a fix
against.
--
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]
