Skip to content

Commit

Permalink
[improvement](alter table) cann't rewrite force property (apache#31820)
Browse files Browse the repository at this point in the history
  • Loading branch information
yujun777 committed Mar 5, 2024
1 parent d2688ae commit fcec33f
Show file tree
Hide file tree
Showing 6 changed files with 87 additions and 2 deletions.
24 changes: 24 additions & 0 deletions fe/fe-core/src/main/java/org/apache/doris/alter/Alter.java
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
import org.apache.doris.common.util.DynamicPartitionUtil;
import org.apache.doris.common.util.MetaLockUtils;
import org.apache.doris.common.util.PropertyAnalyzer;
import org.apache.doris.common.util.PropertyAnalyzer.RewriteProperty;
import org.apache.doris.nereids.trees.plans.commands.info.TableNameInfo;
import org.apache.doris.persist.AlterMTMV;
import org.apache.doris.persist.AlterViewInfo;
Expand Down Expand Up @@ -141,6 +142,19 @@ private boolean processAlterOlapTable(AlterTableStmt stmt, OlapTable olapTable,
AlterOperations currentAlterOps = new AlterOperations();
currentAlterOps.checkConflict(alterClauses);

for (AlterClause clause : alterClauses) {
Map<String, String> properties = null;
try {
properties = clause.getProperties();
} catch (Exception e) {
continue;
}

if (properties != null && !properties.isEmpty()) {
checkNoForceProperty(properties);
}
}

if (olapTable instanceof MTMV) {
currentAlterOps.checkMTMVAllow(alterClauses);
}
Expand Down Expand Up @@ -726,6 +740,7 @@ public void modifyPartitionsProperty(Database db,
Map<String, String> properties,
boolean isTempPartition)
throws DdlException, AnalysisException {
checkNoForceProperty(properties);
Preconditions.checkArgument(olapTable.isWriteLockHeldByCurrentThread());
List<ModifyPartitionInfo> modifyPartitionInfos = Lists.newArrayList();
olapTable.checkNormalStateForAlter();
Expand Down Expand Up @@ -806,6 +821,15 @@ public void modifyPartitionsProperty(Database db,
Env.getCurrentEnv().getEditLog().logBatchModifyPartition(info);
}

public void checkNoForceProperty(Map<String, String> properties) throws DdlException {
for (RewriteProperty property : PropertyAnalyzer.getInstance().getForceProperties()) {
if (properties.containsKey(property.key())) {
throw new DdlException("Cann't modify property '" + property.key() + "'"
+ (Config.isCloudMode() ? " in cloud mode" : "") + ".");
}
}
}

public void replayModifyPartition(ModifyPartitionInfo info) throws MetaNotFoundException {
Database db = Env.getCurrentInternalCatalog().getDbOrMetaException(info.getDbId());
OlapTable olapTable = (OlapTable) db.getTableOrMetaException(info.getTableId(), TableType.OLAP);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2172,6 +2172,7 @@ static long storagePolicyNameToId(String storagePolicy) throws DdlException {
*/
public void updateTableProperties(Database db, String tableName, Map<String, String> properties)
throws UserException {
Env.getCurrentEnv().getAlterInstance().checkNoForceProperty(properties);
List<Partition> partitions = Lists.newArrayList();
OlapTable olapTable = (OlapTable) db.getTableOrMetaException(tableName, Table.TableType.OLAP);
boolean enableUniqueKeyMergeOnWrite = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,9 @@ public void run() {

@VisibleForTesting
public static void modifyTblReplicaCount(Database database, String tblName) {
if (!(Config.min_replication_num_per_tablet < StatisticConstants.STATISTIC_INTERNAL_TABLE_REPLICA_NUM
&& Config.max_replication_num_per_tablet >= StatisticConstants.STATISTIC_INTERNAL_TABLE_REPLICA_NUM)) {
if (Config.isCloudMode()
|| Config.min_replication_num_per_tablet >= StatisticConstants.STATISTIC_INTERNAL_TABLE_REPLICA_NUM
|| Config.max_replication_num_per_tablet < StatisticConstants.STATISTIC_INTERNAL_TABLE_REPLICA_NUM) {
return;
}
while (true) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ public CloudPropertyAnalyzer() {
String.valueOf(ReplicaAllocation.DEFAULT_ALLOCATION.getTotalReplicaNum())),
RewriteProperty.replace(PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION,
ReplicaAllocation.DEFAULT_ALLOCATION.toCreateStmt()),
RewriteProperty.delete("default." + PropertyAnalyzer.PROPERTIES_REPLICATION_NUM),
RewriteProperty.delete("default." + PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION),
RewriteProperty.delete(DynamicPartitionProperty.STORAGE_MEDIUM),
RewriteProperty.replace(DynamicPartitionProperty.REPLICATION_NUM,
String.valueOf(ReplicaAllocation.DEFAULT_ALLOCATION.getTotalReplicaNum())),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,10 @@ private RewriteProperty(RewriteType rewriteType, String key, String value) {
this.value = value;
}

public String key() {
return this.key;
}

public static RewriteProperty put(String key, String value) {
return new RewriteProperty(RewriteType.PUT, key, value);
}
Expand Down Expand Up @@ -1366,6 +1370,10 @@ public void rewriteForceProperties(Map<String, String> properties) {
forceProperties.forEach(property -> property.rewrite(properties));
}

public ImmutableList<RewriteProperty> getForceProperties() {
return forceProperties;
}

private static Map<String, String> rewriteReplicaAllocationProperties(
String ctl, String db, Map<String, String> properties) {
if (Config.force_olap_table_replication_num <= 0) {
Expand Down
49 changes: 49 additions & 0 deletions regression-test/suites/alter_p0/test_alter_force_property.groovy
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// 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_alter_force_property') {
def tbl = 'test_alter_force_property_tbl'
sql "DROP TABLE IF EXISTS ${tbl} FORCE"
sql """
CREATE TABLE ${tbl}
(
k1 int,
k2 int
)
DISTRIBUTED BY HASH(k1) BUCKETS 6
PROPERTIES
(
"replication_num" = "1"
);
"""

if (isCloudCluster()) {
test {
sql "ALTER TABLE ${tbl} SET ('default.replication_num' = '1')"
exception "Cann't modify property 'default.replication_allocation' in cloud mode."
}
test {
sql "ALTER TABLE ${tbl} MODIFY PARTITION (*) SET ('replication_num' = '1')"
exception "Cann't modify property 'replication_num' in cloud mode."
}
} else {
sql "ALTER TABLE ${tbl} SET ('default.replication_num' = '1')"
sql "ALTER TABLE ${tbl} MODIFY PARTITION (*) SET ('replication_num' = '1')"
}

sql "DROP TABLE IF EXISTS ${tbl} FORCE"
}

0 comments on commit fcec33f

Please sign in to comment.