From d1244187e57b2f6c65de21a2d7cca21082f24cfd Mon Sep 17 00:00:00 2001 From: Gavin Chou Date: Wed, 1 Jul 2026 03:10:29 +0800 Subject: [PATCH 1/3] [fix](storage) check partition storage policy resource before BE update --- .../java/org/apache/doris/alter/Alter.java | 56 ++++++++++++++++- .../modify_partition_add_policy.groovy | 61 +++++++++++++++++++ 2 files changed, 116 insertions(+), 1 deletion(-) 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..28b42d00c3fab4 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,56 @@ private void processRename(Database db, Table table, List alterOps) thr } } + private void checkModifyPartitionStoragePolicyResource(OlapTable olapTable, List partitionNames, + Map properties, boolean isTempPartition) throws AnalysisException { + if (!PropertyAnalyzer.hasStoragePolicy(properties)) { + return; + } + + String newStoragePolicy = PropertyAnalyzer.analyzeStoragePolicy(properties); + if (Strings.isNullOrEmpty(newStoragePolicy)) { + return; + } + + 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/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..a0b7e28b4bcbc9 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,61 @@ 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") + } + + def alter_table_partition_diff_resource_result = try_sql """ + ALTER TABLE create_table_partition MODIFY PARTITION (*) SET("storage_policy"="created_create_table_partition_alter_policy_diff"); + """ + assertEquals(alter_table_partition_diff_resource_result, null) + + 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 +203,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 + """ } From 3fdb4102958c0185471cdac5500cc8f62a1dde10 Mon Sep 17 00:00:00 2001 From: Gavin Chou Date: Thu, 2 Jul 2026 01:43:14 +0800 Subject: [PATCH 2/3] [test](storage) assert partition policy resource error --- .../modify_partition_add_policy.groovy | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) 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 a0b7e28b4bcbc9..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 @@ -144,10 +144,12 @@ suite("add_table_policy_by_modify_partition") { assertTrue(par.RemoteStoragePolicy == "created_create_table_partition_alter_policy") } - def alter_table_partition_diff_resource_result = try_sql """ + test { + sql """ ALTER TABLE create_table_partition MODIFY PARTITION (*) SET("storage_policy"="created_create_table_partition_alter_policy_diff"); - """ - assertEquals(alter_table_partition_diff_resource_result, null) + """ + 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 From 0ffe2737b75697a7c2079016bb9e64d426e3ce39 Mon Sep 17 00:00:00 2001 From: Gavin Chou Date: Thu, 2 Jul 2026 01:50:38 +0800 Subject: [PATCH 3/3] [fix](storage) validate target policy before partition meta update --- .../java/org/apache/doris/alter/Alter.java | 3 +- .../ModifyPartitionStoragePolicyTest.java | 116 ++++++++++++++++++ 2 files changed, 118 insertions(+), 1 deletion(-) create mode 100644 fe/fe-core/src/test/java/org/apache/doris/alter/ModifyPartitionStoragePolicyTest.java 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 28b42d00c3fab4..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 @@ -921,7 +921,7 @@ private void processRename(Database db, Table table, List alterOps) thr } private void checkModifyPartitionStoragePolicyResource(OlapTable olapTable, List partitionNames, - Map properties, boolean isTempPartition) throws AnalysisException { + Map properties, boolean isTempPartition) throws DdlException, AnalysisException { if (!PropertyAnalyzer.hasStoragePolicy(properties)) { return; } @@ -930,6 +930,7 @@ private void checkModifyPartitionStoragePolicyResource(OlapTable olapTable, List if (Strings.isNullOrEmpty(newStoragePolicy)) { return; } + Env.getCurrentEnv().getPolicyMgr().checkStoragePolicyExist(newStoragePolicy); Optional newPolicy = Env.getCurrentEnv().getPolicyMgr() .findPolicy(newStoragePolicy, PolicyTypeEnum.STORAGE); 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); + } + } +}