yujun777 commented on code in PR #68648:
URL: https://github.com/apache/doris/pull/68648#discussion_r4225595317
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVRelatedPartitionDescTransferGenerator.java:
##########
@@ -46,7 +47,8 @@ public class MTMVRelatedPartitionDescTransferGenerator
implements MTMVRelatedPar
public void apply(MTMVPartitionInfo mvPartitionInfo, Map<String, String>
mvProperties,
RelatedPartitionDescResult lastResult, List<Column>
partitionColumns,
Map<List<String>, Set<String>> queryUsedPartitionMap)
throws AnalysisException {
- Map<MTMVRelatedTableIf, Map<PartitionKeyDesc, Set<String>>> descs =
lastResult.getDescs();
+ Map<MTMVRelatedTableIf, Map<PartitionKeyDesc, Set<String>>> descs =
+ mergeOverlappingListDescs(lastResult.getDescs());
Map<PartitionKeyDesc, Map<MTMVRelatedTableIf, Set<String>>> res =
Maps.newHashMap();
Review Comment:
Fixed in 65267ad3698. You are right that merging per table left a
multi-table MV unbuildable: t9's partitions projecting to 2020-2022 and t10's
to 2021-2022 came out as {2020,2021,2022} beside {2021,2022}, and
`checkIntersect` rejects exactly that (the partition creation would repeat a
key for any MV as well).
The keys are now grouped across every table, with path compression over the
keys, and the one partition that holds a group's keys names every table's
partitions of them -- which is what a refresh reads and records for those keys.
Covered by unit tests: the existing t6/t7 case used to raise `PartitionValue is
repeat` and now yields the two MV partitions {1,3} and {2}, each naming both
tables' partitions; the new case is your shape (t9 covering 2020-2022, t10
covering 2021-2022 -> one partition holding all three keys, naming t9's two
partitions and t10's one).
The two-table default/window fixture you ask for on top of that is the same
key question, and it is the next comment's.
##########
fe/fe-core/src/main/java/org/apache/doris/mtmv/MTMVRelatedPartitionDescTransferGenerator.java:
##########
@@ -65,6 +70,68 @@ public void apply(MTMVPartitionInfo mvPartitionInfo,
Map<String, String> mvPrope
lastResult.setRes(res);
}
+ /**
+ * One MV partition per set of keys, not one per way of writing a set
down. A partition of a list
+ * partitioned base table can hold several keys of the MV's partition
column, so two of them can describe
+ * keys that meet: an expired partition holding one key of a retained
partition's key list, for instance.
+ * An MV's own partitions cannot overlap, so descs whose keys meet are one
partition whose keys are the
+ * union of theirs. Without this an MV over such a table cannot be built
at all -- its partition items
+ * would repeat a key -- which is the shape a default-partition table is
left unwindowed into, and the
+ * one it is recorded with changes with it: the merged partition names
every base partition of the keys
+ * it covers, which is what a refresh reads for it.
+ *
+ * <p>Descs whose keys are disjoint, one desc per table, and every desc
that is not a list of keys are
+ * left exactly as they were, so an MV whose base partitions do not meet
keeps its partitions and their
+ * names.
+ */
+ private Map<MTMVRelatedTableIf, Map<PartitionKeyDesc, Set<String>>>
mergeOverlappingListDescs(
+ Map<MTMVRelatedTableIf, Map<PartitionKeyDesc, Set<String>>> descs)
{
+ Map<MTMVRelatedTableIf, Map<PartitionKeyDesc, Set<String>>> res =
Maps.newHashMap();
+ for (Entry<MTMVRelatedTableIf, Map<PartitionKeyDesc, Set<String>>>
entry : descs.entrySet()) {
+ res.put(entry.getKey(),
mergeOverlappingListDescsOfOneTable(entry.getValue()));
+ }
+ return res;
+ }
+
+ private Map<PartitionKeyDesc, Set<String>>
mergeOverlappingListDescsOfOneTable(
+ Map<PartitionKeyDesc, Set<String>> descs) {
+ Map<PartitionKeyDesc, Set<String>> res = Maps.newHashMap();
+ List<Set<List<PartitionValue>>> mergedKeys = Lists.newArrayList();
+ List<Set<String>> mergedNames = Lists.newArrayList();
+ List<PartitionKeyDesc> mergedDescs = Lists.newArrayList();
+ for (Entry<PartitionKeyDesc, Set<String>> entry : descs.entrySet()) {
+ if (!entry.getKey().hasInValues()) {
+ res.put(entry.getKey(), entry.getValue());
+ continue;
+ }
+ Set<List<PartitionValue>> keys =
Sets.newHashSet(entry.getKey().getInValues());
+ Set<String> names = Sets.newHashSet(entry.getValue());
+ // The desc this group came from, kept while it is the only one,
since a desc that was not
+ // merged is left as it is rather than written out again in
another key order.
+ PartitionKeyDesc mergedDesc = entry.getKey();
+ for (int i = mergedKeys.size() - 1; i >= 0; i--) {
+ if (Collections.disjoint(mergedKeys.get(i), keys)) {
Review Comment:
Fixed in 65267ad3698, by the shape you suggest: the keys are indexed to
their group and only the groups that share a key are unioned (path compression
on the way), so each key is looked up once instead of every prior group being
scanned for every desc. The disjointness of the result is what the union-find
maintains: two descs that share a key end up in one group, so the groups -- and
therefore the MV's partitions -- cannot repeat a key.
The grace-period point is taken as well: the mapping is built on the rewrite
path whether or not anything is compared, so the cost of building it is worth
bounding; it is now one hash lookup per key of each desc.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/MTMV.java:
##########
@@ -1619,6 +1621,65 @@ public Map<String, Map<MTMVRelatedTableIf, Set<String>>>
calculatePartitionMappi
return res;
}
+ /**
+ * The list partition each base table of this MV has that takes the rows
no other partition of it claims,
+ * by table, or none for a table that has no such partition.
+ *
+ * <p>Read once per mapping rather than per MV partition: the mapping
describes every MV partition and the
+ * answer is the table's, not the partition's. The table's partitions are
read under its read lock, so
+ * that a concurrent ADD or DROP PARTITION cannot be seen half applied --
its name list and the items the
+ * walk resolves against it have to come from one state of the table --
and so that this walk is not one
+ * more reader of a tree another thread is modifying.
+ */
+ private Map<MTMVRelatedTableIf, String> defaultListPartitionsOf() throws
AnalysisException {
+ Map<MTMVRelatedTableIf, String> res = Maps.newHashMap();
+ for (MTMVRelatedTableIf pctTable : mvPartitionInfo.getPctTables()) {
+ if (!(pctTable instanceof OlapTable)) {
+ continue;
+ }
+ OlapTable olapTable = (OlapTable) pctTable;
+ if (!(olapTable.getPartitionInfo() instanceof ListPartitionInfo)) {
+ continue;
+ }
+ olapTable.readLock();
+ try {
+ for (String partitionName : olapTable.getPartitionNames()) {
+ if
(olapTable.getPartitionItemOrAnalysisException(partitionName).isDefaultPartition())
{
+ res.put(pctTable, partitionName);
+ break;
+ }
+ }
+ } finally {
+ olapTable.readUnlock();
+ }
+ }
+ return res;
+ }
+
+ /**
+ * One MV partition's mapping, with every base table's default list
partition named in it.
+ *
+ * <p>Such a partition holds rows for every key its table can be read by,
so it belongs to every MV
+ * partition that reads the table -- not only to the one its own key, the
sentinel those rows were placed
+ * by, maps to. Naming it everywhere is what the read and the record have
to agree on: the refresh reads
+ * the rows of it that belong to the MV partition being refreshed, and the
partition is recorded among the
+ * ones that partition is read through, so an insert into it leaves that
MV partition out of sync instead
+ * of changing nothing the MV compares.
+ */
+ private Map<MTMVRelatedTableIf, Set<String>> withDefaultListPartitions(
+ Map<MTMVRelatedTableIf, Set<String>> mapping,
Map<MTMVRelatedTableIf, String> defaultListPartitions) {
+ if (defaultListPartitions.isEmpty()) {
+ return mapping;
+ }
+ Map<MTMVRelatedTableIf, Set<String>> res = Maps.newHashMap(mapping);
+ for (Entry<MTMVRelatedTableIf, String> entry :
defaultListPartitions.entrySet()) {
+ Set<String> partitions =
Sets.newHashSet(res.getOrDefault(entry.getKey(), Sets.newHashSet()));
+ partitions.add(entry.getValue());
+ res.put(entry.getKey(), partitions);
Review Comment:
Confirmed, and I agree it is a representation gap rather than anything this
PR introduced -- but I do not think it is reachable by the direction this PR
takes, so I have left it alone this round rather than guessed.
Why the merge and the read-scope work do not reach it: the MV's own
partition set comes from the base partitions' keys projected onto the MV's
partition column. A key that only ever lived in the default partition has no
base partition describing it, so it has no MV partition -- and the refresh
reads each MV partition through its own key range, so a key with no range is
read by nobody. That is your k=2: `LIST(k)` with `p1=(1)` and `p_default`, a
committed `k=2` row in `p_default`, an MV partitioned by `k` whose partitions
are key 1 and the sentinel one. Reading the row is not the hard part; routing
it is: `INSERT OVERWRITE` puts a row in the MV partition its key names, and
there is no such partition to put it in. Materializing it into the sentinel
partition would make it invisible to a query for k=2 that prunes to the MV's
partitions.
So it is one of two deliberate choices, and both are user-visible enough
that I would rather ask than take one:
1. Refuse the rewrite for a materialized view whose PCT table has a list
partition's default partition. The MV keeps working for direct queries and
keeps refreshing; queries over that table are answered from the base table.
Narrow, and it cannot serve a row it does not hold; the cost is losing the
rewrite for these MVs.
2. Refuse the shape at `CREATE` -- an MV partitioned on a base table that
has a list partition's default partition -- which is what r4140476259 offered
as the alternative when the fan-out went in. Cheaper to keep sound, but it
takes away the shape rather than the rewrite.
I did not touch either this round. If you want one in this PR, 1 is the
smaller change and the one I would take; tell me and I will do it in the next
round.
--
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]
