github-actions[bot] commented on code in PR #66307:
URL: https://github.com/apache/doris/pull/66307#discussion_r3829672638
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/MetastoreEventSyncDriver.java:
##########
@@ -247,6 +247,18 @@ private void applyDescriptors(PluginDrivenExternalCatalog
catalog, Connector con
// legacy event.process() bodies called, now generalized to work on a
flipped catalog.
private void applyOne(PluginDrivenExternalCatalog catalog, Connector
connector,
MetastoreChangeDescriptor descriptor) throws Exception {
+ if (!affectsConstraintMetadata(descriptor)) {
+ applyOneInternal(catalog, connector, descriptor);
+ return;
+ }
+ try (ExternalCatalog.ConstraintMetadataMutationGuard ignored =
+ catalog.beginConstraintMetadataMutation()) {
Review Comment:
[P1] Transition constraints with external structural events
This guard orders the event against constraint DDL, but the guarded
`UNREGISTER_DATABASE`, `RENAME_DATABASE`, `UNREGISTER_TABLE`, and
`RENAME_TABLE` cases only unregister/register external cache objects; they
never drop or rename `ConstraintManager` entries. For example, after adding an
external PK/UK/FK on `ext.db.t`, an `UNREGISTER_TABLE` followed by recreation
of `t` leaves the old name-keyed constraint attached to an unrelated table, so
Nereids can consume a false uniqueness/FK proof. A rename similarly loses the
proof at the new name and lets later reuse of the old name reactivate it. The
explicit plugin table/database DROP and replay paths have the same missing
transition. Please update the manager within the structural mutation protocol
(with FK safeguards) and make event, explicit DDL, and replay behavior
equivalent, with drop/recreate and rename/old-name-reuse coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/util/JoinUtils.java:
##########
@@ -307,12 +356,85 @@ public static boolean
couldColocateJoin(DistributionSpecHash leftHashSpec, Distr
equalIndices.add(leftIndex);
}
}
- // on conditions must contain all distributed columns
- if
(equalIndices.containsAll(leftHashSpec.getExprIdToEquivalenceSet().values())) {
- return true;
- } else {
+ return
equalIndices.containsAll(leftHashSpec.getExprIdToEquivalenceSet().values());
+ }
+
+ private static boolean
couldColocateJoinOnDistributionMappings(DistributionSpecHash leftHashSpec,
+ DistributionSpecHash rightHashSpec, List<Expression> conjuncts) {
+ return couldColocateJoinOnDistributionMappings(
+ leftHashSpec.getExprIdToEquivalenceSet(),
leftHashSpec.getDistributionMappings(),
+ rightHashSpec.getExprIdToEquivalenceSet(),
rightHashSpec.getDistributionMappings(),
+ leftHashSpec.getOrderedShuffledColumns().size(), conjuncts);
+ }
+
+ private static boolean couldColocateJoinOnDistributionMappings(
+ Map<ExprId, Integer> leftDistributionExprToIndex,
List<DistributionMapping> leftMappings,
+ Map<ExprId, Integer> rightDistributionExprToIndex,
List<DistributionMapping> rightMappings,
+ int distributionKeyCount, List<Expression> conjuncts) {
+ if (!areAllSlotEqualPredicates(conjuncts)) {
return false;
}
+ List<Pair<ExprId, ExprId>> equalExprIds = Lists.newArrayList();
+ Set<Integer> coveredIndices = new HashSet<>();
+ for (Expression expr : conjuncts) {
+ ExprId first = ((SlotReference) ((EqualPredicate)
expr).left()).getExprId();
+ ExprId second = ((SlotReference) ((EqualPredicate)
expr).right()).getExprId();
+ equalExprIds.add(Pair.of(first, second));
+
+ Integer leftIndex = leftDistributionExprToIndex.get(first);
+ Integer rightIndex = rightDistributionExprToIndex.get(second);
+ if (leftIndex == null) {
+ leftIndex = leftDistributionExprToIndex.get(second);
+ rightIndex = rightDistributionExprToIndex.get(first);
+ }
+ if (leftIndex != null && Objects.equals(leftIndex, rightIndex)) {
+ coveredIndices.add(leftIndex);
+ }
+ }
+
+ for (DistributionMapping leftMapping : leftMappings) {
Review Comment:
[P2] Index mapping candidates before comparing determinants
This compares every left mapping with every right mapping for each
mapping-based join proof. Constraint DDL permits an unbounded number of
distinct mappings per table, so two tables with L and R mappings make this path
O(L*R*D*E), even when all mapping IDs are disjoint; the proof is then invoked
during child regulation, output derivation, and final colocate detection. A
table pair with many unrelated mapping IDs can therefore make ordinary
EXPLAIN/planning quadratic in metadata size. Please index one side by
`(mappingId, targetDistributionIndices, determinantCount)` (and stop once all
bucket positions are covered), then compare determinants only among matching
candidates, with a scale test using many disjoint mapping IDs.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java:
##########
@@ -5996,6 +6002,18 @@ private void renameColumn(Database db, OlapTable table,
String colName,
if (partitionInfo.getPartitionColumns().stream().anyMatch(c ->
c.getName().equalsIgnoreCase(colName))) {
throw new DdlException("Renaming partition columns has problems,
forbidden in current Doris version");
}
+ String mappingConstraint =
Review Comment:
[P1] Replay legacy constrained-column renames
This ordinary-constraint fallback also runs for `isReplay=true`, but older
releases allowed RENAME COLUMN on PK/UK/FK columns and emitted
`OP_RENAME_COLUMN`. During upgrade, replaying such an ADD-constraint then
RENAME sequence now throws here; `replayRenameColumn` catches the exception and
continues, leaving the follower with the old column while the original
leader/image has the renamed schema. Distribution mappings cannot occur in an
old log, so their replay guard does not require broadening this compatibility
break to legacy constraint types. Please replay historically valid rename
records (or apply a backward-compatible constraint transition) and add a
synthetic legacy-log test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/DropConstraintCommand.java:
##########
@@ -67,38 +75,123 @@ public DropConstraintCommand(String name, LogicalPlan
plan) {
@Override
public void run(ConnectContext ctx, StmtExecutor executor) throws
Exception {
TableNameInfo tableNameInfo;
- try {
- TableIf table = extractTable(ctx, plan);
- tableNameInfo = TableNameInfoUtils.fromCatalogDb(
- table.getDatabase().getCatalog(), table.getDatabase(),
table);
- } catch (Exception e) {
- // Table may no longer exist (e.g., external table deleted by
another system).
- // Fall back to extracting the table name from the unresolved plan.
- LOG.warn("Table resolution failed for dropping constraint {}, "
- + "falling back to name-based lookup: {}", name,
e.getMessage());
- tableNameInfo = extractTableNameFromPlan(ctx);
+ TableNameInfo unresolvedTableName = plan instanceof UnboundRelation
+ ? extractTableNameFromPlan(ctx) : null;
+ CatalogIf<?> unresolvedCatalog = unresolvedTableName == null ? null
+ :
Env.getCurrentEnv().getCatalogMgr().getCatalog(unresolvedTableName.getCtl());
+ if (unresolvedCatalog instanceof ExternalCatalog) {
+ // External PK/FK/UK constraints are authoritative in
ConstraintManager. Avoid connector
+ // schema loading so DROP works with cache disabled or session
cache bypass.
+ ExternalCatalog externalCatalog = (ExternalCatalog)
unresolvedCatalog;
+ unresolvedTableName = new TableNameInfo(
+ externalCatalog.getName(), unresolvedTableName.getDb(),
unresolvedTableName.getTbl());
+ boolean lowerCaseMetaNames =
+
Boolean.parseBoolean(externalCatalog.getLowerCaseMetaNames());
+ tableNameInfo = !lowerCaseMetaNames
+ && externalCatalog.getLowerCaseTableNames() == 0
+ && externalCatalog.getLowerCaseDatabaseNames() == 0
+ ? unresolvedTableName
+ : Env.getCurrentEnv().getConstraintManager()
+ .canonicalizeExternalTableName(
+ unresolvedTableName,
+ name,
+ lowerCaseMetaNames
+ ||
externalCatalog.getLowerCaseDatabaseNames() != 0,
+ lowerCaseMetaNames
+ ||
externalCatalog.getLowerCaseTableNames() != 0);
+ } else {
+ try {
+ TableIf table = extractTable(ctx, plan);
+ tableNameInfo = TableNameInfoUtils.fromCatalogDb(
+ table.getDatabase().getCatalog(), table.getDatabase(),
table);
+ } catch (Exception e) {
+ // Table may no longer exist (e.g., external table deleted by
another system).
+ // Fall back to extracting the table name from the unresolved
plan.
+ LOG.warn("Table resolution failed for dropping constraint {}, "
+ + "falling back to name-based lookup: {}", name,
e.getMessage());
+ if (unresolvedTableName == null) {
+ throw e;
+ }
+ tableNameInfo = unresolvedTableName;
+ }
}
// must be checked on both paths above: table resolution failing
(which includes an
// authorization failure) falls back to a name-only lookup that binds
nothing.
checkAlterPriv(ctx, tableNameInfo);
+ Constraint initialConstraint = getConstraintOrThrow(tableNameInfo);
+ List<TableNameInfo> initialCascadeDropTables = Env.getCurrentEnv()
+
.getConstraintManager().getCascadeDropTables(initialConstraint);
+ List<TableNameInfo> affectedTableInfos = new ArrayList<>();
+ affectedTableInfos.add(tableNameInfo);
+ affectedTableInfos.addAll(initialCascadeDropTables);
+ ConstraintCommandUtils.ExternalCatalogSnapshots
externalCatalogSnapshots =
+
ConstraintCommandUtils.snapshotExternalCatalogs(affectedTableInfos);
+
+ Constraint constraint;
+ List<MTMV> dependentMtmvs;
+ EditLog.EditLogItem logItem;
+ try (ConstraintCommandUtils.LockedDatabases lockedDatabases =
+ ConstraintCommandUtils.lockCurrentDatabases(
+ affectedTableInfos, externalCatalogSnapshots,
List.of());
+ ConstraintCommandUtils.LockedTables lockedTables =
+ ConstraintCommandUtils.lockCurrentTablesIfPresent(
+ lockedDatabases, affectedTableInfos)) {
+ TableIf currentTable = lockedTables.get(tableNameInfo);
+ constraint = getConstraintOrThrow(tableNameInfo);
+ if (constraint instanceof DistributionMappingConstraint
+ && !(currentTable instanceof OlapTable)) {
+ throw new AnalysisException(
+ "Distribution mapping constraint requires an OLAP
table");
+ }
+ List<TableNameInfo> cascadeDropTables = Env.getCurrentEnv()
+ .getConstraintManager().getCascadeDropTables(constraint);
+ if (!ConstraintCommandUtils.sameTables(
+ initialCascadeDropTables, cascadeDropTables)) {
+ throw new AnalysisException(
+ "Foreign key references changed while dropping
constraint "
+ + name + " on " + tableNameInfo + ", retry the
statement");
+ }
+ for (TableNameInfo fkTableInfo : cascadeDropTables) {
+ checkAlterPriv(ctx, fkTableInfo);
+ }
+ dependentMtmvs = getDependentMtmvs(
+ tableNameInfo, constraint, cascadeDropTables);
+ logItem = Env.getCurrentEnv().getConstraintManager()
+ .dropConstraintAndSubmit(tableNameInfo, name,
cascadeDropTables);
+ if (constraint instanceof DistributionMappingConstraint) {
+ Env.getCurrentEnv().getSqlCacheManager()
+ .invalidateAboutTableAndFencePublication(currentTable);
+ }
+ }
+ if (logItem != null) {
Review Comment:
[P1] Fence MTMV plans before awaiting durability
Putting `await()` here releases the table locks after the manager entry is
removed but delays `invalidateRewriteCachesBestEffort` until the journal flush
completes, which can block indefinitely. Queries in that interval return the
existing `MTMVCache` immediately, even though its stored rewritten plan may
have used the dropped PK/UK/FK for group-by or join elimination; a rewrite can
therefore rely on a proof that no longer exists. Please publish the dependent
MTMV generation fence as part of the locked in-memory DROP transition, then
keep only the durability wait outside the metadata locks, with a
blocked-journal/concurrent-rewrite test.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/constraint/ConstraintManager.java:
##########
@@ -117,20 +157,90 @@ public void addConstraint(TableNameInfo tableNameInfo,
String constraintName,
tableNameInfo, (ForeignKeyConstraint) constraint);
}
tableConstraints.put(constraintName, constraint);
+ if (constraint instanceof DistributionMappingConstraint) {
+ putTableLocalConstraint(table, constraintName, constraint);
+ }
if (!replay) {
- logAddConstraint(tableNameInfo, constraint);
+ logItem = submitAddConstraint(tableNameInfo, constraint);
}
LOG.info("Added constraint {} on table {}", constraintName, key);
} finally {
writeUnlock();
+ if (acquireFrontendAdmission) {
+ frontendAdmissionLock.unlock();
+ }
+ }
+ return logItem;
+ }
+
+ public void acquireFrontendAdmission() throws DdlException {
+ try {
+ if (!frontendAdmissionLock.tryLock(
+ Config.catalog_try_lock_timeout_ms,
TimeUnit.MILLISECONDS)) {
+ throw new DdlException("Failed to acquire frontend admission
lock. Try again");
+ }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new DdlException("Interrupted while acquiring frontend
admission lock", e);
+ }
+ boolean admitted = false;
+ try {
+ readLock();
+ try {
+ for (Map<String, Constraint> tableConstraints :
constraintsMap.values()) {
Review Comment:
[P1] Include recycled mappings in frontend admission
This scans only the live manager index, but a non-force DROP removes that
entry while deliberately retaining the mapping in the table's `TableAttributes`
inside `CatalogRecycleBin`; the new recovery test even asserts the entry is
absent after DROP and rebuilt on RECOVER. Recycle-bin images serialize that
table through Gson, so ADD FRONTEND now succeeds in this interval even though
an older candidate still cannot deserialize the retained
`DistributionMappingConstraint` subtype when it loads the leader image. DROP
DATABASE has the same hidden-copy state. Please keep a capability flag/count
that includes serialized recycle-bin mappings (or scan them under the same
admission fence) until those copies are erased, and cover DROP-without-FORCE
followed by old-FE admission and recovery.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java:
##########
@@ -5996,6 +6002,18 @@ private void renameColumn(Database db, OlapTable table,
String colName,
if (partitionInfo.getPartitionColumns().stream().anyMatch(c ->
c.getName().equalsIgnoreCase(colName))) {
throw new DdlException("Renaming partition columns has problems,
forbidden in current Doris version");
}
+ String mappingConstraint =
+
constraintManager.findDistributionMappingConstraintWithColumn(table, colName);
+ if (mappingConstraint == null) {
+ mappingConstraint = constraintManager.findConstraintWithColumn(
Review Comment:
[P1] Match ordinary constraint columns case-insensitively
This fallback uses `findConstraintWithColumn`, whose PK/UK/FK branches call
case-sensitive `Collection.contains`, while the actual schema rename resolves
`colName` through a case-insensitive column map. Thus a constraint stored on
`k2` does not block `RENAME COLUMN K2 k3`: the schema removes `k2` but the
persisted constraint keeps that name, and recreating `k2` can reactivate a
false uniqueness/FK proof on the replacement column. The mapping-specific
branch already uses `equalsIgnoreCase`; please apply the same identifier
semantics to ordinary constraints and add alternate-case PK/UK/FK rename
coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/plugin/PluginDrivenExternalCatalog.java:
##########
@@ -935,12 +950,8 @@ public void dropColumn(TableIf dorisTable, String
columnName) throws UserExcepti
ConnectorMetadata metadata = PluginDrivenMetadata.get(session,
connector);
ConnectorTableHandle handle = resolveAlterHandle(externalTable,
session, metadata);
long updateTime = System.currentTimeMillis();
- try {
- metadata.dropColumn(session, handle, columnName);
- } catch (DorisConnectorException e) {
- throw new DdlException(e.getMessage(), e);
- }
- afterExternalDdl(externalTable, updateTime);
+ executeSchemaMutation(externalTable, updateTime,
Review Comment:
[P1] Preserve constraints across plugin column DDL
`executeSchemaMutation` only orders this remote change against constraint
ADD/DROP; it never validates an already committed constraint.
`PLUGIN_EXTERNAL_TABLE` DDL reaches this method directly and bypasses
`SchemaChangeHandler.processDropColumn`, so `ADD CONSTRAINT pk PRIMARY KEY(c)`
can finish and a later `DROP COLUMN c` still succeeds remotely and refreshes
the cache while `pk` continues naming the missing column. RENAME has the same
stale-name behavior, and recreating `c` can reactivate a false optimizer proof.
Please check the centralized constraints inside the same mutation protocol
before the remote DROP/RENAME (or atomically rewrite and journal them), with
existing-constraint and ADD-before-DDL 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]
