yujun777 commented on code in PR #68648:
URL: https://github.com/apache/doris/pull/68648#discussion_r4141439333
##########
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:
Fixed in 5bf185b273d, and you were right about the representation: a default
partition's item is not empty, it holds the sentinel key the rows no other
partition claims are placed by, so the `keys.isEmpty()` I had written to read
it in full was dead code and the partition was read as none of its rows.
Measured on the shape in your comment -- `LIST(k1, k2)` with an explicit `(1,
2)` and a default partition holding `(1, 3)`, an MV partitioned by `k1` -- the
MV held only `(1, 2)` before the change and holds both after it.
The two cannot be handled apart, as you say: which rows of a default
partition belong to an MV partition is the MV partition's own question, since
it takes the rows whose key falls in its range wherever the base table placed
them, and that partition is not one the MV partition is recorded with. So a
base table that has one is read the way an unscoped one is -- the MV
partition's own key range -- which is a reading that can be seen to be too wide
rather than one that drops rows belonging to the partition being refreshed. A
regression case pins it (`list_default` in the read scope suite), and it fails
when that fallback is removed.
##########
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:
Fixed in 5bf185b273d. The key is converted with its partition column's full
`Type` now, so a value written with milliseconds is compared at that scale; the
callers that have the column pass its type and the ones that do not keep the
primitive one they were given before.
Measured on a fractional key rather than assumed: `LIST(ts)` with a
partition of `('2024-02-01 00:00:00.123')` and an MV partitioned by `ts` left
that MV partition empty before the change -- the predicate had been rounded to
`.000` -- and holds its row after it. A regression case pins it
(`fractional_key`), failing when the type is taken from the primitive again.
One thing your example led me to check: a `RANGE(ts DATETIME(3))` partition
ending at a fractional value cannot back a year rollup MV at all --
`MTMVPartitionExprDateTrunc` refuses it at CREATE with "partition values not
equal" -- so the range bound is not a way to reach this; the list key is.
--
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]
