From e0114317ab12b807006efd68c448e369582820bb Mon Sep 17 00:00:00 2001 From: amorynan Date: Wed, 8 Jan 2025 19:29:55 +0800 Subject: [PATCH 1/8] support nested type with varchar type to support length growing --- .../java/org/apache/doris/catalog/Type.java | 35 ++++ .../doris/alter/SchemaChangeHandler.java | 5 +- .../java/org/apache/doris/catalog/Column.java | 15 +- .../test_varchar_sc_in_complex.out | 31 ++++ .../test_varchar_sc_in_complex.groovy | 149 ++++++++++++++++++ 5 files changed, 222 insertions(+), 13 deletions(-) create mode 100644 regression-test/data/schema_change_p0/test_varchar_sc_in_complex.out create mode 100644 regression-test/suites/schema_change_p0/test_varchar_sc_in_complex.groovy diff --git a/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java b/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java index 7dfcfd15ebec84..60f855423ff19b 100644 --- a/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java +++ b/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java @@ -511,6 +511,41 @@ public boolean isDecimalV3OrContainsDecimalV3() { return false; } + // This method defines the char type or complex type nested char type + // to support the schema-change behavior of length growth. + public boolean isSupportSchemaChangeForCharType(Type other) { + if ((this.getPrimitiveType() == PrimitiveType.VARCHAR && other.getPrimitiveType() == PrimitiveType.VARCHAR) || ( + this.getPrimitiveType() == PrimitiveType.CHAR && other.getPrimitiveType() == PrimitiveType.VARCHAR) || ( + this.getPrimitiveType() == PrimitiveType.CHAR && other.getPrimitiveType() == PrimitiveType.CHAR)) { + if (this.getLength() > other.getLength()) { + return false; + } else { + return true; + } + } + if (this.isStructType() && other.isStructType()) { + StructType thisStructType = (StructType) this; + StructType otherStructType = (StructType) other; + if (thisStructType.getFields().size() != otherStructType.getFields().size()) { + return false; + } + for (int i = 0; i < thisStructType.getFields().size(); i++) { + if (!thisStructType.getFields().get(i).getType().isSupportSchemaChangeForCharType( + otherStructType.getFields().get(i).getType())) { + return false; + } + } + return true; + } else if (this.isArrayType() && other.isArrayType()) { + return ((ArrayType) this).getItemType().isSupportSchemaChangeForCharType(((ArrayType) other).getItemType()); + } else if (this.isMapType() && other.isMapType()) { + return ((MapType) this).getKeyType().isSupportSchemaChangeForCharType(((MapType) other).getKeyType()) + && + ((MapType) this).getValueType().isSupportSchemaChangeForCharType(((MapType) other).getValueType()); + } + return false; + } + public boolean isDecimalV3() { return isScalarType(PrimitiveType.DECIMAL32) || isScalarType(PrimitiveType.DECIMAL64) || isScalarType(PrimitiveType.DECIMAL128) || isScalarType(PrimitiveType.DECIMAL256); diff --git a/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java b/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java index 4c853951144193..ddfe70a567e202 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java +++ b/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java @@ -627,8 +627,9 @@ private boolean processModifyColumn(ModifyColumnClause alterClause, OlapTable ol if (!col.equals(modColumn)) { typeChanged = true; // TODO:the case where columnPos is not empty has not been considered - if (columnPos == null && col.getDataType() == PrimitiveType.VARCHAR - && modColumn.getDataType() == PrimitiveType.VARCHAR) { + if (columnPos == null && ((col.getDataType() == PrimitiveType.VARCHAR + && modColumn.getDataType() == PrimitiveType.VARCHAR) + || col.getDataType().isComplexType())) { col.checkSchemaChangeAllowed(modColumn); lightSchemaChange = olapTable.getEnableLightSchemaChange(); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java index 3ef5f680e94d15..e0880b2e149a5e 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java @@ -844,11 +844,6 @@ public void checkSchemaChangeAllowed(Column other) throws DdlException { throw new DdlException("Dest column name is empty"); } - // now nested type can only support change order - if (type.isComplexType() && !type.equals(other.type)) { - throw new DdlException("Can not change " + type + " to " + other); - } - if (!ColumnType.isSchemaChangeAllowed(type, other.type)) { throw new DdlException("Can not change " + getDataType() + " to " + other.getDataType()); } @@ -885,12 +880,10 @@ public void checkSchemaChangeAllowed(Column other) throws DdlException { } } - if ((getDataType() == PrimitiveType.VARCHAR && other.getDataType() == PrimitiveType.VARCHAR) || ( - getDataType() == PrimitiveType.CHAR && other.getDataType() == PrimitiveType.VARCHAR) || ( - getDataType() == PrimitiveType.CHAR && other.getDataType() == PrimitiveType.CHAR)) { - if (getStrLen() > other.getStrLen()) { - throw new DdlException("Cannot shorten string length"); - } + // Nested types only support changing the order and increasing the length of the nested char type + // Char-type only support length growing + if (!type.isSupportSchemaChangeForCharType(other.type)) { + throw new DdlException("Cannot shorten string length for type " + type.toSql() + " to " + other.toSql()); } // now we support convert decimal to varchar type diff --git a/regression-test/data/schema_change_p0/test_varchar_sc_in_complex.out b/regression-test/data/schema_change_p0/test_varchar_sc_in_complex.out new file mode 100644 index 00000000000000..145f3c94d8a9ca --- /dev/null +++ b/regression-test/data/schema_change_p0/test_varchar_sc_in_complex.out @@ -0,0 +1,31 @@ +-- This file is automatically generated. You should know what you did if you want to edit this +-- !sc_origin -- +0 ["2025-01-01"] {"amory":"better"} {"col":"commiter"} +1 ["2025-01-02"] {"doris":"better"} {"col":"amory"} + +-- !sc_after -- +0 ["2025-01-01"] {"amory":"better"} {"col":"commiter"} +1 ["2025-01-02"] {"doris":"better"} {"col":"amory"} +2 ["2025-01-03-22-33"] {"doris":"better"} {"col":"amory"} + +-- !sc_origin -- +0 ["2025-01-01"] {"amory":"better"} {"col":"commiter"} +1 ["2025-01-02"] {"doris":"better"} {"col":"amory"} +2 ["2025-01-03-22-33"] {"doris":"better"} {"col":"amory"} + +-- !sc_after -- +0 ["2025-01-01"] {"amory":"better"} {"col":"commiter"} +1 ["2025-01-02"] {"doris":"better"} {"col":"amory"} +2 ["2025-01-03-22-33"] {"doris":"better"} {"col":"amory"} + +-- !sc_origin -- +0 ["2025-01-01"] {"amory":"better"} {"col":"commiter"} +1 ["2025-01-02"] {"doris":"better"} {"col":"amory"} +2 ["2025-01-03-22-33"] {"doris":"better"} {"col":"amory"} + +-- !sc_after -- +0 ["2025-01-01"] {"amory":"better"} {"col":"commiter"} +1 ["2025-01-02"] {"doris":"better"} {"col":"amory"} +2 ["2025-01-03-22-33"] {"doris":"better"} {"col":"amory"} +4 ["2025-01-03-22-33"] {"doris":"better"} {"col":"amoryIsBetter"} + diff --git a/regression-test/suites/schema_change_p0/test_varchar_sc_in_complex.groovy b/regression-test/suites/schema_change_p0/test_varchar_sc_in_complex.groovy new file mode 100644 index 00000000000000..8c65509b91c848 --- /dev/null +++ b/regression-test/suites/schema_change_p0/test_varchar_sc_in_complex.groovy @@ -0,0 +1,149 @@ +// 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. + +import org.codehaus.groovy.runtime.IOGroovyMethods +import java.util.concurrent.TimeUnit +import org.awaitility.Awaitility + +suite ("test_varchar_sc_in_complex") { + def getJobState = { tableName -> + def jobStateResult = sql """ SHOW ALTER TABLE COLUMN WHERE IndexName='${tableName}' ORDER BY createtime DESC LIMIT 1 """ + return jobStateResult[0][9] + } + + def tableName = "test_varchar_sc_in_complex" + + try { + + String backend_id; + def backendId_to_backendIP = [:] + def backendId_to_backendHttpPort = [:] + getBackendIpHttpPort(backendId_to_backendIP, backendId_to_backendHttpPort); + + sql """ DROP TABLE IF EXISTS ${tableName} """ + sql """ + CREATE TABLE IF NOT EXISTS ${tableName} ( + `c0` LARGEINT NOT NULL, + `c_a` ARRAY, + `c_m` MAP, + `c_s` STRUCT + ) DISTRIBUTED BY HASH(c0) BUCKETS 1 + PROPERTIES ( "replication_num" = "1", "light_schema_change" = "true" ) + """ + + sql """ insert into ${tableName} values + (0,['2025-01-01'], {'amory':'better'}, named_struct('col','commiter')); + """ + sql """ insert into ${tableName} values + (1,['2025-01-02'], {'doris':'better'}, named_struct('col','amory')); + """ + // this can be insert but with cut off the left string to 10 + test { + sql """ insert into ${tableName} values + (11, ['2025-01-03-22-33'], {'doris111111111':'better2222222222'}, named_struct('col','amoryIsBetter')); + """ + exception "Insert has filtered data in strict mode" + } + + test { + sql """ alter table ${tableName} modify column c_a array + """ + exception "Cannot shorten string length" + } + + test { + sql """ alter table ${tableName} modify column c_m map + """ + exception "Cannot shorten string length" + } + + test { + sql """ alter table ${tableName} modify column c_s struct + """ + exception "Cannot shorten string length" + } + + + sql """ alter table ${tableName} modify column c_a array """ + int max_try_secs = 300 + Awaitility.await().atMost(max_try_secs, TimeUnit.SECONDS).with().pollDelay(100, TimeUnit.MILLISECONDS).await().until(() -> { + String result = getJobState(tableName) + if (result == "FINISHED") { + return true; + } + return false; + }); + + String[][] res = sql """ desc ${tableName} """ + logger.info(res[1][1]) + // array varchar(10) + assertEquals(res[1][1].toLowerCase(),"array") + + qt_sc_origin " select * from ${tableName} order by c0; " + + // then insert some data to modified array with varchar(20) + sql """ insert into ${tableName} values + (2,['2025-01-03-22-33'], {'doris':'better'}, named_struct('col','amory')); + """ + qt_sc_after " select * from ${tableName} order by c0; " + + // map varchar(10) + sql """ alter table ${tableName} modify column c_m map """ + Awaitility.await().atMost(max_try_secs, TimeUnit.SECONDS).with().pollDelay(100, TimeUnit.MILLISECONDS).await().until(() -> { + String result = getJobState(tableName) + if (result == "FINISHED") { + return true; + } + return false; + }); + + res = sql """ desc ${tableName} """ + logger.info(res[2][1]) + assertEquals(res[2][1].toLowerCase(),"map") + + // insert some data to modified map with varchar(20) + qt_sc_origin " select * from ${tableName} order by c0; " + sql """ insert into ts values + (3,['2025-01-03-22-33'], {'doris111111111':'better2222222222'}, named_struct('col','amory')); + """ + qt_sc_after " select * from ${tableName} order by c0; " + + // struct varchar(10) + sql """ alter table ${tableName} modify column c_s struct """ + Awaitility.await().atMost(max_try_secs, TimeUnit.SECONDS).with().pollDelay(100, TimeUnit.MILLISECONDS).await().until(() -> { + String result = getJobState(tableName) + if (result == "FINISHED") { + return true; + } + return false; + }); + + res = sql """ desc ${tableName} """ + logger.info(res[3][1]) + assertEquals(res[3][1].toLowerCase(),"struct") + + // insert some data to modified struct with varchar(20) + qt_sc_origin " select * from ${tableName} order by c0; " + sql """ insert into ${tableName} values + (4,['2025-01-03-22-33'], {'doris':'better'}, named_struct('col','amoryIsBetter')); + """ + qt_sc_after " select * from ${tableName} order by c0; " + } finally { + try_sql("DROP TABLE IF EXISTS ${tableName}") + } + +} From ac1d2f052f58168e0fe4a3d855fbac4e2576eb9c Mon Sep 17 00:00:00 2001 From: amorynan Date: Thu, 9 Jan 2025 17:28:57 +0800 Subject: [PATCH 2/8] fix code --- .../java/org/apache/doris/catalog/Type.java | 13 +++---- .../java/org/apache/doris/catalog/Column.java | 8 +++- .../test_varchar_sc_in_complex.groovy | 38 +++++++++++++++++++ 3 files changed, 50 insertions(+), 9 deletions(-) diff --git a/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java b/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java index 60f855423ff19b..a8df1d005fbe70 100644 --- a/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java +++ b/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java @@ -513,29 +513,28 @@ public boolean isDecimalV3OrContainsDecimalV3() { // This method defines the char type or complex type nested char type // to support the schema-change behavior of length growth. - public boolean isSupportSchemaChangeForCharType(Type other) { + public boolean isSupportSchemaChangeForCharType(Type other) throws TypeException { if ((this.getPrimitiveType() == PrimitiveType.VARCHAR && other.getPrimitiveType() == PrimitiveType.VARCHAR) || ( this.getPrimitiveType() == PrimitiveType.CHAR && other.getPrimitiveType() == PrimitiveType.VARCHAR) || ( this.getPrimitiveType() == PrimitiveType.CHAR && other.getPrimitiveType() == PrimitiveType.CHAR)) { if (this.getLength() > other.getLength()) { - return false; + throw new TypeException("Cannot shorten string length for type " + + this.toSql() + " to " + other.toSql()); } else { return true; } - } - if (this.isStructType() && other.isStructType()) { + } else if (this.isStructType() && other.isStructType()) { StructType thisStructType = (StructType) this; StructType otherStructType = (StructType) other; if (thisStructType.getFields().size() != otherStructType.getFields().size()) { - return false; + throw new TypeException("Cannot change struct type with different field size"); } for (int i = 0; i < thisStructType.getFields().size(); i++) { if (!thisStructType.getFields().get(i).getType().isSupportSchemaChangeForCharType( otherStructType.getFields().get(i).getType())) { - return false; + throw new TypeException("Cannot change struct type with different field type"); } } - return true; } else if (this.isArrayType() && other.isArrayType()) { return ((ArrayType) this).getItemType().isSupportSchemaChangeForCharType(((ArrayType) other).getItemType()); } else if (this.isMapType() && other.isMapType()) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java index e0880b2e149a5e..f5b9eda29bc378 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java @@ -882,8 +882,12 @@ public void checkSchemaChangeAllowed(Column other) throws DdlException { // Nested types only support changing the order and increasing the length of the nested char type // Char-type only support length growing - if (!type.isSupportSchemaChangeForCharType(other.type)) { - throw new DdlException("Cannot shorten string length for type " + type.toSql() + " to " + other.toSql()); + try { + if (!type.isSupportSchemaChangeForCharType(other.type)) { + throw new DdlException("Can not change " + type.toSql() + " to " + other.type.toSql()); + } + } catch (TypeException e) { + throw new DdlException(e.getMessage()); } // now we support convert decimal to varchar type diff --git a/regression-test/suites/schema_change_p0/test_varchar_sc_in_complex.groovy b/regression-test/suites/schema_change_p0/test_varchar_sc_in_complex.groovy index 8c65509b91c848..1ce39b9867b5fc 100644 --- a/regression-test/suites/schema_change_p0/test_varchar_sc_in_complex.groovy +++ b/regression-test/suites/schema_change_p0/test_varchar_sc_in_complex.groovy @@ -77,6 +77,44 @@ suite ("test_varchar_sc_in_complex") { exception "Cannot shorten string length" } + // add case alter modify array/map/struct to other type + // test array to struct + test { + sql """ alter table ${tableName} modify column c_a struct + """ + exception "Cannot chang" + } + // test array to map + test { + sql """ alter table ${tableName} modify column c_a map + """ + exception "Cannot chang" + } + // test map to array + test { + sql """ alter table ${tableName} modify column c_m array + """ + exception "Cannot chang" + } + // test map to struct + test { + sql """ alter table ${tableName} modify column c_m struct + """ + exception "Cannot chang" + } + // test struct to array + test { + sql """ alter table ${tableName} modify column c_s array + """ + exception "Cannot chang" + } + // test struct to map + test { + sql """ alter table ${tableName} modify column c_s map + """ + exception "Cannot chang" + } + sql """ alter table ${tableName} modify column c_a array """ int max_try_secs = 300 From 6295442e7a6fde91445fd6d3a5b9c464712751dd Mon Sep 17 00:00:00 2001 From: amorynan Date: Thu, 9 Jan 2025 22:49:29 +0800 Subject: [PATCH 3/8] update fe code --- .../java/org/apache/doris/catalog/Type.java | 26 ++++++++++++------- .../doris/alter/SchemaChangeHandler.java | 25 ++++++++++++++---- .../java/org/apache/doris/catalog/Column.java | 2 +- 3 files changed, 38 insertions(+), 15 deletions(-) diff --git a/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java b/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java index cd0a3e52806ee1..dbc5a9059e8064 100644 --- a/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java +++ b/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java @@ -523,38 +523,46 @@ public boolean isDecimalV3OrContainsDecimalV3() { return false; } - // This method defines the char type or complex type nested char type + // This method defines the char type // to support the schema-change behavior of length growth. public boolean isSupportSchemaChangeForCharType(Type other) throws TypeException { if ((this.getPrimitiveType() == PrimitiveType.VARCHAR && other.getPrimitiveType() == PrimitiveType.VARCHAR) || ( this.getPrimitiveType() == PrimitiveType.CHAR && other.getPrimitiveType() == PrimitiveType.VARCHAR) || ( this.getPrimitiveType() == PrimitiveType.CHAR && other.getPrimitiveType() == PrimitiveType.CHAR)) { if (this.getLength() > other.getLength()) { - throw new TypeException("Cannot shorten string length for type " - + this.toSql() + " to " + other.toSql()); + throw new TypeException("Cannot shorten string length"); } else { return true; } - } else if (this.isStructType() && other.isStructType()) { + } + return false; + } + + // This method defines the complex type which is struct, array, map if nested char-type + // to support the schema-change behavior of length growth. + public boolean isSupportSchemaChangeForComplexType(Type other) throws TypeException { + if (this.isStructType() && other.isStructType()) { StructType thisStructType = (StructType) this; StructType otherStructType = (StructType) other; if (thisStructType.getFields().size() != otherStructType.getFields().size()) { throw new TypeException("Cannot change struct type with different field size"); } for (int i = 0; i < thisStructType.getFields().size(); i++) { - if (!thisStructType.getFields().get(i).getType().isSupportSchemaChangeForCharType( + if (!thisStructType.getFields().get(i).getType().isSupportSchemaChangeForComplexType( otherStructType.getFields().get(i).getType())) { throw new TypeException("Cannot change struct type with different field type"); } } } else if (this.isArrayType() && other.isArrayType()) { - return ((ArrayType) this).getItemType().isSupportSchemaChangeForCharType(((ArrayType) other).getItemType()); + return ((ArrayType) this).getItemType().isSupportSchemaChangeForComplexType( + ((ArrayType) other).getItemType()); } else if (this.isMapType() && other.isMapType()) { - return ((MapType) this).getKeyType().isSupportSchemaChangeForCharType(((MapType) other).getKeyType()) + return ((MapType) this).getKeyType().isSupportSchemaChangeForComplexType(((MapType) other).getKeyType()) && - ((MapType) this).getValueType().isSupportSchemaChangeForCharType(((MapType) other).getValueType()); + ((MapType) this).getValueType().isSupportSchemaChangeForComplexType( + ((MapType) other).getValueType()); } - return false; + return isSupportSchemaChangeForCharType(other); } public boolean isDecimalV3() { diff --git a/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java b/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java index bea7a691089d76..1043fb34fecd44 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java +++ b/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java @@ -67,6 +67,7 @@ import org.apache.doris.catalog.TableIf.TableType; import org.apache.doris.catalog.Tablet; import org.apache.doris.catalog.TabletMeta; +import org.apache.doris.catalog.TypeException; import org.apache.doris.common.AnalysisException; import org.apache.doris.common.Config; import org.apache.doris.common.DdlException; @@ -627,11 +628,25 @@ private boolean processModifyColumn(ModifyColumnClause alterClause, OlapTable ol if (!col.equals(modColumn)) { typeChanged = true; // TODO:the case where columnPos is not empty has not been considered - if (columnPos == null && ((col.getDataType() == PrimitiveType.VARCHAR - && modColumn.getDataType() == PrimitiveType.VARCHAR) - || col.getDataType().isComplexType())) { - col.checkSchemaChangeAllowed(modColumn); - lightSchemaChange = olapTable.getEnableLightSchemaChange(); + if (columnPos == null && col.getDataType() == PrimitiveType.VARCHAR + && modColumn.getDataType() == PrimitiveType.VARCHAR) { + try { + if (col.getType().isSupportSchemaChangeForCharType(modColumn.getType())) { + lightSchemaChange = olapTable.getEnableLightSchemaChange(); + } + } catch (TypeException e) { + throw new DdlException(e.getMessage()); + } + } + if (columnPos == null && col.getDataType().isComplexType() + && modColumn.getDataType().isComplexType()) { + try { + if (col.getType().isSupportSchemaChangeForComplexType(modColumn.getType())) { + lightSchemaChange = olapTable.getEnableLightSchemaChange(); + } + } catch (TypeException e) { + throw new DdlException(e.getMessage()); + } } if (col.isClusterKey()) { throw new DdlException("Can not modify cluster key column: " + col.getName()); diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java index 4d7968d47b832e..06ffb9370f2e04 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java @@ -886,7 +886,7 @@ public void checkSchemaChangeAllowed(Column other) throws DdlException { // Nested types only support changing the order and increasing the length of the nested char type // Char-type only support length growing try { - if (!type.isSupportSchemaChangeForCharType(other.type)) { + if (!type.isSupportSchemaChangeForComplexType(other.type)) { throw new DdlException("Can not change " + type.toSql() + " to " + other.type.toSql()); } } catch (TypeException e) { From 6bf5fc1769cd78fb03c74bc5ca0aef80b75f2b07 Mon Sep 17 00:00:00 2001 From: amorynan Date: Fri, 10 Jan 2025 11:35:39 +0800 Subject: [PATCH 4/8] fix code --- .../java/org/apache/doris/catalog/Type.java | 42 ------------------ .../doris/alter/SchemaChangeHandler.java | 20 +++------ .../java/org/apache/doris/catalog/Column.java | 8 +--- .../org/apache/doris/catalog/ColumnType.java | 43 +++++++++++++++++++ .../test_varchar_sc_in_complex.out | 3 ++ .../test_varchar_sc_in_complex.groovy | 14 +++--- 6 files changed, 59 insertions(+), 71 deletions(-) diff --git a/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java b/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java index dbc5a9059e8064..d3218203bf6462 100644 --- a/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java +++ b/fe/fe-common/src/main/java/org/apache/doris/catalog/Type.java @@ -523,48 +523,6 @@ public boolean isDecimalV3OrContainsDecimalV3() { return false; } - // This method defines the char type - // to support the schema-change behavior of length growth. - public boolean isSupportSchemaChangeForCharType(Type other) throws TypeException { - if ((this.getPrimitiveType() == PrimitiveType.VARCHAR && other.getPrimitiveType() == PrimitiveType.VARCHAR) || ( - this.getPrimitiveType() == PrimitiveType.CHAR && other.getPrimitiveType() == PrimitiveType.VARCHAR) || ( - this.getPrimitiveType() == PrimitiveType.CHAR && other.getPrimitiveType() == PrimitiveType.CHAR)) { - if (this.getLength() > other.getLength()) { - throw new TypeException("Cannot shorten string length"); - } else { - return true; - } - } - return false; - } - - // This method defines the complex type which is struct, array, map if nested char-type - // to support the schema-change behavior of length growth. - public boolean isSupportSchemaChangeForComplexType(Type other) throws TypeException { - if (this.isStructType() && other.isStructType()) { - StructType thisStructType = (StructType) this; - StructType otherStructType = (StructType) other; - if (thisStructType.getFields().size() != otherStructType.getFields().size()) { - throw new TypeException("Cannot change struct type with different field size"); - } - for (int i = 0; i < thisStructType.getFields().size(); i++) { - if (!thisStructType.getFields().get(i).getType().isSupportSchemaChangeForComplexType( - otherStructType.getFields().get(i).getType())) { - throw new TypeException("Cannot change struct type with different field type"); - } - } - } else if (this.isArrayType() && other.isArrayType()) { - return ((ArrayType) this).getItemType().isSupportSchemaChangeForComplexType( - ((ArrayType) other).getItemType()); - } else if (this.isMapType() && other.isMapType()) { - return ((MapType) this).getKeyType().isSupportSchemaChangeForComplexType(((MapType) other).getKeyType()) - && - ((MapType) this).getValueType().isSupportSchemaChangeForComplexType( - ((MapType) other).getValueType()); - } - return isSupportSchemaChangeForCharType(other); - } - public boolean isDecimalV3() { return isScalarType(PrimitiveType.DECIMAL32) || isScalarType(PrimitiveType.DECIMAL64) || isScalarType(PrimitiveType.DECIMAL128) || isScalarType(PrimitiveType.DECIMAL256); diff --git a/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java b/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java index 1043fb34fecd44..f615a5c59dd96a 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java +++ b/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java @@ -38,6 +38,7 @@ import org.apache.doris.catalog.AggregateType; import org.apache.doris.catalog.BinlogConfig; import org.apache.doris.catalog.Column; +import org.apache.doris.catalog.ColumnType; import org.apache.doris.catalog.Database; import org.apache.doris.catalog.DistributionInfo; import org.apache.doris.catalog.DistributionInfo.DistributionInfoType; @@ -67,7 +68,6 @@ import org.apache.doris.catalog.TableIf.TableType; import org.apache.doris.catalog.Tablet; import org.apache.doris.catalog.TabletMeta; -import org.apache.doris.catalog.TypeException; import org.apache.doris.common.AnalysisException; import org.apache.doris.common.Config; import org.apache.doris.common.DdlException; @@ -630,23 +630,13 @@ private boolean processModifyColumn(ModifyColumnClause alterClause, OlapTable ol // TODO:the case where columnPos is not empty has not been considered if (columnPos == null && col.getDataType() == PrimitiveType.VARCHAR && modColumn.getDataType() == PrimitiveType.VARCHAR) { - try { - if (col.getType().isSupportSchemaChangeForCharType(modColumn.getType())) { - lightSchemaChange = olapTable.getEnableLightSchemaChange(); - } - } catch (TypeException e) { - throw new DdlException(e.getMessage()); - } + ColumnType.checkSupportSchemaChangeForCharType(col.getType(), modColumn.getType()); + lightSchemaChange = olapTable.getEnableLightSchemaChange(); } if (columnPos == null && col.getDataType().isComplexType() && modColumn.getDataType().isComplexType()) { - try { - if (col.getType().isSupportSchemaChangeForComplexType(modColumn.getType())) { - lightSchemaChange = olapTable.getEnableLightSchemaChange(); - } - } catch (TypeException e) { - throw new DdlException(e.getMessage()); - } + ColumnType.checkSupportSchemaChangeForComplexType(col.getType(), modColumn.getType()); + lightSchemaChange = olapTable.getEnableLightSchemaChange(); } if (col.isClusterKey()) { throw new DdlException("Can not modify cluster key column: " + col.getName()); diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java index 06ffb9370f2e04..22df67a5c642fe 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java @@ -885,13 +885,7 @@ public void checkSchemaChangeAllowed(Column other) throws DdlException { // Nested types only support changing the order and increasing the length of the nested char type // Char-type only support length growing - try { - if (!type.isSupportSchemaChangeForComplexType(other.type)) { - throw new DdlException("Can not change " + type.toSql() + " to " + other.type.toSql()); - } - } catch (TypeException e) { - throw new DdlException(e.getMessage()); - } + ColumnType.checkSupportSchemaChangeForComplexType(type, other.type); // now we support convert decimal to varchar type if ((getDataType() == PrimitiveType.DECIMALV2 || getDataType().isDecimalV3Type()) diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java index 458a6a46110f40..ad7a618e8d8438 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java @@ -17,6 +17,7 @@ package org.apache.doris.catalog; +import org.apache.doris.common.DdlException; import org.apache.doris.common.FeMetaVersion; import org.apache.doris.common.io.Text; import org.apache.doris.persist.gson.GsonUtils; @@ -168,6 +169,48 @@ static boolean isSchemaChangeAllowed(Type lhs, Type rhs) { return schemaChangeMatrix[lhs.getPrimitiveType().ordinal()][rhs.getPrimitiveType().ordinal()]; } + // This method defines the char type + // to support the schema-change behavior of length growth. + public static void checkSupportSchemaChangeForCharType(Type checkType, Type other) throws DdlException { + if ((checkType.getPrimitiveType() == PrimitiveType.VARCHAR && other.getPrimitiveType() == PrimitiveType.VARCHAR) + || (checkType.getPrimitiveType() == PrimitiveType.CHAR + && other.getPrimitiveType() == PrimitiveType.VARCHAR) + || (checkType.getPrimitiveType() == PrimitiveType.CHAR + && other.getPrimitiveType() == PrimitiveType.CHAR)) { + if (checkType.getLength() > other.getLength()) { + throw new DdlException("Cannot shorten string length"); + } + } else { + throw new DdlException("Cannot change " + checkType.toSql() + " to " + other.toSql()); + } + } + + // This method defines the complex type which is struct, array, map if nested char-type + // to support the schema-change behavior of length growth. + public static void checkSupportSchemaChangeForComplexType(Type checkType, Type other) throws DdlException { + if (checkType.isStructType() && other.isStructType()) { + StructType thisStructType = (StructType) checkType; + StructType otherStructType = (StructType) other; + if (thisStructType.getFields().size() != otherStructType.getFields().size()) { + throw new DdlException("Cannot change struct type with different field size"); + } + for (int i = 0; i < thisStructType.getFields().size(); i++) { + checkSupportSchemaChangeForComplexType(thisStructType.getFields().get(i).getType(), + otherStructType.getFields().get(i).getType()); + } + } else if (checkType.isArrayType() && other.isArrayType()) { + checkSupportSchemaChangeForComplexType(((ArrayType) checkType).getItemType(), + ((ArrayType) other).getItemType()); + } else if (checkType.isMapType() && other.isMapType()) { + checkSupportSchemaChangeForComplexType(((MapType) checkType).getKeyType(), + ((MapType) other).getKeyType()); + checkSupportSchemaChangeForComplexType(((MapType) checkType).getValueType(), + ((MapType) other).getValueType()); + } else { + checkSupportSchemaChangeForCharType(checkType, other); + } + } + public static void write(DataOutput out, Type type) throws IOException { Preconditions.checkArgument(type.isScalarType() || type.isAggStateType() || type.isArrayType() || type.isMapType() || type.isStructType(), diff --git a/regression-test/data/schema_change_p0/test_varchar_sc_in_complex.out b/regression-test/data/schema_change_p0/test_varchar_sc_in_complex.out index 145f3c94d8a9ca..c852a4008fe750 100644 --- a/regression-test/data/schema_change_p0/test_varchar_sc_in_complex.out +++ b/regression-test/data/schema_change_p0/test_varchar_sc_in_complex.out @@ -17,15 +17,18 @@ 0 ["2025-01-01"] {"amory":"better"} {"col":"commiter"} 1 ["2025-01-02"] {"doris":"better"} {"col":"amory"} 2 ["2025-01-03-22-33"] {"doris":"better"} {"col":"amory"} +3 ["2025-01-03-22-33"] {"doris111111111":"better2222222222"} {"col":"amory"} -- !sc_origin -- 0 ["2025-01-01"] {"amory":"better"} {"col":"commiter"} 1 ["2025-01-02"] {"doris":"better"} {"col":"amory"} 2 ["2025-01-03-22-33"] {"doris":"better"} {"col":"amory"} +3 ["2025-01-03-22-33"] {"doris111111111":"better2222222222"} {"col":"amory"} -- !sc_after -- 0 ["2025-01-01"] {"amory":"better"} {"col":"commiter"} 1 ["2025-01-02"] {"doris":"better"} {"col":"amory"} 2 ["2025-01-03-22-33"] {"doris":"better"} {"col":"amory"} +3 ["2025-01-03-22-33"] {"doris111111111":"better2222222222"} {"col":"amory"} 4 ["2025-01-03-22-33"] {"doris":"better"} {"col":"amoryIsBetter"} diff --git a/regression-test/suites/schema_change_p0/test_varchar_sc_in_complex.groovy b/regression-test/suites/schema_change_p0/test_varchar_sc_in_complex.groovy index 1ce39b9867b5fc..d3d2554025f2ff 100644 --- a/regression-test/suites/schema_change_p0/test_varchar_sc_in_complex.groovy +++ b/regression-test/suites/schema_change_p0/test_varchar_sc_in_complex.groovy @@ -82,37 +82,37 @@ suite ("test_varchar_sc_in_complex") { test { sql """ alter table ${tableName} modify column c_a struct """ - exception "Cannot chang" + exception "Can not change ARRAY to STRUCT" } // test array to map test { sql """ alter table ${tableName} modify column c_a map """ - exception "Cannot chang" + exception "Can not change ARRAY to MAP" } // test map to array test { sql """ alter table ${tableName} modify column c_m array """ - exception "Cannot chang" + exception "Can not change MAP to ARRAY" } // test map to struct test { sql """ alter table ${tableName} modify column c_m struct """ - exception "Cannot chang" + exception "Can not change MAP to STRUCT" } // test struct to array test { sql """ alter table ${tableName} modify column c_s array """ - exception "Cannot chang" + exception "Can not change STRUCT to ARRAY" } // test struct to map test { sql """ alter table ${tableName} modify column c_s map """ - exception "Cannot chang" + exception "Can not change STRUCT to MAP" } @@ -155,7 +155,7 @@ suite ("test_varchar_sc_in_complex") { // insert some data to modified map with varchar(20) qt_sc_origin " select * from ${tableName} order by c0; " - sql """ insert into ts values + sql """ insert into ${tableName} values (3,['2025-01-03-22-33'], {'doris111111111':'better2222222222'}, named_struct('col','amory')); """ qt_sc_after " select * from ${tableName} order by c0; " From 8d480bf8dc137d9b7a5ad041ebb1ff4e12497038 Mon Sep 17 00:00:00 2001 From: amorynan Date: Mon, 13 Jan 2025 18:45:47 +0800 Subject: [PATCH 5/8] fix feut --- .../org/apache/doris/catalog/ColumnType.java | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java index ad7a618e8d8438..64f06891be4516 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java @@ -180,15 +180,16 @@ public static void checkSupportSchemaChangeForCharType(Type checkType, Type othe if (checkType.getLength() > other.getLength()) { throw new DdlException("Cannot shorten string length"); } - } else { - throw new DdlException("Cannot change " + checkType.toSql() + " to " + other.toSql()); } } // This method defines the complex type which is struct, array, map if nested char-type // to support the schema-change behavior of length growth. public static void checkSupportSchemaChangeForComplexType(Type checkType, Type other) throws DdlException { - if (checkType.isStructType() && other.isStructType()) { + if (checkType.isStructType()) { + if (!other.isStructType()) { + throw new DdlException("Cannot change " + checkType.toSql() + " to " + other.toSql()); + } StructType thisStructType = (StructType) checkType; StructType otherStructType = (StructType) other; if (thisStructType.getFields().size() != otherStructType.getFields().size()) { @@ -198,10 +199,16 @@ public static void checkSupportSchemaChangeForComplexType(Type checkType, Type o checkSupportSchemaChangeForComplexType(thisStructType.getFields().get(i).getType(), otherStructType.getFields().get(i).getType()); } - } else if (checkType.isArrayType() && other.isArrayType()) { + } else if (checkType.isArrayType()) { + if (!other.isArrayType()) { + throw new DdlException("Cannot change " + checkType.toSql() + " to " + other.toSql()); + } checkSupportSchemaChangeForComplexType(((ArrayType) checkType).getItemType(), ((ArrayType) other).getItemType()); - } else if (checkType.isMapType() && other.isMapType()) { + } else if (checkType.isMapType()) { + if (!other.isMapType()) { + throw new DdlException("Cannot change " + checkType.toSql() + " to " + other.toSql()); + } checkSupportSchemaChangeForComplexType(((MapType) checkType).getKeyType(), ((MapType) other).getKeyType()); checkSupportSchemaChangeForComplexType(((MapType) checkType).getValueType(), From 6be45648cd585277631280a7b4096146ef96c124 Mon Sep 17 00:00:00 2001 From: amorynan Date: Mon, 13 Jan 2025 23:09:27 +0800 Subject: [PATCH 6/8] add return value for function --- .../doris/alter/SchemaChangeHandler.java | 2 +- .../java/org/apache/doris/catalog/Column.java | 2 +- .../org/apache/doris/catalog/ColumnType.java | 34 +++++++++++-------- 3 files changed, 21 insertions(+), 17 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java b/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java index f615a5c59dd96a..ced75bf388b1c8 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java +++ b/fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java @@ -635,7 +635,7 @@ private boolean processModifyColumn(ModifyColumnClause alterClause, OlapTable ol } if (columnPos == null && col.getDataType().isComplexType() && modColumn.getDataType().isComplexType()) { - ColumnType.checkSupportSchemaChangeForComplexType(col.getType(), modColumn.getType()); + ColumnType.checkSupportSchemaChangeForComplexType(col.getType(), modColumn.getType(), true); lightSchemaChange = olapTable.getEnableLightSchemaChange(); } if (col.isClusterKey()) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java index 22df67a5c642fe..fa7c7feb7f0670 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java @@ -885,7 +885,7 @@ public void checkSchemaChangeAllowed(Column other) throws DdlException { // Nested types only support changing the order and increasing the length of the nested char type // Char-type only support length growing - ColumnType.checkSupportSchemaChangeForComplexType(type, other.type); + ColumnType.checkSupportSchemaChangeForComplexType(type, other.type, false); // now we support convert decimal to varchar type if ((getDataType() == PrimitiveType.DECIMALV2 || getDataType().isDecimalV3Type()) diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java index 64f06891be4516..329f1941be24ab 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java @@ -171,7 +171,9 @@ static boolean isSchemaChangeAllowed(Type lhs, Type rhs) { // This method defines the char type // to support the schema-change behavior of length growth. - public static void checkSupportSchemaChangeForCharType(Type checkType, Type other) throws DdlException { + // return true if the checkType and other are both char-type otherwise return false, + // which used in checkSupportSchemaChangeForComplexType + public static boolean checkSupportSchemaChangeForCharType(Type checkType, Type other) throws DdlException { if ((checkType.getPrimitiveType() == PrimitiveType.VARCHAR && other.getPrimitiveType() == PrimitiveType.VARCHAR) || (checkType.getPrimitiveType() == PrimitiveType.CHAR && other.getPrimitiveType() == PrimitiveType.VARCHAR) @@ -180,16 +182,17 @@ public static void checkSupportSchemaChangeForCharType(Type checkType, Type othe if (checkType.getLength() > other.getLength()) { throw new DdlException("Cannot shorten string length"); } + return true; + } else { + return false; } } // This method defines the complex type which is struct, array, map if nested char-type // to support the schema-change behavior of length growth. - public static void checkSupportSchemaChangeForComplexType(Type checkType, Type other) throws DdlException { - if (checkType.isStructType()) { - if (!other.isStructType()) { - throw new DdlException("Cannot change " + checkType.toSql() + " to " + other.toSql()); - } + public static void checkSupportSchemaChangeForComplexType(Type checkType, Type other, boolean nested) + throws DdlException { + if (checkType.isStructType() && other.isStructType()) { StructType thisStructType = (StructType) checkType; StructType otherStructType = (StructType) other; if (thisStructType.getFields().size() != otherStructType.getFields().size()) { @@ -197,24 +200,25 @@ public static void checkSupportSchemaChangeForComplexType(Type checkType, Type o } for (int i = 0; i < thisStructType.getFields().size(); i++) { checkSupportSchemaChangeForComplexType(thisStructType.getFields().get(i).getType(), - otherStructType.getFields().get(i).getType()); + otherStructType.getFields().get(i).getType(), true); } } else if (checkType.isArrayType()) { if (!other.isArrayType()) { throw new DdlException("Cannot change " + checkType.toSql() + " to " + other.toSql()); } checkSupportSchemaChangeForComplexType(((ArrayType) checkType).getItemType(), - ((ArrayType) other).getItemType()); - } else if (checkType.isMapType()) { - if (!other.isMapType()) { - throw new DdlException("Cannot change " + checkType.toSql() + " to " + other.toSql()); - } + ((ArrayType) other).getItemType(), true); + } else if (checkType.isMapType() && other.isMapType()) { checkSupportSchemaChangeForComplexType(((MapType) checkType).getKeyType(), - ((MapType) other).getKeyType()); + ((MapType) other).getKeyType(), true); checkSupportSchemaChangeForComplexType(((MapType) checkType).getValueType(), - ((MapType) other).getValueType()); + ((MapType) other).getValueType(), true); } else { - checkSupportSchemaChangeForCharType(checkType, other); + // only support char-type schema change behavior for nested complex type + // if nested is false, we do not check return value. + if (nested && !checkSupportSchemaChangeForCharType(checkType, other)) { + throw new DdlException("Cannot change " + checkType.toSql() + " to " + other.toSql()); + } } } From af9aec88f891b2754972b370227dcf80a3a15134 Mon Sep 17 00:00:00 2001 From: amorynan Date: Tue, 14 Jan 2025 12:01:44 +0800 Subject: [PATCH 7/8] fix string --- .../src/main/java/org/apache/doris/catalog/ColumnType.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java index 329f1941be24ab..078f803d30e54f 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java @@ -184,7 +184,9 @@ public static boolean checkSupportSchemaChangeForCharType(Type checkType, Type o } return true; } else { - return false; + // both are string no need to check length + return checkType.getPrimitiveType() == PrimitiveType.STRING + && other.getPrimitiveType() == PrimitiveType.STRING; } } From 8b2aaf73c318049ffdb15f19e1d6f1cd3fdcc647 Mon Sep 17 00:00:00 2001 From: amorynan Date: Tue, 14 Jan 2025 16:08:49 +0800 Subject: [PATCH 8/8] fix code --- .../src/main/java/org/apache/doris/catalog/ColumnType.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java index 078f803d30e54f..234c0d4eef0efb 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/ColumnType.java @@ -184,9 +184,8 @@ public static boolean checkSupportSchemaChangeForCharType(Type checkType, Type o } return true; } else { - // both are string no need to check length - return checkType.getPrimitiveType() == PrimitiveType.STRING - && other.getPrimitiveType() == PrimitiveType.STRING; + // types equal can return true + return checkType.equals(other); } }