You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2020/10/06 15:50:19 UTC
[GitHub] [incubator-doris] morningman commented on a change in pull request #4699: Support Alter Table Clause For External Table
morningman commented on a change in pull request #4699:
URL: https://github.com/apache/incubator-doris/pull/4699#discussion_r500402902
##########
File path: fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java
##########
@@ -172,71 +256,18 @@ public void processAlterTable(AlterTableStmt stmt) throws UserException {
ErrorReport.reportDdlException(ErrorCode.ERR_BAD_TABLE_ERROR, tableName);
}
- if (table.getType() != TableType.OLAP) {
- throw new DdlException("Do not support alter non-OLAP table[" + tableName + "]");
- }
- OlapTable olapTable = (OlapTable) table;
- stmt.rewriteAlterClause(olapTable);
-
- // check conflict alter ops first
- alterClauses = stmt.getOps();
- AlterOperations currentAlterOps = new AlterOperations();
- currentAlterOps.checkConflict(alterClauses);
-
- // check cluster capacity and db quota, only need to check once.
- if (currentAlterOps.needCheckCapacity()) {
- Catalog.getCurrentSystemInfo().checkClusterCapacity(clusterName);
- db.checkQuota();
- }
-
- if (olapTable.getState() != OlapTableState.NORMAL) {
- throw new DdlException(
- "Table[" + table.getName() + "]'s state is not NORMAL. Do not allow doing ALTER ops");
- }
-
- if (currentAlterOps.hasSchemaChangeOp()) {
- // if modify storage type to v2, do schema change to convert all related tablets to segment v2 format
- schemaChangeHandler.process(alterClauses, clusterName, db, olapTable);
- } else if (currentAlterOps.hasRollupOp()) {
- materializedViewHandler.process(alterClauses, clusterName, db, olapTable);
- } else if (currentAlterOps.hasPartitionOp()) {
- Preconditions.checkState(alterClauses.size() == 1);
- AlterClause alterClause = alterClauses.get(0);
- if (alterClause instanceof DropPartitionClause) {
- if (!((DropPartitionClause) alterClause).isTempPartition()) {
- DynamicPartitionUtil.checkAlterAllowed((OlapTable) db.getTable(tableName));
- }
- Catalog.getCurrentCatalog().dropPartition(db, olapTable, ((DropPartitionClause) alterClause));
- } else if (alterClause instanceof ReplacePartitionClause) {
- Catalog.getCurrentCatalog().replaceTempPartition(db, tableName, (ReplacePartitionClause) alterClause);
- } else if (alterClause instanceof ModifyPartitionClause) {
- ModifyPartitionClause clause = ((ModifyPartitionClause) alterClause);
- // expand the partition names if it is 'Modify Partition(*)'
- if (clause.isNeedExpand()) {
- List<String> partitionNames = clause.getPartitionNames();
- partitionNames.clear();
- for (Partition partition : olapTable.getPartitions()) {
- partitionNames.add(partition.getName());
- }
- }
- Map<String, String> properties = clause.getProperties();
- if (properties.containsKey(PropertyAnalyzer.PROPERTIES_INMEMORY)) {
- needProcessOutsideDatabaseLock = true;
- } else {
- List<String> partitionNames = clause.getPartitionNames();
- modifyPartitionsProperty(db, olapTable, partitionNames, properties);
- }
- } else if (alterClause instanceof AddPartitionClause) {
- needProcessOutsideDatabaseLock = true;
- } else {
- throw new DdlException("Invalid alter operation: " + alterClause.getOpType());
- }
- } else if (currentAlterOps.hasRenameOp()) {
- processRename(db, olapTable, alterClauses);
- } else if (currentAlterOps.contains(AlterOpType.MODIFY_TABLE_PROPERTY_SYNC)) {
- needProcessOutsideDatabaseLock = true;
- } else {
- throw new DdlException("Invalid alter operations: " + currentAlterOps);
+ switch (table.getType()) {
+ case OLAP:
+ OlapTable olapTable = (OlapTable) table;
+ needProcessOutsideDatabaseLock = processAlterOlapTable(stmt, olapTable, alterClauses, clusterName, db);
+ break;
+ case ODBC:
+ case MYSQL:
+ case ELASTICSEARCH:
+ processAlterExternalTable(stmt, table, db);
+ break;
Review comment:
use `return` instead of `break` is more clear
##########
File path: fe/fe-core/src/main/java/org/apache/doris/alter/AlterHandler.java
##########
@@ -380,6 +381,12 @@ public void start() {
public abstract void process(List<AlterClause> alterClauses, String clusterName, Database db, OlapTable olapTable)
throws UserException;
+ /*
+ * entry function. handle alter ops for external table
+ */
+ public void process(List<AlterClause> alterClauses, Database db, Table externalTable)
Review comment:
`processExternalTable` is more clear
##########
File path: fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java
##########
@@ -132,6 +132,30 @@ private void processAddColumn(AddColumnClause alterClause, OlapTable olapTable,
indexSchemaMap, newColNameSet);
}
+ private void processAddColumn(AddColumnClause alterClause, Table externalTable) throws DdlException {
+ Column column = alterClause.getColumn();
+ ColumnPosition columnPos = alterClause.getColPos();
+ Set<String> newColNameSet = Sets.newHashSet(column.getName());
+
+ List<Column> modIndexSchema = externalTable.getBaseSchema();
+ addColumnInternal(externalTable, column, columnPos, modIndexSchema, newColNameSet);
+ externalTable.setNewFullSchema(modIndexSchema);
Review comment:
Should not set new schema to `externalTable` here directly.
The other alter clause may throw exception and the entire alter operation will fail, and you have to rollback all changes to the table.
A correct way to to modify a "copied" schema of external table, and set the new schema at the end of all alter clauses.
##########
File path: fe/fe-core/src/main/java/org/apache/doris/catalog/Catalog.java
##########
@@ -5045,6 +5046,36 @@ public void renameTable(Database db, OlapTable table, TableRenameClause tableRen
LOG.info("rename table[{}] to {}", tableName, newTableName);
}
+ public void renameTable(Database db, Table table, TableRenameClause tableRenameClause) throws DdlException {
+ String tableName = table.getName();
+ String newTableName = tableRenameClause.getNewTableName();
+ if (tableName.equals(newTableName)) {
+ throw new DdlException("Same table name");
+ }
+
+ // check if name is already used
+ if (db.getTable(newTableName) != null) {
+ throw new DdlException("Table name[" + newTableName + "] is already used");
+ }
+
+ table.setName(newTableName);
+
+ db.dropTable(tableName);
+ db.createTable(table);
+
+ TableInfo tableInfo = TableInfo.createForTableRename(db.getId(), table.getId(), newTableName);
+ editLog.logTableRename(tableInfo);
+ LOG.info("rename table[{}] to {}", tableName, newTableName);
+ }
+
+ public void reflushTable(Database db, Table table) throws DdlException {
Review comment:
This is not a good way to alter external table.
You write 2 edit logs here, which will make this operation non-atomic。
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org