From 98b1e82e2df6064818519fedd7457d50e9853a6b Mon Sep 17 00:00:00 2001 From: morningman Date: Tue, 16 Apr 2019 12:37:58 +0800 Subject: [PATCH 1/3] Support add columns to multiple rollup indexes in one ALTER operation --- .../doris/alter/SchemaChangeHandler.java | 38 +++++++++++-------- .../doris/analysis/AddColumnClause.java | 5 ++- .../doris/analysis/AddColumnsClause.java | 6 ++- .../java/org/apache/doris/catalog/Column.java | 17 +++++++++ 4 files changed, 47 insertions(+), 19 deletions(-) diff --git a/fe/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java b/fe/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java index 3391f8ba77a78f..b90d4514d203b2 100644 --- a/fe/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java +++ b/fe/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java @@ -533,8 +533,6 @@ private void addColumnInternal(OlapTable olapTable, Column newColumn, ColumnPosi if (KeysType.UNIQUE_KEYS == olapTable.getKeysType()) { // check if has default value. this should be done in Analyze phase // 1. add to base index first - // List modIndexSchema = indexSchemaMap.get(baseIndexId); - // checkAndAddColumn(modIndexSchema, newColumn, columnPos); List modIndexSchema; if (newColumn.isKey()) { // add key column to unique key table, should add to all rollups @@ -542,32 +540,33 @@ private void addColumnInternal(OlapTable olapTable, Column newColumn, ColumnPosi // add to all table including base and rollup for (Map.Entry> entry : indexSchemaMap.entrySet()) { modIndexSchema = entry.getValue(); - checkAndAddColumn(modIndexSchema, newColumn, columnPos); + boolean isBaseIdex = entry.getKey() == baseIndexId; + checkAndAddColumn(modIndexSchema, newColumn, columnPos, isBaseIdex); } } else { // 1. add to base table modIndexSchema = indexSchemaMap.get(baseIndexId); - checkAndAddColumn(modIndexSchema, newColumn, columnPos); + checkAndAddColumn(modIndexSchema, newColumn, columnPos, true); if (targetIndexId == -1L) { return; } // 2. add to rollup modIndexSchema = indexSchemaMap.get(targetIndexId); - checkAndAddColumn(modIndexSchema, newColumn, columnPos); + checkAndAddColumn(modIndexSchema, newColumn, columnPos, false); } } else if (KeysType.DUP_KEYS == olapTable.getKeysType()) { if (targetIndexId == -1L) { // check if has default value. this should be done in Analyze phase // 1. add to base index first List modIndexSchema = indexSchemaMap.get(baseIndexId); - checkAndAddColumn(modIndexSchema, newColumn, columnPos); + checkAndAddColumn(modIndexSchema, newColumn, columnPos, true); // no specified target index. return return; } else { // 2. add to rollup index List modIndexSchema = indexSchemaMap.get(targetIndexId); - checkAndAddColumn(modIndexSchema, newColumn, columnPos); + checkAndAddColumn(modIndexSchema, newColumn, columnPos, false); if (newColumn.isKey()) { /* @@ -575,17 +574,17 @@ private void addColumnInternal(OlapTable olapTable, Column newColumn, ColumnPosi * then put the column in base table as end key */ modIndexSchema = indexSchemaMap.get(baseIndexId); - checkAndAddColumn(modIndexSchema, newColumn, null); + checkAndAddColumn(modIndexSchema, newColumn, null, true); } else { modIndexSchema = indexSchemaMap.get(baseIndexId); - checkAndAddColumn(modIndexSchema, newColumn, columnPos); + checkAndAddColumn(modIndexSchema, newColumn, columnPos, true); } } } else { // check if has default value. this should be done in Analyze phase // 1. add to base index first List modIndexSchema = indexSchemaMap.get(baseIndexId); - checkAndAddColumn(modIndexSchema, newColumn, columnPos); + checkAndAddColumn(modIndexSchema, newColumn, columnPos, true); if (targetIndexId == -1L) { // no specified target index. return @@ -594,20 +593,29 @@ private void addColumnInternal(OlapTable olapTable, Column newColumn, ColumnPosi // 2. add to rollup index modIndexSchema = indexSchemaMap.get(targetIndexId); - checkAndAddColumn(modIndexSchema, newColumn, columnPos); + checkAndAddColumn(modIndexSchema, newColumn, columnPos, false); } } - private void checkAndAddColumn(List modIndexSchema, Column newColumn, ColumnPosition columnPos) - throws DdlException { + private void checkAndAddColumn(List modIndexSchema, Column newColumn, ColumnPosition columnPos, + boolean isBaseIndex) throws DdlException { int posIndex = -1; String newColName = newColumn.getName(); boolean hasPos = (columnPos != null && !columnPos.isFirst()); for (int i = 0; i < modIndexSchema.size(); i++) { Column col = modIndexSchema.get(i); if (col.getName().equalsIgnoreCase(newColName)) { - // already add - throw new DdlException("Duplicately add column[" + newColName + "]"); + if (!isBaseIndex || !col.isFromAddColumnOperation()) { + // if this is not a base index, we should check if user repeatedly add columns + throw new DdlException("Repeatedly add column[" + newColName + "]"); + } + // this is a base index, and the column we check here is added by previous 'add column clause' + // in same ALTER stmt. + // so here we will check if the 2 columns is exactly same. if not, throw exception + if (!col.equals(newColumn)) { + throw new DdlException("Repeatedly add same column[" + newColName + "] with different definition"); + } + continue; } if (hasPos) { // after the field diff --git a/fe/src/main/java/org/apache/doris/analysis/AddColumnClause.java b/fe/src/main/java/org/apache/doris/analysis/AddColumnClause.java index 95edafe95c08ff..fa5e4b74489dfe 100644 --- a/fe/src/main/java/org/apache/doris/analysis/AddColumnClause.java +++ b/fe/src/main/java/org/apache/doris/analysis/AddColumnClause.java @@ -24,11 +24,11 @@ import com.google.common.base.Strings; -import java.util.Map; - import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import java.util.Map; + // clause which is used to add one column to public class AddColumnClause extends AlterClause { private static final Logger LOG = LogManager.getLogger(AddColumnClause.class); @@ -83,6 +83,7 @@ public void analyze(Analyzer analyzer) throws AnalysisException { } column = columnDef.toColumn(); + column.setFromAddColumnOperation(true); } @Override diff --git a/fe/src/main/java/org/apache/doris/analysis/AddColumnsClause.java b/fe/src/main/java/org/apache/doris/analysis/AddColumnsClause.java index 81b108c86963f7..1bd54096ed0ec5 100644 --- a/fe/src/main/java/org/apache/doris/analysis/AddColumnsClause.java +++ b/fe/src/main/java/org/apache/doris/analysis/AddColumnsClause.java @@ -17,13 +17,13 @@ package org.apache.doris.analysis; -import com.google.common.collect.Lists; import org.apache.doris.catalog.Column; import org.apache.doris.common.AnalysisException; import org.apache.doris.common.ErrorCode; import org.apache.doris.common.ErrorReport; import com.google.common.base.Strings; +import com.google.common.collect.Lists; import java.util.List; import java.util.Map; @@ -69,7 +69,9 @@ public void analyze(Analyzer analyzer) throws AnalysisException { columns = Lists.newArrayList(); for (ColumnDef columnDef : columnDefs) { - columns.add(columnDef.toColumn()); + Column col = columnDef.toColumn(); + col.setFromAddColumnOperation(true); + columns.add(col); } } diff --git a/fe/src/main/java/org/apache/doris/catalog/Column.java b/fe/src/main/java/org/apache/doris/catalog/Column.java index af41d8ed6578d0..e0b97f1c5a272a 100644 --- a/fe/src/main/java/org/apache/doris/catalog/Column.java +++ b/fe/src/main/java/org/apache/doris/catalog/Column.java @@ -51,6 +51,10 @@ public class Column implements Writable { private ColumnStats stats; // cardinality and selectivity etc. + // This variable is used to represent if this column instance is generated from ALTER TABLE ADD COLUMN stmt. + // And it does not need to persist + private boolean isFromAddColumnOperation = false; + public Column() { this.name = ""; this.type = Type.NULL; @@ -177,6 +181,14 @@ public String getComment() { return comment; } + public void setFromAddColumnOperation(boolean isFromAddColumnOperation) { + this.isFromAddColumnOperation = isFromAddColumnOperation; + } + + public boolean isFromAddColumnOperation() { + return isFromAddColumnOperation; + } + public int getOlapColumnIndexSize() { PrimitiveType type = this.getDataType(); if (type == PrimitiveType.CHAR) { @@ -324,6 +336,11 @@ public boolean equals(Object obj) { if (this.getScale() != other.getScale()) { return false; } + + if (!comment.equals(other.getComment())) { + return false; + } + return true; } From 8de478300c204ea7747781b40fafed03a53d3bc5 Mon Sep 17 00:00:00 2001 From: morningman Date: Tue, 16 Apr 2019 14:37:42 +0800 Subject: [PATCH 2/3] fix bug --- fe/src/main/java/org/apache/doris/alter/AlterHandler.java | 2 +- .../java/org/apache/doris/alter/SchemaChangeHandler.java | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/fe/src/main/java/org/apache/doris/alter/AlterHandler.java b/fe/src/main/java/org/apache/doris/alter/AlterHandler.java index 2841069f6c7699..6bff6c45d436c5 100644 --- a/fe/src/main/java/org/apache/doris/alter/AlterHandler.java +++ b/fe/src/main/java/org/apache/doris/alter/AlterHandler.java @@ -67,7 +67,7 @@ protected void unlock() { } public AlterHandler(String name) { - super(name, 20000); + super(name, 10000); } protected void addAlterJob(AlterJob alterJob) { diff --git a/fe/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java b/fe/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java index b90d4514d203b2..2a7b9a824bacb4 100644 --- a/fe/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java +++ b/fe/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java @@ -615,8 +615,11 @@ private void checkAndAddColumn(List modIndexSchema, Column newColumn, Co if (!col.equals(newColumn)) { throw new DdlException("Repeatedly add same column[" + newColName + "] with different definition"); } - continue; + + // column already exist, return + return; } + if (hasPos) { // after the field if (col.getName().equalsIgnoreCase(columnPos.getLastCol())) { From 9e7c0764bcfc311c138ea59c56aeaf7519931ea1 Mon Sep 17 00:00:00 2001 From: morningman Date: Tue, 16 Apr 2019 15:11:29 +0800 Subject: [PATCH 3/3] change way --- .../doris/alter/SchemaChangeHandler.java | 36 ++++++++++--------- .../doris/analysis/AddColumnClause.java | 1 - .../doris/analysis/AddColumnsClause.java | 1 - .../java/org/apache/doris/catalog/Column.java | 12 ------- 4 files changed, 20 insertions(+), 30 deletions(-) diff --git a/fe/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java b/fe/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java index 2a7b9a824bacb4..3f4caa58f62459 100644 --- a/fe/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java +++ b/fe/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java @@ -103,8 +103,9 @@ private void processAddColumn(AddColumnClause alterClause, OlapTable olapTable, targetIndexId = olapTable.getIndexIdByName(targetIndexName); } + Set newColNameSet = Sets.newHashSet(column.getName()); addColumnInternal(olapTable, column, columnPos, targetIndexId, baseIndexId, - baseIndexName, indexSchemaMap); + baseIndexName, indexSchemaMap, newColNameSet); } private void processAddColumns(AddColumnsClause alterClause, OlapTable olapTable, @@ -113,10 +114,12 @@ private void processAddColumns(AddColumnsClause alterClause, OlapTable olapTable String targetIndexName = alterClause.getRollupName(); checkIndexExists(olapTable, targetIndexName); + Set newColNameSet = Sets.newHashSet(); for (Column column : columns) { if (column.isKey()) { checkKeyModificationIfInRandomDistributedTable(olapTable); } + newColNameSet.add(column.getName()); } String baseIndexName = olapTable.getName(); @@ -128,9 +131,9 @@ private void processAddColumns(AddColumnsClause alterClause, OlapTable olapTable targetIndexId = olapTable.getIndexIdByName(targetIndexName); } - for (Column columnDef : columns) { - addColumnInternal(olapTable, columnDef, null, targetIndexId, baseIndexId, - baseIndexName, indexSchemaMap); + for (Column column : columns) { + addColumnInternal(olapTable, column, null, targetIndexId, baseIndexId, + baseIndexName, indexSchemaMap, newColNameSet); } } @@ -488,7 +491,8 @@ private void processReorderColumn(ReorderColumnsClause alterClause, OlapTable ol private void addColumnInternal(OlapTable olapTable, Column newColumn, ColumnPosition columnPos, long targetIndexId, long baseIndexId, String baseIndexName, - Map> indexSchemaMap) throws DdlException { + Map> indexSchemaMap, + Set newColNameSet) throws DdlException { if (KeysType.AGG_KEYS == olapTable.getKeysType()) { if (newColumn.isKey() && newColumn.getAggregationType() != null) { @@ -541,32 +545,32 @@ private void addColumnInternal(OlapTable olapTable, Column newColumn, ColumnPosi for (Map.Entry> entry : indexSchemaMap.entrySet()) { modIndexSchema = entry.getValue(); boolean isBaseIdex = entry.getKey() == baseIndexId; - checkAndAddColumn(modIndexSchema, newColumn, columnPos, isBaseIdex); + checkAndAddColumn(modIndexSchema, newColumn, columnPos, newColNameSet, isBaseIdex); } } else { // 1. add to base table modIndexSchema = indexSchemaMap.get(baseIndexId); - checkAndAddColumn(modIndexSchema, newColumn, columnPos, true); + checkAndAddColumn(modIndexSchema, newColumn, columnPos, newColNameSet, true); if (targetIndexId == -1L) { return; } // 2. add to rollup modIndexSchema = indexSchemaMap.get(targetIndexId); - checkAndAddColumn(modIndexSchema, newColumn, columnPos, false); + checkAndAddColumn(modIndexSchema, newColumn, columnPos, newColNameSet, false); } } else if (KeysType.DUP_KEYS == olapTable.getKeysType()) { if (targetIndexId == -1L) { // check if has default value. this should be done in Analyze phase // 1. add to base index first List modIndexSchema = indexSchemaMap.get(baseIndexId); - checkAndAddColumn(modIndexSchema, newColumn, columnPos, true); + checkAndAddColumn(modIndexSchema, newColumn, columnPos, newColNameSet, true); // no specified target index. return return; } else { // 2. add to rollup index List modIndexSchema = indexSchemaMap.get(targetIndexId); - checkAndAddColumn(modIndexSchema, newColumn, columnPos, false); + checkAndAddColumn(modIndexSchema, newColumn, columnPos, newColNameSet, false); if (newColumn.isKey()) { /* @@ -574,17 +578,17 @@ private void addColumnInternal(OlapTable olapTable, Column newColumn, ColumnPosi * then put the column in base table as end key */ modIndexSchema = indexSchemaMap.get(baseIndexId); - checkAndAddColumn(modIndexSchema, newColumn, null, true); + checkAndAddColumn(modIndexSchema, newColumn, null, newColNameSet, true); } else { modIndexSchema = indexSchemaMap.get(baseIndexId); - checkAndAddColumn(modIndexSchema, newColumn, columnPos, true); + checkAndAddColumn(modIndexSchema, newColumn, columnPos, newColNameSet, true); } } } else { // check if has default value. this should be done in Analyze phase // 1. add to base index first List modIndexSchema = indexSchemaMap.get(baseIndexId); - checkAndAddColumn(modIndexSchema, newColumn, columnPos, true); + checkAndAddColumn(modIndexSchema, newColumn, columnPos, newColNameSet, true); if (targetIndexId == -1L) { // no specified target index. return @@ -593,19 +597,19 @@ private void addColumnInternal(OlapTable olapTable, Column newColumn, ColumnPosi // 2. add to rollup index modIndexSchema = indexSchemaMap.get(targetIndexId); - checkAndAddColumn(modIndexSchema, newColumn, columnPos, false); + checkAndAddColumn(modIndexSchema, newColumn, columnPos, newColNameSet, false); } } private void checkAndAddColumn(List modIndexSchema, Column newColumn, ColumnPosition columnPos, - boolean isBaseIndex) throws DdlException { + Set newColNameSet, boolean isBaseIndex) throws DdlException { int posIndex = -1; String newColName = newColumn.getName(); boolean hasPos = (columnPos != null && !columnPos.isFirst()); for (int i = 0; i < modIndexSchema.size(); i++) { Column col = modIndexSchema.get(i); if (col.getName().equalsIgnoreCase(newColName)) { - if (!isBaseIndex || !col.isFromAddColumnOperation()) { + if (!isBaseIndex || !newColNameSet.contains(newColName)) { // if this is not a base index, we should check if user repeatedly add columns throw new DdlException("Repeatedly add column[" + newColName + "]"); } diff --git a/fe/src/main/java/org/apache/doris/analysis/AddColumnClause.java b/fe/src/main/java/org/apache/doris/analysis/AddColumnClause.java index fa5e4b74489dfe..952a1c475c30e2 100644 --- a/fe/src/main/java/org/apache/doris/analysis/AddColumnClause.java +++ b/fe/src/main/java/org/apache/doris/analysis/AddColumnClause.java @@ -83,7 +83,6 @@ public void analyze(Analyzer analyzer) throws AnalysisException { } column = columnDef.toColumn(); - column.setFromAddColumnOperation(true); } @Override diff --git a/fe/src/main/java/org/apache/doris/analysis/AddColumnsClause.java b/fe/src/main/java/org/apache/doris/analysis/AddColumnsClause.java index 1bd54096ed0ec5..1c9506b70c35c8 100644 --- a/fe/src/main/java/org/apache/doris/analysis/AddColumnsClause.java +++ b/fe/src/main/java/org/apache/doris/analysis/AddColumnsClause.java @@ -70,7 +70,6 @@ public void analyze(Analyzer analyzer) throws AnalysisException { columns = Lists.newArrayList(); for (ColumnDef columnDef : columnDefs) { Column col = columnDef.toColumn(); - col.setFromAddColumnOperation(true); columns.add(col); } } diff --git a/fe/src/main/java/org/apache/doris/catalog/Column.java b/fe/src/main/java/org/apache/doris/catalog/Column.java index e0b97f1c5a272a..6430887e9d7bb9 100644 --- a/fe/src/main/java/org/apache/doris/catalog/Column.java +++ b/fe/src/main/java/org/apache/doris/catalog/Column.java @@ -51,10 +51,6 @@ public class Column implements Writable { private ColumnStats stats; // cardinality and selectivity etc. - // This variable is used to represent if this column instance is generated from ALTER TABLE ADD COLUMN stmt. - // And it does not need to persist - private boolean isFromAddColumnOperation = false; - public Column() { this.name = ""; this.type = Type.NULL; @@ -181,14 +177,6 @@ public String getComment() { return comment; } - public void setFromAddColumnOperation(boolean isFromAddColumnOperation) { - this.isFromAddColumnOperation = isFromAddColumnOperation; - } - - public boolean isFromAddColumnOperation() { - return isFromAddColumnOperation; - } - public int getOlapColumnIndexSize() { PrimitiveType type = this.getDataType(); if (type == PrimitiveType.CHAR) {