From 52729b7dfca89e373dacbc70ba190eb6011b3548 Mon Sep 17 00:00:00 2001 From: Siyang Tang Date: Tue, 4 Mar 2025 13:10:19 +0800 Subject: [PATCH 1/6] [fix](schema-change) Check for string type length change is not completed --- .../doris/alter/SchemaChangeHandler.java | 2 +- .../java/org/apache/doris/catalog/Column.java | 2 + .../org/apache/doris/catalog/ColumnType.java | 30 +++++++--- .../test_type_length_change.groovy | 59 +++++++++++++++++++ 4 files changed, 83 insertions(+), 10 deletions(-) create mode 100644 regression-test/suites/schema_change_p0/test_type_length_change.groovy 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 1831fa27235a73..0d43b1a6afd4ef 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 @@ -630,7 +630,7 @@ 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) { - ColumnType.checkSupportSchemaChangeForCharType(col.getType(), modColumn.getType()); + ColumnType.checkForTypeLengthChange(col.getType(), modColumn.getType()); lightSchemaChange = olapTable.getEnableLightSchemaChange(); } if (columnPos == null && col.getDataType().isComplexType() 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 f7d4065b00c4a0..ad52f4d2083264 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 @@ -851,6 +851,8 @@ public void checkSchemaChangeAllowed(Column other) throws DdlException { throw new DdlException("Can not change " + getDataType() + " to " + other.getDataType()); } + ColumnType.checkForTypeLengthChange(type, other.type); + if (type.isNumericType() && other.type.isStringType()) { try { Integer lSize = type.getColumnStringRepSize(); 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 234c0d4eef0efb..302e53ec457e94 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 @@ -169,19 +169,31 @@ static boolean isSchemaChangeAllowed(Type lhs, Type rhs) { return schemaChangeMatrix[lhs.getPrimitiveType().ordinal()][rhs.getPrimitiveType().ordinal()]; } + /** + * Used for checking type length changing. + * Currently, type length is only meaningful for string types{@link Type#isStringType()}, + * see {@link ScalarType#len}. + */ + public static void checkForTypeLengthChange(Type src, Type dst) throws DdlException { + final int srcTypeLen = src.getLength(); + final int dstTypeLen = dst.getLength(); + if (srcTypeLen < 0 || dstTypeLen < 0) { + // type length is negative means that it is meaningless, just return + return; + } + if (srcTypeLen > dstTypeLen) { + throw new DdlException( + String.format("Shorten type length is prohibited, srcType=%s, dstType=%s", src.toSql(), dst.toSql())); + } + } + // This method defines the char type // to support the schema-change behavior of length growth. // 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) - || (checkType.getPrimitiveType() == PrimitiveType.CHAR - && other.getPrimitiveType() == PrimitiveType.CHAR)) { - if (checkType.getLength() > other.getLength()) { - throw new DdlException("Cannot shorten string length"); - } + private static boolean checkSupportSchemaChangeForCharType(Type checkType, Type other) throws DdlException { + if (checkType.isStringType() && other.isStringType()) { + checkForTypeLengthChange(checkType, other); return true; } else { // types equal can return true diff --git a/regression-test/suites/schema_change_p0/test_type_length_change.groovy b/regression-test/suites/schema_change_p0/test_type_length_change.groovy new file mode 100644 index 00000000000000..5eca6d786e1937 --- /dev/null +++ b/regression-test/suites/schema_change_p0/test_type_length_change.groovy @@ -0,0 +1,59 @@ +// 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. + +suite("test_type_length_change", "p0") { + def tableName = "test_type_length_change"; + sql """ + CREATE TABLE ${tableName} + ( + k INT, + c0 CHAR(5), + c1 VARCHAR(5) + ) + PROPERTIES ( "replication_allocation" = "tag.location.default: 1"); + """ + + sql """ INSERT INTO ${tableName} VALUES(1, "abc", "abc") """ + + test { + sql """ ALTER TABLE ${tableName} MODIFY COLUMN c0 CHAR(3) """ + exception "Shorten type length is prohibited" + } + + sql """ ALTER TABLE ${tableName} MODIFY COLUMN c0 CHAR(6) """ + sql """ INSERT INTO ${tableName} VALUES(2, "abcde", "abc") """ + master_qt_sql """ SELECT * FROM ${tableName} ORDER BY k""" + + test { + sql """ ALTER TABLE ${tableName} MODIFY COLUMN c1 VARCHAR(3) """ + exception "Shorten type length is prohibited" + } + + sql """ ALTER TABLE ${tableName} MODIFY COLUMN c1 VARCHAR(6) """ + sql """ INSERT INTO ${tableName} VALUES(2, "abcde", "abcde") """ + master_qt_sql """ SELECT * FROM ${tableName} ORDER BY k""" + + test { + sql """ ALTER TABLE ${tableName} MODIFY COLUMN c0 VARCHAR(3) """ + exception "Shorten type length is prohibited" + } + + sql """ ALTER TABLE ${tableName} MODIFY COLUMN c0 VARCHAR(6) """ + sql """ INSERT INTO ${tableName} VALUES(3, "abcde", "abcde") """ + master_qt_sql """ SELECT * FROM ${tableName} ORDER BY k""" + master_qt_sql """ DESC ${tableName} """ +} From a491cc3c73404c428dbbf2822ca73823eb8ee02e Mon Sep 17 00:00:00 2001 From: Siyang Tang Date: Tue, 4 Mar 2025 13:10:19 +0800 Subject: [PATCH 2/6] [fix](schema-change) Check for string type length change is not completed --- .../doris/alter/SchemaChangeHandler.java | 2 +- .../java/org/apache/doris/catalog/Column.java | 2 + .../org/apache/doris/catalog/ColumnType.java | 30 +++++--- .../test_type_length_change.out | 21 +++++ .../test_type_length_change.groovy | 77 +++++++++++++++++++ 5 files changed, 122 insertions(+), 10 deletions(-) create mode 100644 regression-test/data/schema_change_p0/test_type_length_change.out create mode 100644 regression-test/suites/schema_change_p0/test_type_length_change.groovy 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 1831fa27235a73..0d43b1a6afd4ef 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 @@ -630,7 +630,7 @@ 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) { - ColumnType.checkSupportSchemaChangeForCharType(col.getType(), modColumn.getType()); + ColumnType.checkForTypeLengthChange(col.getType(), modColumn.getType()); lightSchemaChange = olapTable.getEnableLightSchemaChange(); } if (columnPos == null && col.getDataType().isComplexType() 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 f7d4065b00c4a0..ad52f4d2083264 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 @@ -851,6 +851,8 @@ public void checkSchemaChangeAllowed(Column other) throws DdlException { throw new DdlException("Can not change " + getDataType() + " to " + other.getDataType()); } + ColumnType.checkForTypeLengthChange(type, other.type); + if (type.isNumericType() && other.type.isStringType()) { try { Integer lSize = type.getColumnStringRepSize(); 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 234c0d4eef0efb..302e53ec457e94 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 @@ -169,19 +169,31 @@ static boolean isSchemaChangeAllowed(Type lhs, Type rhs) { return schemaChangeMatrix[lhs.getPrimitiveType().ordinal()][rhs.getPrimitiveType().ordinal()]; } + /** + * Used for checking type length changing. + * Currently, type length is only meaningful for string types{@link Type#isStringType()}, + * see {@link ScalarType#len}. + */ + public static void checkForTypeLengthChange(Type src, Type dst) throws DdlException { + final int srcTypeLen = src.getLength(); + final int dstTypeLen = dst.getLength(); + if (srcTypeLen < 0 || dstTypeLen < 0) { + // type length is negative means that it is meaningless, just return + return; + } + if (srcTypeLen > dstTypeLen) { + throw new DdlException( + String.format("Shorten type length is prohibited, srcType=%s, dstType=%s", src.toSql(), dst.toSql())); + } + } + // This method defines the char type // to support the schema-change behavior of length growth. // 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) - || (checkType.getPrimitiveType() == PrimitiveType.CHAR - && other.getPrimitiveType() == PrimitiveType.CHAR)) { - if (checkType.getLength() > other.getLength()) { - throw new DdlException("Cannot shorten string length"); - } + private static boolean checkSupportSchemaChangeForCharType(Type checkType, Type other) throws DdlException { + if (checkType.isStringType() && other.isStringType()) { + checkForTypeLengthChange(checkType, other); return true; } else { // types equal can return true diff --git a/regression-test/data/schema_change_p0/test_type_length_change.out b/regression-test/data/schema_change_p0/test_type_length_change.out new file mode 100644 index 00000000000000..cfdc79c47c697e --- /dev/null +++ b/regression-test/data/schema_change_p0/test_type_length_change.out @@ -0,0 +1,21 @@ +-- This file is automatically generated. You should know what you did if you want to edit this +-- !master_sql -- +1 abc abc +2 abcde abc + +-- !master_sql -- +1 abc abc +2 abcde abc +3 abcde abcde + +-- !master_sql -- +1 abc abc +2 abcde abc +3 abcde abcde +4 abcde abcde + +-- !master_sql -- +k int Yes true \N +c0 varchar(6) Yes true \N +c1 varchar(6) Yes true \N + diff --git a/regression-test/suites/schema_change_p0/test_type_length_change.groovy b/regression-test/suites/schema_change_p0/test_type_length_change.groovy new file mode 100644 index 00000000000000..6da10d30166520 --- /dev/null +++ b/regression-test/suites/schema_change_p0/test_type_length_change.groovy @@ -0,0 +1,77 @@ +// 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. + +suite("test_type_length_change", "p0") { + def tableName = "test_type_length_change"; + sql """ DROP TABLE IF EXISTS ${tableName} """ + sql """ + CREATE TABLE ${tableName} + ( + k INT, + c0 CHAR(5), + c1 VARCHAR(5) + ) + PROPERTIES ( "replication_allocation" = "tag.location.default: 1"); + """ + + def getTableStatusSql = " SHOW ALTER TABLE COLUMN WHERE IndexName='${tableName}' ORDER BY createtime DESC LIMIT 1 " + + sql """ INSERT INTO ${tableName} VALUES(1, "abc", "abc") """ + + test { + sql """ ALTER TABLE ${tableName} MODIFY COLUMN c0 CHAR(3) """ + exception "Shorten type length is prohibited" + } + + sql """ ALTER TABLE ${tableName} MODIFY COLUMN c0 CHAR(6) """ + waitForSchemaChangeDone { + sql getTableStatusSql + time 600 + } + + sql """ INSERT INTO ${tableName} VALUES(2, "abcde", "abc") """ + qt_master_sql """ SELECT * FROM ${tableName} ORDER BY k""" + + test { + sql """ ALTER TABLE ${tableName} MODIFY COLUMN c1 VARCHAR(3) """ + exception "Shorten type length is prohibited" + } + + sql """ ALTER TABLE ${tableName} MODIFY COLUMN c1 VARCHAR(6) """ + waitForSchemaChangeDone { + sql getTableStatusSql + time 600 + } + + sql """ INSERT INTO ${tableName} VALUES(3, "abcde", "abcde") """ + qt_master_sql """ SELECT * FROM ${tableName} ORDER BY k""" + + test { + sql """ ALTER TABLE ${tableName} MODIFY COLUMN c0 VARCHAR(3) """ + exception "Shorten type length is prohibited" + } + + sql """ ALTER TABLE ${tableName} MODIFY COLUMN c0 VARCHAR(6) """ + waitForSchemaChangeDone { + sql getTableStatusSql + time 600 + } + + sql """ INSERT INTO ${tableName} VALUES(4, "abcde", "abcde") """ + qt_master_sql """ SELECT * FROM ${tableName} ORDER BY k""" + qt_master_sql """ DESC ${tableName} """ +} From b250fbd42026f42b7473aaf4ef5127a6ef3a1a9f Mon Sep 17 00:00:00 2001 From: Siyang Tang Date: Tue, 4 Mar 2025 16:02:54 +0800 Subject: [PATCH 3/6] Update Column.java --- .../src/main/java/org/apache/doris/catalog/Column.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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 ad52f4d2083264..7d0e5de0b93b3d 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 @@ -851,8 +851,6 @@ public void checkSchemaChangeAllowed(Column other) throws DdlException { throw new DdlException("Can not change " + getDataType() + " to " + other.getDataType()); } - ColumnType.checkForTypeLengthChange(type, other.type); - if (type.isNumericType() && other.type.isStringType()) { try { Integer lSize = type.getColumnStringRepSize(); @@ -885,6 +883,10 @@ public void checkSchemaChangeAllowed(Column other) throws DdlException { } } + if (type.isStringType() && other.type.isStringType) { + ColumnType.checkForTypeLengthChange(type, other.type); + } + // 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, false); From 689eaaa75bb05185edc27ed6d655af960056299b Mon Sep 17 00:00:00 2001 From: Siyang Tang Date: Tue, 4 Mar 2025 16:11:00 +0800 Subject: [PATCH 4/6] Update Column.java --- fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 7d0e5de0b93b3d..be1716b1c7d20d 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 @@ -883,7 +883,7 @@ public void checkSchemaChangeAllowed(Column other) throws DdlException { } } - if (type.isStringType() && other.type.isStringType) { + if (type.isStringType() && other.type.isStringType()) { ColumnType.checkForTypeLengthChange(type, other.type); } From 504f9cdea8108cdba80fe5e2e0133e0488a7c5f0 Mon Sep 17 00:00:00 2001 From: Siyang Tang Date: Tue, 4 Mar 2025 19:52:14 +0800 Subject: [PATCH 5/6] fix case --- .../schema_change_p0/test_varchar_sc_in_complex.groovy | 6 +++--- .../schema_change_p0/test_varchar_schema_change.groovy | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) 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 d3d2554025f2ff..172913bd8aa920 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 @@ -62,19 +62,19 @@ suite ("test_varchar_sc_in_complex") { test { sql """ alter table ${tableName} modify column c_a array """ - exception "Cannot shorten string length" + exception "Shorten type length is prohibited" } test { sql """ alter table ${tableName} modify column c_m map """ - exception "Cannot shorten string length" + exception "Shorten type length is prohibited" } test { sql """ alter table ${tableName} modify column c_s struct """ - exception "Cannot shorten string length" + exception "Shorten type length is prohibited" } // add case alter modify array/map/struct to other type diff --git a/regression-test/suites/schema_change_p0/test_varchar_schema_change.groovy b/regression-test/suites/schema_change_p0/test_varchar_schema_change.groovy index 7b5d7897114352..9387cdaed067c8 100644 --- a/regression-test/suites/schema_change_p0/test_varchar_schema_change.groovy +++ b/regression-test/suites/schema_change_p0/test_varchar_schema_change.groovy @@ -55,7 +55,7 @@ suite ("test_varchar_schema_change") { test { sql """ alter table ${tableName} modify column c2 varchar(10) """ - exception "Cannot shorten string length" + exception "Shorten type length is prohibited" } // test {//为什么第一次改没发生Nothing is changed错误？查看branch-1.2-lts代码 From c99c9efeaed24d1f6584dfa18ef0c83d700f1082 Mon Sep 17 00:00:00 2001 From: Siyang Tang Date: Tue, 4 Mar 2025 20:28:10 +0800 Subject: [PATCH 6/6] Update Column.java --- fe/fe-core/src/main/java/org/apache/doris/catalog/Column.java | 2 -- 1 file changed, 2 deletions(-) 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 6c32ce683eb4e6..be1716b1c7d20d 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 @@ -851,8 +851,6 @@ public void checkSchemaChangeAllowed(Column other) throws DdlException { throw new DdlException("Can not change " + getDataType() + " to " + other.getDataType()); } - ColumnType.checkForTypeLengthChange(type, other.type); - if (type.isNumericType() && other.type.isStringType()) { try { Integer lSize = type.getColumnStringRepSize();