github-actions[bot] commented on code in PR #66307:
URL: https://github.com/apache/doris/pull/66307#discussion_r3758462772
##########
fe/fe-core/src/main/java/org/apache/doris/backup/RestoreJob.java:
##########
@@ -2631,11 +2644,22 @@ private Status atomicReplaceOlapTables(Database db,
boolean isReplay) {
Review Comment:
[P1] Serialize constraint ADD with atomic replacement
This check/drop is not one catalog transaction with ordinary PK/UK/FK ADD:
those paths release their planner locks and call
`ConstraintManager.addConstraint()` without the database/table locks held here.
An ADD can therefore validate and journal a PK against the old object after
this call but before `unregisterTable`; the leader's mapping-only
`restoreTableConstraints()` leaves that name-indexed PK on the replacement,
while a follower replays ADD and then `FINISHED` restore, reruns this call, and
removes it. That leaves different constraint metadata (and potentially a false
uniqueness assertion) across FEs. Please serialize all constraint DDL with this
replacement transition and add a latch-based ADD/restore replay test.
##########
fe/fe-core/src/main/java/org/apache/doris/backup/RestoreJob.java:
##########
@@ -2631,11 +2644,22 @@ private Status atomicReplaceOlapTables(Database db,
boolean isReplay) {
Review Comment:
[P1] Prevalidate every atomic-restore target before publishing
This check can fail after earlier entries in `backupOlapTableObjects` have
already been replaced. With two targets, A is unregistered/replaced/erased,
then B's referenced PK makes this call fail. Cancellation skips A because its
restored object no longer has the atomic prefix, and the only terminal journal
is `CANCELLED`; follower replay therefore never runs the replacement path
reserved for `FINISHED`, leaving restored A on the leader and old A on the
follower. The added test covers only one target failing before any mutation.
Please validate every target before the first unregister, and add a two-target
cancellation plus replay test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/ChildOutputPropertyDeriver.java:
##########
@@ -197,6 +247,36 @@ public PhysicalProperties visitPhysicalHashAggregate(
}
}
+ private PhysicalProperties computeAggregateOutputProperties(
+ PhysicalHashAggregate<? extends Plan> agg, PhysicalProperties
childOutputProperty) {
+ NaturalDistributionMappingSpec naturalMappingSpec =
+ childOutputProperty.getNaturalDistributionMappingSpec().get();
+ if (agg.hasSourceRepeat()) {
+ return withoutNaturalDistributionMapping(childOutputProperty);
+ }
+
+ Set<ExprId> groupByExprIds = Sets.newHashSet();
+ for (Expression groupBy : agg.getGroupByExpressions()) {
+ if (!(groupBy instanceof SlotReference)) {
+ return withoutNaturalDistributionMapping(childOutputProperty);
+ }
+ groupByExprIds.add(((SlotReference) groupBy).getExprId());
+ }
+ if
(!naturalMappingSpec.distributionKeysCoveredByDirectOrMapping(groupByExprIds)) {
Review Comment:
[P1] Clear order on mapping-aware hash aggregates
This path preserves `childOrderSpec` through
`computeProjectOutputProperties()`, and every fallback through
`withoutNaturalDistributionMapping()` does the same, while the established
non-mapping aggregate path deliberately clears order. A reachable tree is
`Window outer -> HashAggregate -> Window inner -> LocalSort -> OlapScan` with a
natural mapping: the inner window retains the mapping and order, the hash
aggregate can emit arbitrary hash-table order but still advertises that order,
and `PhysicalProperties.satisfy()` lets the outer window skip its required
sort. Please preserve only the projected distribution/mapping proof and return
an empty `OrderSpec` on every hash-aggregate path, with ordered mapping-child
coverage.
--
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]
