diff --git a/fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java b/fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java index 1825170b3200a8..9f9eb03ceec676 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java +++ b/fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java @@ -94,6 +94,8 @@ import org.apache.doris.persist.ModifyTableEngineOperationLog; import org.apache.doris.persist.ModifyTablePropertyOperationLog; import org.apache.doris.persist.ReplaceTableOperationLog; +import org.apache.doris.policy.Policy; +import org.apache.doris.policy.PolicyTypeEnum; import org.apache.doris.policy.StoragePolicy; import org.apache.doris.qe.ConnectContext; import org.apache.doris.qe.ConnectContextUtil; @@ -647,9 +649,11 @@ public void processAlterTable(AlterTableCommand command) throws UserException { ModifyPartitionOp clause = ((ModifyPartitionOp) alterOp); Map properties = clause.getProperties(); List partitionNames = clause.getPartitionNames(); + OlapTable olapTable = (OlapTable) tableIf; + checkModifyPartitionStoragePolicyResource( + olapTable, partitionNames, properties, clause.isTempPartition()); ((SchemaChangeHandler) schemaChangeHandler).updatePartitionsProperties( db, tableName, partitionNames, properties); - OlapTable olapTable = (OlapTable) tableIf; olapTable.writeLockOrDdlException(); try { modifyPartitionsProperty(db, olapTable, partitionNames, properties, clause.isTempPartition()); @@ -916,6 +920,57 @@ private void processRename(Database db, Table table, List alterOps) thr } } + private void checkModifyPartitionStoragePolicyResource(OlapTable olapTable, List partitionNames, + Map properties, boolean isTempPartition) throws DdlException, AnalysisException { + if (!PropertyAnalyzer.hasStoragePolicy(properties)) { + return; + } + + String newStoragePolicy = PropertyAnalyzer.analyzeStoragePolicy(properties); + if (Strings.isNullOrEmpty(newStoragePolicy)) { + return; + } + Env.getCurrentEnv().getPolicyMgr().checkStoragePolicyExist(newStoragePolicy); + + Optional newPolicy = Env.getCurrentEnv().getPolicyMgr() + .findPolicy(newStoragePolicy, PolicyTypeEnum.STORAGE); + if (!newPolicy.isPresent() || !(newPolicy.get() instanceof StoragePolicy)) { + return; + } + + olapTable.readLock(); + try { + PartitionInfo partitionInfo = olapTable.getPartitionInfo(); + for (String partitionName : partitionNames) { + Partition partition = olapTable.getPartition(partitionName, isTempPartition); + if (partition == null) { + continue; + } + + DataProperty dataProperty = partitionInfo.getDataProperty(partition.getId()); + String oldStoragePolicy = dataProperty.getStoragePolicy(); + if (Strings.isNullOrEmpty(oldStoragePolicy) || oldStoragePolicy.equals(newStoragePolicy)) { + continue; + } + + Optional oldPolicy = Env.getCurrentEnv().getPolicyMgr() + .findPolicy(oldStoragePolicy, PolicyTypeEnum.STORAGE); + if (!oldPolicy.isPresent() || !(oldPolicy.get() instanceof StoragePolicy)) { + continue; + } + + String newResource = ((StoragePolicy) newPolicy.get()).getStorageResource(); + String oldResource = ((StoragePolicy) oldPolicy.get()).getStorageResource(); + if (!newResource.equals(oldResource)) { + throw new AnalysisException("currently do not support change origin " + + "storage policy to another one with different resource: "); + } + } + } finally { + olapTable.readUnlock(); + } + } + /** * Batch update partitions' properties * caller should hold the table lock diff --git a/fe/fe-core/src/test/java/org/apache/doris/alter/ModifyPartitionStoragePolicyTest.java b/fe/fe-core/src/test/java/org/apache/doris/alter/ModifyPartitionStoragePolicyTest.java new file mode 100644 index 00000000000000..37d7fadd99e7b4 --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/alter/ModifyPartitionStoragePolicyTest.java @@ -0,0 +1,116 @@ +// 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. + +package org.apache.doris.alter; + +import org.apache.doris.catalog.Env; +import org.apache.doris.common.Config; +import org.apache.doris.common.ExceptionChecker; +import org.apache.doris.common.FeConstants; +import org.apache.doris.common.jmockit.Deencapsulation; +import org.apache.doris.utframe.TestWithFeService; + +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; + +public class ModifyPartitionStoragePolicyTest extends TestWithFeService { + + @Override + protected void beforeCluster() { + Config.enable_storage_policy = true; + FeConstants.runningUnitTest = true; + } + + @Override + protected void runBeforeAll() throws Exception { + createDatabase("test_cir_19357"); + useDatabase("test_cir_19357"); + + executeSql("CREATE RESOURCE IF NOT EXISTS \"cir_19357_resource_a\" " + + "PROPERTIES(" + + "\"type\"=\"s3\"," + + "\"AWS_REGION\"=\"bj\"," + + "\"AWS_ENDPOINT\"=\"bj.s3.comaaaa\"," + + "\"AWS_ROOT_PATH\"=\"path/to/rootaaaa\"," + + "\"AWS_SECRET_KEY\"=\"aaaa\"," + + "\"AWS_ACCESS_KEY\"=\"bbba\"," + + "\"AWS_BUCKET\"=\"test-bucket-a\"," + + "\"s3_validity_check\"=\"false\")"); + executeSql("CREATE RESOURCE IF NOT EXISTS \"cir_19357_resource_b\" " + + "PROPERTIES(" + + "\"type\"=\"s3\"," + + "\"AWS_REGION\"=\"bj\"," + + "\"AWS_ENDPOINT\"=\"bj.s3.comaaaa\"," + + "\"AWS_ROOT_PATH\"=\"path/to/rootaaaa\"," + + "\"AWS_SECRET_KEY\"=\"aaaa\"," + + "\"AWS_ACCESS_KEY\"=\"bbba\"," + + "\"AWS_BUCKET\"=\"test-bucket-b\"," + + "\"s3_validity_check\"=\"false\")"); + executeSql("CREATE STORAGE POLICY IF NOT EXISTS cir_19357_policy_a " + + "PROPERTIES(" + + "\"storage_resource\"=\"cir_19357_resource_a\"," + + "\"cooldown_datetime\"=\"2999-01-01 00:00:00\")"); + executeSql("CREATE STORAGE POLICY IF NOT EXISTS cir_19357_policy_b " + + "PROPERTIES(" + + "\"storage_resource\"=\"cir_19357_resource_b\"," + + "\"cooldown_datetime\"=\"2999-01-01 00:00:00\")"); + + createTable("CREATE TABLE tbl_with_policy (k1 int, k2 int) " + + "PARTITION BY RANGE(k1) (" + + "PARTITION p1 VALUES LESS THAN (\"10\")," + + "PARTITION p2 VALUES LESS THAN (\"20\")) " + + "DISTRIBUTED BY HASH(k1) BUCKETS 1 " + + "PROPERTIES(\"replication_num\"=\"1\", \"storage_policy\"=\"cir_19357_policy_a\")"); + createTable("CREATE TABLE tbl_without_policy (k1 int, k2 int) " + + "PARTITION BY RANGE(k1) (" + + "PARTITION p1 VALUES LESS THAN (\"10\")," + + "PARTITION p2 VALUES LESS THAN (\"20\")) " + + "DISTRIBUTED BY HASH(k1) BUCKETS 1 " + + "PROPERTIES(\"replication_num\"=\"1\")"); + } + + @Test + public void testRejectDifferentResourcePolicyBeforeBeUpdate() throws Exception { + assertModifyPartitionRejectedBeforeBeUpdate( + "ALTER TABLE tbl_with_policy MODIFY PARTITION (*) " + + "SET (\"storage_policy\"=\"cir_19357_policy_b\")", + "currently do not support change origin storage policy to another one with different resource"); + } + + @Test + public void testRejectUninitializedDefaultPolicyBeforeBeUpdate() throws Exception { + assertModifyPartitionRejectedBeforeBeUpdate( + "ALTER TABLE tbl_without_policy MODIFY PARTITION (*) " + + "SET (\"storage_policy\"=\"default_storage_policy\")", + "Use default storage policy, but not give s3 info"); + } + + private void assertModifyPartitionRejectedBeforeBeUpdate(String sql, String errorMessage) throws Exception { + Alter alter = Env.getCurrentEnv().getAlterInstance(); + SchemaChangeHandler originHandler = (SchemaChangeHandler) alter.getSchemaChangeHandler(); + SchemaChangeHandler spyHandler = Mockito.spy(originHandler); + Deencapsulation.setField(alter, "schemaChangeHandler", spyHandler); + try { + ExceptionChecker.expectThrowsWithMsg(IllegalStateException.class, errorMessage, + () -> executeSql(sql)); + Mockito.verify(spyHandler, Mockito.never()).updatePartitionsProperties( + Mockito.any(), Mockito.anyString(), Mockito.anyList(), Mockito.anyMap()); + } finally { + Deencapsulation.setField(alter, "schemaChangeHandler", originHandler); + } + } +} diff --git a/regression-test/suites/cold_heat_separation/empty_table_use_policy/modify_partition_add_policy.groovy b/regression-test/suites/cold_heat_separation/empty_table_use_policy/modify_partition_add_policy.groovy index 9608c296836adb..bff574b04730de 100644 --- a/regression-test/suites/cold_heat_separation/empty_table_use_policy/modify_partition_add_policy.groovy +++ b/regression-test/suites/cold_heat_separation/empty_table_use_policy/modify_partition_add_policy.groovy @@ -112,6 +112,63 @@ suite("add_table_policy_by_modify_partition") { "cooldown_datetime" = "$cooldownTime" ); """ + sql """ALTER STORAGE POLICY tmp2 PROPERTIES("cooldown_datetime" = "$cooldownTime")""" + + try_sql """ + CREATE RESOURCE IF NOT EXISTS "test_modify_partition_table_use_resource_diff" + PROPERTIES( + "type"="s3", + "AWS_REGION" = "bj", + "AWS_ENDPOINT" = "bj.s3.comaaaa", + "AWS_ROOT_PATH" = "path/to/rootaaaa", + "AWS_SECRET_KEY" = "aaaa", + "AWS_ACCESS_KEY" = "bbba", + "AWS_BUCKET" = "test-bucket-diff", + "s3_validity_check" = "false" + ); + """ + try_sql """ + CREATE STORAGE POLICY IF NOT EXISTS created_create_table_partition_alter_policy_diff + PROPERTIES( + "storage_resource" = "test_modify_partition_table_use_resource_diff", + "cooldown_datetime" = "$cooldownTime" + ); + """ + sql """ALTER STORAGE POLICY created_create_table_partition_alter_policy_diff PROPERTIES("cooldown_datetime" = "$cooldownTime")""" + assertEquals(storage_exist.call("created_create_table_partition_alter_policy_diff"), true) + + def partitionsAfterSetPolicy = sql_return_maparray """ + show partitions from create_table_partition + """ + for (def par in partitionsAfterSetPolicy) { + assertTrue(par.RemoteStoragePolicy == "created_create_table_partition_alter_policy") + } + + test { + sql """ + ALTER TABLE create_table_partition MODIFY PARTITION (*) SET("storage_policy"="created_create_table_partition_alter_policy_diff"); + """ + exception "currently do not support change origin storage policy to another one with different resource" + } + + def partitionsAfterFailedAlter = sql_return_maparray """ + show partitions from create_table_partition + """ + for (def par in partitionsAfterFailedAlter) { + assertTrue(par.RemoteStoragePolicy == "created_create_table_partition_alter_policy") + } + + def alter_table_partition_same_resource_result = try_sql """ + ALTER TABLE create_table_partition MODIFY PARTITION (*) SET("storage_policy"="tmp2"); + """ + assertEquals(alter_table_partition_same_resource_result.size(), 1); + + def partitionsAfterSameResourceAlter = sql_return_maparray """ + show partitions from create_table_partition + """ + for (def par in partitionsAfterSameResourceAlter) { + assertTrue(par.RemoteStoragePolicy == "tmp2") + } sql """ CREATE TABLE create_table_partion_use_created_policy_test @@ -148,9 +205,15 @@ suite("add_table_policy_by_modify_partition") { DROP STORAGE POLICY created_create_table_partition_alter_policy """ sql """ + DROP STORAGE POLICY created_create_table_partition_alter_policy_diff + """ + sql """ DROP STORAGE POLICY tmp2 """ sql """ DROP RESOURCE test_modify_partition_table_use_resource """ + sql """ + DROP RESOURCE test_modify_partition_table_use_resource_diff + """ }