Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature] Support Column rename for unique/foreign key constraints #47851

Merged
merged 6 commits into from
Jul 12, 2024
Merged

[Feature] Support Column rename for unique/foreign key constraints #47851

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7579237
fix
gengjun-git Jul 2, 2024
b668508
fix
gengjun-git Jul 5, 2024
260fe43
fix
gengjun-git Jul 5, 2024
f739063
fix
gengjun-git Jul 5, 2024
c4428ee
fix
gengjun-git Jul 5, 2024
118af78
fix
gengjun-git Jul 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions fe/fe-core/src/main/java/com/starrocks/alter/SchemaChangeHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2560,8 +2560,8 @@ public void updateTableConstraint(Database db, String tableName, Map<String, Str
}
} catch (SemanticException e) {
throw new DdlException(
String.format("analyze table unique constraint:%s failed",
properties.get(PropertyAnalyzer.PROPERTIES_UNIQUE_CONSTRAINT)), e);
String.format("analyze table unique constraint:%s failed, msg: %s",
properties.get(PropertyAnalyzer.PROPERTIES_UNIQUE_CONSTRAINT), e.getDetailMsg()), e);
}
}
if (properties.containsKey(PropertyAnalyzer.PROPERTIES_FOREIGN_KEY_CONSTRAINT)) {
Expand All @@ -2580,8 +2580,8 @@ public void updateTableConstraint(Database db, String tableName, Map<String, Str
}
} catch (SemanticException e) {
throw new DdlException(
String.format("analyze table foreign key constraint:%s failed",
properties.get(PropertyAnalyzer.PROPERTIES_FOREIGN_KEY_CONSTRAINT)), e);
String.format("analyze table foreign key constraint:%s failed, msg: %s",
properties.get(PropertyAnalyzer.PROPERTIES_FOREIGN_KEY_CONSTRAINT), e.getDetailMsg()), e);
}
}
}
Expand Down
76 changes: 59 additions & 17 deletions fe/fe-core/src/main/java/com/starrocks/catalog/ForeignKeyConstraint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,12 @@
import com.google.common.collect.Streams;
import com.starrocks.common.Pair;
import com.starrocks.server.GlobalStateMgr;
import com.starrocks.sql.common.MetaUtils;
import org.apache.commons.lang.math.NumberUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.regex.Matcher;
Expand All @@ -52,15 +54,13 @@ public class ForeignKeyConstraint {
// table with foreign key, it only used for materialized view.
private final BaseTableInfo childTableInfo;

// here id is preferred, but meta of column does not have id.
// have to use name here, so column rename is not supported
// eg: [column1 -> column1', column2 -> column2']
private final List<Pair<String, String>> columnRefPairs;
private final List<Pair<ColumnId, ColumnId>> columnRefPairs;

public ForeignKeyConstraint(
BaseTableInfo parentTableInfo,
BaseTableInfo childTableInfo,
List<Pair<String, String>> columnRefPairs) {
List<Pair<ColumnId, ColumnId>> columnRefPairs) {
this.parentTableInfo = parentTableInfo;
this.childTableInfo = childTableInfo;
this.columnRefPairs = columnRefPairs;
Expand All @@ -74,10 +74,55 @@ public BaseTableInfo getChildTableInfo() {
return childTableInfo;
}

public List<Pair<String, String>> getColumnRefPairs() {
public List<Pair<ColumnId, ColumnId>> getColumnRefPairs() {
return columnRefPairs;
}

public List<Pair<String, String>> getColumnNameRefPairs(Table defaultChildTable) {
Table parentTable = getParentTable();
Table childTable = defaultChildTable;
if (childTableInfo != null) {
childTable = getChildTable();
}
List<Pair<String, String>> result = new ArrayList<>(columnRefPairs.size());
for (Pair<ColumnId, ColumnId> pair : columnRefPairs) {
Column childColumn = childTable.getColumn(pair.first);
if (childColumn == null) {
LOG.warn("can not find column by column id: {} in table: {}, the column may have been dropped",
pair.first, childTableInfo);
continue;
}
Column parentColumn = parentTable.getColumn(pair.second);
if (parentColumn == null) {
LOG.warn("can not find column by column id: {} in table: {}, the column may have been dropped",
pair.second, parentTableInfo);
continue;
}

result.add(Pair.create(childColumn.getName(), parentColumn.getName()));
}

return result;
}

private Table getParentTable() {
if (parentTableInfo.isInternalCatalog()) {
return MetaUtils.getTable(parentTableInfo.getDbId(), parentTableInfo.getTableId());
} else {
return MetaUtils.getTable(parentTableInfo.getCatalogName(), parentTableInfo.getDbName(),
parentTableInfo.getTableName());
}
}

private Table getChildTable() {
if (childTableInfo.isInternalCatalog()) {
return MetaUtils.getTable(childTableInfo.getDbId(), childTableInfo.getTableId());
} else {
return MetaUtils.getTable(childTableInfo.getCatalogName(), childTableInfo.getDbName(),
childTableInfo.getTableName());
}
}

// for olap table, the format is: (column1, column2) REFERENCES default_catalog.dbid.tableid(column1', column2')
// for materialized view, the format is: catalog1.dbName1.tableName1(column1, column2) REFERENCES
// catalog2.dbName2.tableName2(column1', column2')
Expand Down Expand Up @@ -127,10 +172,10 @@ public static List<ForeignKeyConstraint> parse(String foreignKeyConstraintDescs)
String targetTablePath = foreignKeyMatcher.group(6);
String targetColumns = foreignKeyMatcher.group(8);

List<String> childColumns = Arrays.stream(sourceColumns.split(",")).
map(String::trim).collect(Collectors.toList());
List<String> parentColumns = Arrays.stream(targetColumns.split(",")).
map(String::trim).collect(Collectors.toList());
List<ColumnId> childColumns = Arrays.stream(sourceColumns.split(",")).
map(colStr -> ColumnId.create(colStr.trim())).collect(Collectors.toList());
List<ColumnId> parentColumns = Arrays.stream(targetColumns.split(",")).
map(colStr -> ColumnId.create(colStr.trim())).collect(Collectors.toList());

String[] targetTableParts = targetTablePath.split("\\.");
Preconditions.checkState(targetTableParts.length == 3);
Expand Down Expand Up @@ -160,7 +205,7 @@ public static List<ForeignKeyConstraint> parse(String foreignKeyConstraintDescs)
}
}

List<Pair<String, String>> columnRefPairs =
List<Pair<ColumnId, ColumnId>> columnRefPairs =
Streams.zip(childColumns.stream(), parentColumns.stream(), Pair::create).collect(Collectors.toList());
foreignKeyConstraints.add(new ForeignKeyConstraint(parentTableInfo, childTableInfo, columnRefPairs));
}
Expand Down Expand Up @@ -194,9 +239,7 @@ private static BaseTableInfo getTableBaseInfo(String catalogName, String db, Str
return baseTableInfo;
}

public static String getShowCreateTableConstraintDesc(List<ForeignKeyConstraint> constraints) {
StringBuilder sb = new StringBuilder();

public static String getShowCreateTableConstraintDesc(OlapTable baseTable, List<ForeignKeyConstraint> constraints) {
List<String> constraintStrs = Lists.newArrayList();
for (ForeignKeyConstraint constraint : constraints) {
BaseTableInfo parentTableInfo = constraint.getParentTableInfo();
Expand All @@ -207,22 +250,21 @@ public static String getShowCreateTableConstraintDesc(List<ForeignKeyConstraint>
constraintSb.append(childTableInfo.getReadableString());
}
constraintSb.append("(");
String baseColumns = Joiner.on(",").join(constraint.getColumnRefPairs()
String baseColumns = Joiner.on(",").join(constraint.getColumnNameRefPairs(baseTable)
.stream().map(pair -> pair.first).collect(Collectors.toList()));
constraintSb.append(baseColumns);
constraintSb.append(")");
constraintSb.append(" REFERENCES ");
constraintSb.append(parentTableInfo.getReadableString());

constraintSb.append("(");
String parentColumns = Joiner.on(",").join(constraint.getColumnRefPairs()
String parentColumns = Joiner.on(",").join(constraint.getColumnNameRefPairs(baseTable)
.stream().map(pair -> pair.second).collect(Collectors.toList()));
constraintSb.append(parentColumns);
constraintSb.append(")");
constraintStrs.add(constraintSb.toString());
}

sb.append(Joiner.on(";").join(constraintStrs));
return sb.toString();
return Joiner.on(";").join(constraintStrs);
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most risky bug in this code is:
Potential NullPointerException due to missing null checks for the tables returned by MetaUtils.getTable.

You can modify the code like this:

private Table getParentTable() {
    Table table;
    if (parentTableInfo.isInternalCatalog()) {
        table = MetaUtils.getTable(parentTableInfo.getDbId(), parentTableInfo.getTableId());
    } else {
        table = MetaUtils.getTable(parentTableInfo.getCatalogName(), parentTableInfo.getDbName(),
                parentTableInfo.getTableName());
    }
    if (table == null) {
        LOG.warn("Parent table {} not found", parentTableInfo);
        throw new IllegalStateException("Parent table not found");
    }
    return table;
}

private Table getChildTable() {
    Table table;
    if (childTableInfo.isInternalCatalog()) {
        table = MetaUtils.getTable(childTableInfo.getDbId(), childTableInfo.getTableId());
    } else {
        table = MetaUtils.getTable(childTableInfo.getCatalogName(), childTableInfo.getDbName(),
                childTableInfo.getTableName());
    }
    if (table == null) {
        LOG.warn("Child table {} not found", childTableInfo);
        throw new IllegalStateException("Child table not found");
    }
    return table;
}

2 changes: 1 addition & 1 deletion fe/fe-core/src/main/java/com/starrocks/catalog/MaterializedView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1278,7 +1278,7 @@ public String getMaterializedViewDdlStmt(boolean simple, boolean isReplay) {
sb.append("\"")
.append(PropertyAnalyzer.PROPERTIES_FOREIGN_KEY_CONSTRAINT)
.append("\" = \"")
.append(ForeignKeyConstraint.getShowCreateTableConstraintDesc(getForeignKeyConstraints()))
.append(ForeignKeyConstraint.getShowCreateTableConstraintDesc(this, getForeignKeyConstraints()))
.append("\"");
} else if (name.equalsIgnoreCase(PropertyAnalyzer.PROPERTIES_STORAGE_COOLDOWN_TIME)) {
sb.append("\"").append(PropertyAnalyzer.PROPERTIES_STORAGE_COOLDOWN_TIME)
Expand Down
9 changes: 5 additions & 4 deletions fe/fe-core/src/main/java/com/starrocks/catalog/OlapTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2711,8 +2711,8 @@ public List<UniqueConstraint> getUniqueConstraints() {
}
if (keysType == KeysType.UNIQUE_KEYS || keysType == KeysType.PRIMARY_KEYS) {
uniqueConstraints.add(
new UniqueConstraint(null, null, null, getKeyColumns().stream().map(Column::getName).collect(
Collectors.toList())));
new UniqueConstraint(this, getKeyColumns()
.stream().map(Column::getColumnId).collect(Collectors.toList())));
}
if (tableProperty != null && tableProperty.getUniqueConstraints() != null) {
uniqueConstraints.addAll(tableProperty.getUniqueConstraints());
Expand Down Expand Up @@ -3093,14 +3093,15 @@ public Map<String, String> getUniqueProperties() {
// unique constraint
String uniqueConstraint = tableProperties.get(PropertyAnalyzer.PROPERTIES_UNIQUE_CONSTRAINT);
if (!Strings.isNullOrEmpty(uniqueConstraint)) {
properties.put(PropertyAnalyzer.PROPERTIES_UNIQUE_CONSTRAINT, uniqueConstraint);
properties.put(PropertyAnalyzer.PROPERTIES_UNIQUE_CONSTRAINT,
UniqueConstraint.getShowCreateTableConstraintDesc(getTableProperty().getUniqueConstraints()));
}

// foreign key constraint
String foreignKeyConstraint = tableProperties.get(PropertyAnalyzer.PROPERTIES_FOREIGN_KEY_CONSTRAINT);
if (!Strings.isNullOrEmpty(foreignKeyConstraint)) {
properties.put(PropertyAnalyzer.PROPERTIES_FOREIGN_KEY_CONSTRAINT,
ForeignKeyConstraint.getShowCreateTableConstraintDesc(getForeignKeyConstraints()));
ForeignKeyConstraint.getShowCreateTableConstraintDesc(this, getForeignKeyConstraints()));
}

// storage type
Expand Down
93 changes: 73 additions & 20 deletions fe/fe-core/src/main/java/com/starrocks/catalog/UniqueConstraint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,15 @@
import com.google.common.base.Joiner;
import com.google.common.base.Strings;
import com.google.common.collect.Lists;
import com.starrocks.analysis.TableName;
import com.starrocks.common.Pair;
import com.starrocks.server.GlobalStateMgr;
import com.starrocks.sql.analyzer.SemanticException;
import com.starrocks.sql.common.MetaUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import java.util.Set;
Expand All @@ -35,22 +39,47 @@
// a table may have multi unique constraints.
public class UniqueConstraint {
private static final Logger LOG = LogManager.getLogger(UniqueConstraint.class);
// here id is preferred, but meta of column does not have id.
// have to use name here, so column rename is not supported
private final List<String> uniqueColumns;
private final List<ColumnId> uniqueColumns;

private final String catalogName;
private final String dbName;
private final String tableName;
private String catalogName;
private String dbName;
private String tableName;

public UniqueConstraint(String catalogName, String dbName, String tableName, List<String> uniqueColumns) {
private Table referencedTable;

public UniqueConstraint(String catalogName, String dbName, String tableName, List<ColumnId> uniqueColumns) {
this.catalogName = catalogName;
this.dbName = dbName;
this.tableName = tableName;
this.uniqueColumns = uniqueColumns;
}

public List<String> getUniqueColumns() {
// Used for primaryKey/uniqueKey table to create default uniqueConstraints.
public UniqueConstraint(Table referencedTable, List<ColumnId> uniqueColumns) {
this.referencedTable = referencedTable;
this.uniqueColumns = uniqueColumns;
}

public List<String> getUniqueColumnNames() {
Table targetTable;
if (referencedTable != null) {
targetTable = referencedTable;
} else {
targetTable = MetaUtils.getTable(catalogName, dbName, tableName);
}
List<String> result = new ArrayList<>(uniqueColumns.size());
for (ColumnId columnId : uniqueColumns) {
Column column = targetTable.getColumn(columnId);
if (column == null) {
LOG.warn("Can not find column by column id: {}, the column may have been dropped", columnId);
continue;
}
result.add(column.getName());
}
return result;
}

public List<ColumnId> getUniqueColumns() {
return uniqueColumns;
}

Expand All @@ -66,7 +95,8 @@ public boolean isMatch(Table parentTable, Set<String> foreignKeys) {
return false;
}
}
Set<String> uniqueColumnSet = uniqueColumns.stream().map(String::toLowerCase).collect(Collectors.toSet());
Set<String> uniqueColumnSet = getUniqueColumnNames().stream().map(String::toLowerCase)
.collect(Collectors.toSet());
return uniqueColumnSet.equals(foreignKeys);
}

Expand All @@ -81,10 +111,30 @@ public String toString() {
if (tableName != null) {
sb.append(tableName).append(".");
}
sb.append(Joiner.on(",").join(uniqueColumns));
sb.append(Joiner.on(",").join(getUniqueColumns()));
return sb.toString();
}

public static String getShowCreateTableConstraintDesc(List<UniqueConstraint> constraints) {
List<String> constraintStrs = Lists.newArrayList();
for (UniqueConstraint constraint : constraints) {
StringBuilder constraintSb = new StringBuilder();
if (constraint.catalogName != null) {
constraintSb.append(constraint.catalogName).append(".");
}
if (constraint.dbName != null) {
constraintSb.append(constraint.dbName).append(".");
}
if (constraint.tableName != null) {
constraintSb.append(constraint.tableName).append(".");
}
constraintSb.append(Joiner.on(",").join(constraint.getUniqueColumnNames()));
constraintStrs.add(constraintSb.toString());
}

return Joiner.on(";").join(constraintStrs);
}

public String getCatalogName() {
return catalogName;
}
Expand All @@ -97,8 +147,8 @@ public String getTableName() {
return tableName;
}

public static List<UniqueConstraint> parse(String catalogName, String dbName, String tableName,
String constraintDescs) {
public static List<UniqueConstraint> parse(String defaultCatalogName, String defaultDbName,
String defaultTableName, String constraintDescs) {
if (Strings.isNullOrEmpty(constraintDescs)) {
return null;
}
Expand All @@ -108,17 +158,20 @@ public static List<UniqueConstraint> parse(String catalogName, String dbName, St
if (Strings.isNullOrEmpty(constraintDesc)) {
continue;
}
String[] uniqueColumns = constraintDesc.split(",");
List<String> columnNames =
Arrays.stream(uniqueColumns).map(String::trim).collect(Collectors.toList());
parseUniqueConstraintColumns(catalogName, dbName, tableName, columnNames, uniqueConstraints);
Pair<TableName, List<String>> descResult = parseUniqueConstraintDesc(
defaultCatalogName, defaultDbName, defaultTableName, constraintDesc);
uniqueConstraints.add(new UniqueConstraint(descResult.first.getCatalog(),
descResult.first.getDb(), descResult.first.getTbl(),
descResult.second.stream().map(ColumnId::create).collect(Collectors.toList())));
}
return uniqueConstraints;
}

private static void parseUniqueConstraintColumns(String defaultCatalogName, String defaultDbName,
String defaultTableName, List<String> columnNames,
List<UniqueConstraint> uniqueConstraints) {
// result if a pair, the fist value is TableName(catalogName, dbName, tableName), the second value is columns
public static Pair<TableName, List<String>> parseUniqueConstraintDesc(String defaultCatalogName, String defaultDbName,
String defaultTableName, String constraintDesc) {
String[] uniqueColumns = constraintDesc.split(",");
List<String> columnNames = Arrays.stream(uniqueColumns).map(String::trim).collect(Collectors.toList());
String catalogName = null;
String dbName = null;
String tableName = null;
Expand Down Expand Up @@ -172,6 +225,6 @@ private static void parseUniqueConstraintColumns(String defaultCatalogName, Stri
tableName = defaultTableName;
}

uniqueConstraints.add(new UniqueConstraint(catalogName, dbName, tableName, uniqueConstraintColumns));
return Pair.create(new TableName(catalogName, dbName, tableName), uniqueConstraintColumns);
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The most risky bug in this code is:
Using getUniqueColumns() instead of getUniqueColumnNames() in the toString method.

You can modify the code like this:

public String toString() {
    StringBuilder sb = new StringBuilder();
    if (tableName != null) {
        sb.append(tableName).append(".");
    }
    sb.append(Joiner.on(",").join(getUniqueColumnNames())); // Change from getUniqueColumns() to getUniqueColumnNames()
    return sb.toString();
}

Loading
Loading