github-actions[bot] commented on code in PR #61118: URL: https://github.com/apache/doris/pull/61118#discussion_r2910242377
########## fe/fe-core/src/main/java/org/apache/doris/catalog/constraint/ConstraintManager.java: ########## @@ -0,0 +1,901 @@ +// 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. + +package org.apache.doris.catalog.constraint; + +import org.apache.doris.catalog.DatabaseIf; +import org.apache.doris.catalog.Env; +import org.apache.doris.catalog.TableIf; +import org.apache.doris.common.DdlException; +import org.apache.doris.common.io.Text; +import org.apache.doris.common.io.Writable; +import org.apache.doris.datasource.CatalogIf; +import org.apache.doris.info.TableNameInfo; +import org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.persist.AlterConstraintLog; +import org.apache.doris.persist.gson.GsonPostProcessable; +import org.apache.doris.persist.gson.GsonUtils; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.gson.annotations.SerializedName; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import java.io.DataInput; +import java.io.DataOutput; +import java.io.IOException; +import java.util.Collection; +import java.util.HashMap; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.Map.Entry; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.stream.Collectors; + +/** + * Centralized manager for all table constraints (PK, FK, UNIQUE). + * Constraints are indexed by fully qualified table name (catalog.db.table). + */ +public class ConstraintManager implements Writable, GsonPostProcessable { + + private static final Logger LOG = LogManager.getLogger(ConstraintManager.class); + + @SerializedName("cm") + private final ConcurrentHashMap<String, Map<String, Constraint>> constraintsMap + = new ConcurrentHashMap<>(); + + private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(); + + public ConstraintManager() { + } + + private static String toKey(TableNameInfo tni) { + return tni.getCtl() + "." + tni.getDb() + "." + tni.getTbl(); + } + + /** Returns true if no constraints are stored. */ + public boolean isEmpty() { + return constraintsMap.isEmpty(); + } + + private void readLock() { + lock.readLock().lock(); + } + + private void readUnlock() { + lock.readLock().unlock(); + } + + private void writeLock() { + lock.writeLock().lock(); + } + + private void writeUnlock() { + lock.writeLock().unlock(); + } + + /** + * Add a constraint to the specified table. + * For FK constraints, validates that the referenced PK exists + * and registers bidirectional reference via foreignTableInfos. + */ + public void addConstraint(TableNameInfo tableNameInfo, String constraintName, + Constraint constraint, boolean replay) { + String key = toKey(tableNameInfo); + writeLock(); + try { + if (!replay) { + validateTableAndColumns(tableNameInfo, constraint); + } + Map<String, Constraint> tableConstraints = constraintsMap.computeIfAbsent( + key, k -> new HashMap<>()); + checkConstraintNotExistence(constraintName, constraint, tableConstraints); + if (constraint instanceof ForeignKeyConstraint) { + registerForeignKeyReference( + tableNameInfo, (ForeignKeyConstraint) constraint); + } + tableConstraints.put(constraintName, constraint); + if (!replay) { + logAddConstraint(tableNameInfo, constraint); + } + LOG.info("Added constraint {} on table {}", constraintName, key); + } finally { + writeUnlock(); + } + } + + /** + * Drop a constraint from the specified table. + * For PK constraints, cascade-drops all referencing FKs. + * For FK constraints, updates the referenced PK's foreign table set. + */ + public void dropConstraint(TableNameInfo tableNameInfo, String constraintName, + boolean replay) { + String key = toKey(tableNameInfo); + writeLock(); + try { + Map<String, Constraint> tableConstraints = constraintsMap.get(key); + if (tableConstraints == null || !tableConstraints.containsKey(constraintName)) { + if (replay) { + LOG.warn("Constraint {} not found on table {} during replay, skipping", + constraintName, key); + return; + } + throw new AnalysisException(String.format( + "Unknown constraint %s on table %s.", + constraintName, key)); + } + Constraint constraint = tableConstraints.remove(constraintName); + cleanupConstraintReferences(tableNameInfo, constraint); + if (tableConstraints.isEmpty()) { + constraintsMap.remove(key); + } + if (!replay) { + logDropConstraint(tableNameInfo, constraint); + } + LOG.info("Dropped constraint {} from table {}", + constraintName, key); + } finally { + writeUnlock(); + } + } + + /** Returns an immutable copy of all constraints for the given table. */ + public Map<String, Constraint> getConstraints(TableNameInfo tableNameInfo) { + String key = toKey(tableNameInfo); + readLock(); + try { + Map<String, Constraint> tableConstraints + = constraintsMap.get(key); + if (tableConstraints == null) { + return ImmutableMap.of(); + } + return ImmutableMap.copyOf(tableConstraints); + } finally { + readUnlock(); + } + } + + /** Get a single constraint by name, or null if not found. */ + public Constraint getConstraint(TableNameInfo tableNameInfo, + String constraintName) { + String key = toKey(tableNameInfo); + readLock(); + try { + Map<String, Constraint> tableConstraints + = constraintsMap.get(key); + if (tableConstraints == null) { + return null; + } + return tableConstraints.get(constraintName); + } finally { + readUnlock(); + } + } + + /** Returns all PrimaryKeyConstraints for the given table. */ + public ImmutableList<PrimaryKeyConstraint> getPrimaryKeyConstraints( + TableNameInfo tableNameInfo) { + return getConstraintsByType(toKey(tableNameInfo), + PrimaryKeyConstraint.class); + } + + /** Returns all ForeignKeyConstraints for the given table. */ + public ImmutableList<ForeignKeyConstraint> getForeignKeyConstraints( + TableNameInfo tableNameInfo) { + return getConstraintsByType(toKey(tableNameInfo), + ForeignKeyConstraint.class); + } + + /** Returns all UniqueConstraints for the given table. */ + public ImmutableList<UniqueConstraint> getUniqueConstraints( + TableNameInfo tableNameInfo) { + return getConstraintsByType(toKey(tableNameInfo), + UniqueConstraint.class); + } + + /** + * Remove all constraints for a table and clean up bidirectional references. + * Called when a table is dropped. + */ + public void dropTableConstraints(TableNameInfo tableNameInfo) { + String key = toKey(tableNameInfo); + writeLock(); + try { + Map<String, Constraint> tableConstraints + = constraintsMap.remove(key); + if (tableConstraints == null) { + return; + } + for (Constraint constraint : tableConstraints.values()) { + cleanupConstraintReferences(tableNameInfo, constraint); + } + LOG.info("Dropped all constraints for table {}", key); + } finally { + writeUnlock(); + } + } + + /** + * Remove all constraints whose qualified table name starts with + * the given catalog prefix. Called when a catalog is dropped. + */ + public void dropCatalogConstraints(String catalogName) { + writeLock(); + try { + String prefix = catalogName + "."; + List<String> tablesToRemove = constraintsMap.keySet().stream() + .filter(k -> k.startsWith(prefix)) + .collect(Collectors.toList()); + for (String tableName : tablesToRemove) { + Map<String, Constraint> tableConstraints + = constraintsMap.remove(tableName); + if (tableConstraints != null) { + for (Constraint constraint : tableConstraints.values()) { + cleanupConstraintReferencesOutsideCatalog( + tableName, constraint, prefix); + } + } + } + LOG.info("Dropped all constraints for catalog {}", catalogName); + } finally { + writeUnlock(); + } + } + + /** + * Move constraints from oldTableInfo to newTableInfo + * and update all FK/PK references. Called when a table is renamed. + */ + public void renameTable(TableNameInfo oldTableInfo, Review Comment: **Bug (High): `renameTable()` is never called** This method correctly handles all aspects of table rename (moving constraint map entries, updating FK/PK cross-references), but it is never invoked from any rename path: - `Env.renameTable()` (line 5560) — does NOT call this - `Env.replayRenameTable()` (line 5621) — does NOT call this - External catalog rename (`Alter.java:409` → `ExternalCatalog.renameTable()`) — does NOT call this After a table rename, constraints remain keyed under the old qualified name and become unreachable. Suggested fix in `Env.renameTable()` before `editLog.logTableRename()`: ```java TableNameInfo oldInfo = new TableNameInfo(InternalCatalog.INTERNAL_CATALOG_NAME, db.getFullName(), oldTableName); TableNameInfo newInfo = new TableNameInfo(InternalCatalog.INTERNAL_CATALOG_NAME, db.getFullName(), newTableName); Env.getCurrentEnv().getConstraintManager().renameTable(oldInfo, newInfo); ``` And similarly in `replayRenameTable()` and the external catalog rename path. ########## fe/fe-core/src/main/java/org/apache/doris/info/TableNameInfo.java: ########## @@ -149,6 +149,29 @@ public TableNameInfo(TableIf tableIf) throws AnalysisException { this.tbl = tableName; } + /** + * Create TableNameInfo from a TableIf, returning null if the table + * lacks a database or catalog (e.g., standalone tables in unit tests). + */ + public static TableNameInfo createOrNull(TableIf tableIf) { + if (tableIf == null || StringUtils.isEmpty(tableIf.getName())) { + return null; + } + DatabaseIf db = tableIf.getDatabase(); Review Comment: **Bug (Critical): `hashCode()`/`equals()` contract violation in this class** While this new `createOrNull` method is fine, the existing `equals()` (line 275) and `hashCode()` (line 283) methods in this same file have a contract violation that is critical for this PR since `TableNameInfo` is now used extensively as a key identifier. `equals()` delegates to `toString()` comparison, but `toString()` omits the catalog name for internal catalog (`"internal"`). Meanwhile, `hashCode()` uses `Objects.hash(tbl, db, ctl)` which always includes catalog. Example: `{ctl="internal", db="mydb", tbl="t"}` and `{ctl=null, db="mydb", tbl="t"}` will be `equals()` (both produce `"mydb.t"`) but have different `hashCode()` values. Fix: normalize catalog in both methods: ```java private static String normalizeCatalog(String c) { return (c == null || InternalCatalog.INTERNAL_CATALOG_NAME.equals(c)) ? null : c; } @Override public boolean equals(Object other) { if (this == other) return true; if (other == null || getClass() != other.getClass()) return false; TableNameInfo that = (TableNameInfo) other; return Objects.equals(normalizeCatalog(ctl), normalizeCatalog(that.ctl)) && Objects.equals(db, that.db) && Objects.equals(tbl, that.tbl); } @Override public int hashCode() { return Objects.hash(tbl, db, normalizeCatalog(ctl)); } ``` ########## fe/fe-core/src/main/java/org/apache/doris/catalog/constraint/ConstraintManager.java: ########## @@ -0,0 +1,901 @@ +// 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. + +package org.apache.doris.catalog.constraint; + +import org.apache.doris.catalog.DatabaseIf; +import org.apache.doris.catalog.Env; +import org.apache.doris.catalog.TableIf; +import org.apache.doris.common.DdlException; +import org.apache.doris.common.io.Text; +import org.apache.doris.common.io.Writable; +import org.apache.doris.datasource.CatalogIf; +import org.apache.doris.info.TableNameInfo; +import org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.persist.AlterConstraintLog; +import org.apache.doris.persist.gson.GsonPostProcessable; +import org.apache.doris.persist.gson.GsonUtils; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.gson.annotations.SerializedName; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import java.io.DataInput; +import java.io.DataOutput; +import java.io.IOException; +import java.util.Collection; +import java.util.HashMap; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.Map.Entry; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.stream.Collectors; + +/** + * Centralized manager for all table constraints (PK, FK, UNIQUE). + * Constraints are indexed by fully qualified table name (catalog.db.table). + */ +public class ConstraintManager implements Writable, GsonPostProcessable { + + private static final Logger LOG = LogManager.getLogger(ConstraintManager.class); + + @SerializedName("cm") + private final ConcurrentHashMap<String, Map<String, Constraint>> constraintsMap + = new ConcurrentHashMap<>(); + + private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock(); + + public ConstraintManager() { + } + + private static String toKey(TableNameInfo tni) { + return tni.getCtl() + "." + tni.getDb() + "." + tni.getTbl(); + } + + /** Returns true if no constraints are stored. */ + public boolean isEmpty() { + return constraintsMap.isEmpty(); + } + + private void readLock() { + lock.readLock().lock(); + } + + private void readUnlock() { + lock.readLock().unlock(); + } + + private void writeLock() { + lock.writeLock().lock(); + } + + private void writeUnlock() { + lock.writeLock().unlock(); + } + + /** + * Add a constraint to the specified table. + * For FK constraints, validates that the referenced PK exists + * and registers bidirectional reference via foreignTableInfos. + */ + public void addConstraint(TableNameInfo tableNameInfo, String constraintName, + Constraint constraint, boolean replay) { + String key = toKey(tableNameInfo); + writeLock(); + try { + if (!replay) { + validateTableAndColumns(tableNameInfo, constraint); + } + Map<String, Constraint> tableConstraints = constraintsMap.computeIfAbsent( + key, k -> new HashMap<>()); + checkConstraintNotExistence(constraintName, constraint, tableConstraints); + if (constraint instanceof ForeignKeyConstraint) { + registerForeignKeyReference( + tableNameInfo, (ForeignKeyConstraint) constraint); + } + tableConstraints.put(constraintName, constraint); + if (!replay) { + logAddConstraint(tableNameInfo, constraint); + } + LOG.info("Added constraint {} on table {}", constraintName, key); + } finally { + writeUnlock(); + } + } + + /** + * Drop a constraint from the specified table. + * For PK constraints, cascade-drops all referencing FKs. + * For FK constraints, updates the referenced PK's foreign table set. + */ + public void dropConstraint(TableNameInfo tableNameInfo, String constraintName, + boolean replay) { + String key = toKey(tableNameInfo); + writeLock(); + try { + Map<String, Constraint> tableConstraints = constraintsMap.get(key); + if (tableConstraints == null || !tableConstraints.containsKey(constraintName)) { + if (replay) { + LOG.warn("Constraint {} not found on table {} during replay, skipping", + constraintName, key); + return; + } + throw new AnalysisException(String.format( + "Unknown constraint %s on table %s.", + constraintName, key)); + } + Constraint constraint = tableConstraints.remove(constraintName); + cleanupConstraintReferences(tableNameInfo, constraint); + if (tableConstraints.isEmpty()) { + constraintsMap.remove(key); + } + if (!replay) { + logDropConstraint(tableNameInfo, constraint); + } + LOG.info("Dropped constraint {} from table {}", + constraintName, key); + } finally { + writeUnlock(); + } + } + + /** Returns an immutable copy of all constraints for the given table. */ + public Map<String, Constraint> getConstraints(TableNameInfo tableNameInfo) { + String key = toKey(tableNameInfo); + readLock(); + try { + Map<String, Constraint> tableConstraints + = constraintsMap.get(key); + if (tableConstraints == null) { + return ImmutableMap.of(); + } + return ImmutableMap.copyOf(tableConstraints); + } finally { + readUnlock(); + } + } + + /** Get a single constraint by name, or null if not found. */ + public Constraint getConstraint(TableNameInfo tableNameInfo, + String constraintName) { + String key = toKey(tableNameInfo); + readLock(); + try { + Map<String, Constraint> tableConstraints + = constraintsMap.get(key); + if (tableConstraints == null) { + return null; + } + return tableConstraints.get(constraintName); + } finally { + readUnlock(); + } + } + + /** Returns all PrimaryKeyConstraints for the given table. */ + public ImmutableList<PrimaryKeyConstraint> getPrimaryKeyConstraints( + TableNameInfo tableNameInfo) { + return getConstraintsByType(toKey(tableNameInfo), + PrimaryKeyConstraint.class); + } + + /** Returns all ForeignKeyConstraints for the given table. */ + public ImmutableList<ForeignKeyConstraint> getForeignKeyConstraints( + TableNameInfo tableNameInfo) { + return getConstraintsByType(toKey(tableNameInfo), + ForeignKeyConstraint.class); + } + + /** Returns all UniqueConstraints for the given table. */ + public ImmutableList<UniqueConstraint> getUniqueConstraints( + TableNameInfo tableNameInfo) { + return getConstraintsByType(toKey(tableNameInfo), + UniqueConstraint.class); + } + + /** + * Remove all constraints for a table and clean up bidirectional references. + * Called when a table is dropped. + */ + public void dropTableConstraints(TableNameInfo tableNameInfo) { + String key = toKey(tableNameInfo); + writeLock(); + try { + Map<String, Constraint> tableConstraints + = constraintsMap.remove(key); + if (tableConstraints == null) { + return; + } + for (Constraint constraint : tableConstraints.values()) { + cleanupConstraintReferences(tableNameInfo, constraint); + } + LOG.info("Dropped all constraints for table {}", key); + } finally { + writeUnlock(); + } + } + + /** + * Remove all constraints whose qualified table name starts with + * the given catalog prefix. Called when a catalog is dropped. + */ + public void dropCatalogConstraints(String catalogName) { Review Comment: **Observation**: There's `dropCatalogConstraints()` (correctly called from `CatalogMgr.removeCatalog()`), `dropTableConstraints()`, but no `dropDatabaseConstraints()` method. Currently database drop works because `unprotectDropDb()` iterates all tables and calls `dropTableConstraints()` for each. However, this is fragile — if the iteration is ever short-circuited or if external database drops don't iterate tables, constraints would leak. Consider adding `dropDatabaseConstraints(String catalogName, String dbName)` for symmetry and robustness. ########## fe/fe-core/src/main/java/org/apache/doris/catalog/constraint/ConstraintManager.java: ########## @@ -0,0 +1,901 @@ +// 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. + +package org.apache.doris.catalog.constraint; + +import org.apache.doris.catalog.DatabaseIf; +import org.apache.doris.catalog.Env; +import org.apache.doris.catalog.TableIf; +import org.apache.doris.common.DdlException; +import org.apache.doris.common.io.Text; +import org.apache.doris.common.io.Writable; +import org.apache.doris.datasource.CatalogIf; +import org.apache.doris.info.TableNameInfo; +import org.apache.doris.nereids.exceptions.AnalysisException; +import org.apache.doris.persist.AlterConstraintLog; +import org.apache.doris.persist.gson.GsonPostProcessable; +import org.apache.doris.persist.gson.GsonUtils; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.gson.annotations.SerializedName; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import java.io.DataInput; +import java.io.DataOutput; +import java.io.IOException; +import java.util.Collection; +import java.util.HashMap; +import java.util.Iterator; +import java.util.List; +import java.util.Map; +import java.util.Map.Entry; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.locks.ReentrantReadWriteLock; +import java.util.stream.Collectors; + +/** + * Centralized manager for all table constraints (PK, FK, UNIQUE). + * Constraints are indexed by fully qualified table name (catalog.db.table). + */ +public class ConstraintManager implements Writable, GsonPostProcessable { Review Comment: **Minor**: `ConcurrentHashMap` is redundant here since all access is already guarded by the `ReentrantReadWriteLock` on line 58. A plain `HashMap` would be sufficient and avoids the marginal overhead of `ConcurrentHashMap`'s internal synchronization. The inner map is already a plain `HashMap`, so this is inconsistent. -- 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]
