github-actions[bot] commented on code in PR #68648: URL: https://github.com/apache/doris/pull/68648#discussion_r4140476271
########## regression-test/suites/mtmv_p0/test_mtmv_base_partition_read_scope.groovy: ########## @@ -0,0 +1,107 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +suite("test_mtmv_base_partition_read_scope") { + // A refresh reads the base partitions the MV partition is recorded with, and no others. The window of + // partition_sync_limit below keeps the last two days, so the day before them is recorded nowhere: it + // must not be read either, or its rows would sit in the MV partition -- whose key range does cover + // them -- while the snapshot says the MV does not hold that partition. A base partition dropped after + // such a read would then be invisible to the sync check, and the transparent rewrite would serve the + // rows of a partition the base table no longer has. + // + // The MV partition is a year and the base table's are days, so the range it is read through is wider + // than what it is recorded with. Days are taken relative to today, and the two sides are asserted + // separately rather than as a total, so that where the window's edge falls does not decide the case. + def today = java.time.LocalDate.now() + def expiredDay = today.minusDays(5) + def keptDay = today.minusDays(1) + // The expired day is read by a refresh only while it falls inside a retained MV partition's range, and + // both days are inside the same year only from the third of January on. Skipped rather than asserted + // when they are not, so that a run that exercises the case is a run that can fail without the change. + org.junit.Assume.assumeTrue("the expired day and today are in the same year", Review Comment: [P2] Avoid a JUnit assumption as this suite's calendar guard. From January 1 through 5, `expiredDay` is in the prior year and `assumeTrue` throws, but the Groovy regression runner catches every `Throwable` in `SuiteContext.start` and records it through `onSuiteFailed`; this makes the suite fail rather than skip, and the independent LIST case never runs. Use a calendar-independent fixture or a supported conditional that leaves the LIST case runnable. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/UpdateMvByPartitionCommand.java: ########## @@ -176,14 +218,71 @@ public static Set<Expression> constructPredicates(Set<PartitionItem> partitions, return predicates; } - private static Expression convertPartitionKeyToLiteral(PartitionKey key) { - return Literal.fromLegacyLiteral(key.getKeys().get(0), - Type.fromPrimitiveType(key.getTypes().get(0))); + /** + * The predicate a base table is read through when the refresh is to read exactly these partitions of it. + * + * <p>A partition of a list partitioned table holds one key per partition column, and the column the MV + * partition is named by is only one of them. A predicate on that column alone also reaches the + * partitions whose other keys differ -- a table partitioned by (d, region) has one partition of + * (d0, 'US') and one of (d0, 'EU'), and `d = d0` reaches both, while only the second is a partition + * this refresh is to read; a later drop of the first would then leave its rows in the MV partition + * while the snapshot, which names only the second, still calls it synchronized. So a list partition is + * pinned to its whole key. A range partition is pinned to its bounds, which is the same thing: a base + * table partitioned by range has a single partition column, see + * {@code RangePartitionItem#toPartitionKeyDesc(int)}. + * + * <p>The partitions are never empty: a table the caller scopes with no partition is read as nothing + * before this is reached, see {@code constructTableWithPredicates}. + */ + private static Set<Expression> constructPredicatesOfBasePartitions(Set<PartitionItem> partitions, + OlapTable baseTable, String colName) throws AnalysisException { + if (!(partitions.iterator().next() instanceof ListPartitionItem)) { + return constructPredicates(partitions, colName); + } + List<Slot> partitionSlots = Lists.newArrayList(); + for (Column partitionColumn : baseTable.getPartitionColumns()) { + partitionSlots.add(new UnboundSlot(partitionColumn.getName())); + } + Set<Expression> predicates = new HashSet<>(); + for (PartitionItem item : partitions) { + predicates.add(convertListPartitionToKey(item, partitionSlots)); + } + return predicates; + } + + /** + * One partition of a list partitioned table, pinned to the whole of each key it holds: the keys are + * what tells it apart from a partition that shares a key with it, and the value of a key a row does not + * have is asked for as {@code IS NULL}, since no comparison to it is ever true. + * + * <p>A partition that holds no key at all -- a list partitioned table's default partition, which takes + * the rows no other partition claims -- is read in full, as it was before this scope existed: what it + * holds cannot be said with a predicate on the partition columns, and reading it in full keeps its rows + * in the MV, which is the reading that can be seen to be too wide rather than one that loses them + * quietly. + */ + private static Expression convertListPartitionToKey(PartitionItem item, List<Slot> partitionSlots) { + List<Expression> keys = new ArrayList<>(); + for (PartitionKey key : ((ListPartitionItem) item).getItems()) { + List<Expression> oneKey = new ArrayList<>(); + for (int pos = 0; pos < partitionSlots.size(); pos++) { + Expression value = convertPartitionKeyToLiteral(key, pos); + oneKey.add(value instanceof NullLiteral ? new IsNull(partitionSlots.get(pos)) + : new EqualTo(partitionSlots.get(pos), value)); + } + keys.add(ExpressionUtils.and(oneKey)); + } + return keys.isEmpty() ? BooleanLiteral.TRUE : ExpressionUtils.or(keys); + } + + private static Expression convertPartitionKeyToLiteral(PartitionKey key, int keyPos) { + return Literal.fromLegacyLiteral(key.getKeys().get(keyPos), + Type.fromPrimitiveType(key.getTypes().get(keyPos))); Review Comment: [P1] Preserve DATETIMEV2 scale in scoped base-partition predicates. `Type.fromPrimitiveType(DATETIMEV2)` supplies scale 0, so this conversion rounds fractional partition keys before building both LIST tuple equalities and RANGE bounds. For example, a `RANGE(ts DATETIMEV2(3))` partition ending at `2024-02-01 00:00:00.123` in a year-rollup MV gets a new `ts < ...00.000` filter, omitting rows that belong to the mapped partition; a secondary LIST key at `.123` is similarly compared to `.000`. The snapshot still records that partition, so the MV can appear synchronized while missing rows. Convert with the base partition column's full `Type` and cover fractional keys in refresh tests. ########## fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/UpdateMvByPartitionCommand.java: ########## @@ -176,14 +218,71 @@ public static Set<Expression> constructPredicates(Set<PartitionItem> partitions, return predicates; } - private static Expression convertPartitionKeyToLiteral(PartitionKey key) { - return Literal.fromLegacyLiteral(key.getKeys().get(0), - Type.fromPrimitiveType(key.getTypes().get(0))); + /** + * The predicate a base table is read through when the refresh is to read exactly these partitions of it. + * + * <p>A partition of a list partitioned table holds one key per partition column, and the column the MV + * partition is named by is only one of them. A predicate on that column alone also reaches the + * partitions whose other keys differ -- a table partitioned by (d, region) has one partition of + * (d0, 'US') and one of (d0, 'EU'), and `d = d0` reaches both, while only the second is a partition + * this refresh is to read; a later drop of the first would then leave its rows in the MV partition + * while the snapshot, which names only the second, still calls it synchronized. So a list partition is + * pinned to its whole key. A range partition is pinned to its bounds, which is the same thing: a base + * table partitioned by range has a single partition column, see + * {@code RangePartitionItem#toPartitionKeyDesc(int)}. + * + * <p>The partitions are never empty: a table the caller scopes with no partition is read as nothing + * before this is reached, see {@code constructTableWithPredicates}. + */ + private static Set<Expression> constructPredicatesOfBasePartitions(Set<PartitionItem> partitions, + OlapTable baseTable, String colName) throws AnalysisException { + if (!(partitions.iterator().next() instanceof ListPartitionItem)) { + return constructPredicates(partitions, colName); + } + List<Slot> partitionSlots = Lists.newArrayList(); + for (Column partitionColumn : baseTable.getPartitionColumns()) { + partitionSlots.add(new UnboundSlot(partitionColumn.getName())); + } + Set<Expression> predicates = new HashSet<>(); + for (PartitionItem item : partitions) { + predicates.add(convertListPartitionToKey(item, partitionSlots)); + } + return predicates; + } + + /** + * One partition of a list partitioned table, pinned to the whole of each key it holds: the keys are + * what tells it apart from a partition that shares a key with it, and the value of a key a row does not + * have is asked for as {@code IS NULL}, since no comparison to it is ever true. + * + * <p>A partition that holds no key at all -- a list partitioned table's default partition, which takes + * the rows no other partition claims -- is read in full, as it was before this scope existed: what it + * holds cannot be said with a predicate on the partition columns, and reading it in full keeps its rows + * in the MV, which is the reading that can be seen to be too wide rather than one that loses them + * quietly. + */ + private static Expression convertListPartitionToKey(PartitionItem item, List<Slot> partitionSlots) { + List<Expression> keys = new ArrayList<>(); + for (PartitionKey key : ((ListPartitionItem) item).getItems()) { + List<Expression> oneKey = new ArrayList<>(); + for (int pos = 0; pos < partitionSlots.size(); pos++) { + Expression value = convertPartitionKeyToLiteral(key, pos); + oneKey.add(value instanceof NullLiteral ? new IsNull(partitionSlots.get(pos)) + : new EqualTo(partitionSlots.get(pos), value)); + } + keys.add(ExpressionUtils.and(oneKey)); + } + return keys.isEmpty() ? BooleanLiteral.TRUE : ExpressionUtils.or(keys); Review Comment: [P1] Keep default LIST rows reachable when scoping by full tuples. With `LIST(k1,k2)`, an explicit partition for `(1,2)`, and a default partition containing `(1,3)`, an MV partitioned by `k1` previously read both rows for key 1. This predicate now reads only `(1,2)`. Doris represents the default partition with a synthetic MIN key, so `keys.isEmpty()` is never true; its separate MIN MV partition also cannot supply `(1,3)`. The MV loses a committed row while its mapped snapshots can report synchronized. Handle default partition coverage in the mapping/read together, or reject this shape until it can be represented; add a regression with a default row sharing an explicit MV key. -- 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]
