diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/ModifyTablePropertiesClause.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/ModifyTablePropertiesClause.java index c092c7df6732e0..1b3edc871c9681 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/ModifyTablePropertiesClause.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/ModifyTablePropertiesClause.java @@ -83,6 +83,8 @@ public void analyze() throws AnalysisException { throw new AnalysisException("Properties is not set"); } + PropertyAnalyzer.analyzeTTLAlter(properties); + if (properties.size() != 1 && !TableProperty.isSamePrefixProperties( properties, DynamicPartitionProperty.DYNAMIC_PARTITION_PROPERTY_PREFIX) @@ -348,9 +350,6 @@ public void analyze() throws AnalysisException { } this.needTableStable = false; this.opType = AlterOpType.MODIFY_TABLE_PROPERTY_SYNC; - } else if (properties.containsKey(PropertyAnalyzer.PROPERTIES_FILE_CACHE_TTL_SECONDS)) { - this.needTableStable = false; - this.opType = AlterOpType.MODIFY_TABLE_PROPERTY_SYNC; } else if (properties.containsKey(PropertyAnalyzer.PROPERTIES_STORAGE_VAULT_NAME)) { throw new AnalysisException("You can not modify storage vault name"); } else if (properties.containsKey(PropertyAnalyzer.PROPERTIES_STORAGE_VAULT_ID)) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/cloud/alter/CloudSchemaChangeHandler.java b/fe/fe-core/src/main/java/org/apache/doris/cloud/alter/CloudSchemaChangeHandler.java index 1b4c1899845cf1..6fea3e9c2b01e4 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/cloud/alter/CloudSchemaChangeHandler.java +++ b/fe/fe-core/src/main/java/org/apache/doris/cloud/alter/CloudSchemaChangeHandler.java @@ -98,11 +98,11 @@ public void updatePartitionsProperties(Database db, String tableName, List properties) throws UserException { + PropertyAnalyzer.analyzeTTLAlter(properties); final Set allowedProps = new HashSet() { { add(PropertyAnalyzer.PROPERTIES_GROUP_COMMIT_INTERVAL_MS); add(PropertyAnalyzer.PROPERTIES_GROUP_COMMIT_DATA_BYTES); - add(PropertyAnalyzer.PROPERTIES_FILE_CACHE_TTL_SECONDS); add(PropertyAnalyzer.PROPERTIES_COMPACTION_POLICY); add(PropertyAnalyzer.PROPERTIES_TIME_SERIES_COMPACTION_GOAL_SIZE_MBYTES); add(PropertyAnalyzer.PROPERTIES_TIME_SERIES_COMPACTION_FILE_COUNT_THRESHOLD); @@ -136,22 +136,7 @@ public void updateTableProperties(Database db, String tableName, Map properties) { public PropertyAnalyzer() { forceProperties = ImmutableList.of( - RewriteProperty.replace(PROPERTIES_FILE_CACHE_TTL_SECONDS, "0") + RewriteProperty.delete(PROPERTIES_FILE_CACHE_TTL_SECONDS) ); } @@ -609,17 +618,23 @@ public static long analyzeTTL(Map properties) throws AnalysisExc String ttlSecondsStr = properties.get(PROPERTIES_FILE_CACHE_TTL_SECONDS); try { ttlSeconds = Long.parseLong(ttlSecondsStr); - if (ttlSeconds < 0) { - throw new NumberFormatException(); - } } catch (NumberFormatException e) { - throw new AnalysisException("The value " + ttlSecondsStr + " formats error or is out of range " - + "(0 < integer < Long.MAX_VALUE)"); + throw new AnalysisException("The value " + ttlSecondsStr + " formats error or is out of range " + + "(" + MIN_FILE_CACHE_TTL_SECONDS + " <= integer <= Long.MAX_VALUE)"); + } + if (ttlSeconds < MIN_FILE_CACHE_TTL_SECONDS) { + throw new AnalysisException(FILE_CACHE_TTL_CREATE_RESTRICTION_MSG); } } return ttlSeconds; } + public static void analyzeTTLAlter(Map properties) throws AnalysisException { + if (properties.containsKey(PROPERTIES_FILE_CACHE_TTL_SECONDS)) { + throw new AnalysisException(FILE_CACHE_TTL_ALTER_RESTRICTION_MSG); + } + } + public static int analyzePartitionRetentionCount(Map properties) throws AnalysisException { int retentionCount = -1; if (properties != null && properties.containsKey(PROPERTIES_PARTITION_RETENTION_COUNT)) { diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/CreateTableInfo.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/CreateTableInfo.java index c897a216268f34..c3017abb56e1df 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/CreateTableInfo.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/CreateTableInfo.java @@ -190,8 +190,7 @@ public CreateTableInfo( this.partitionTableInfo = partitionTableInfo; this.distribution = distribution; this.rollups = Utils.copyRequiredList(rollups); - this.properties = properties; - PropertyAnalyzer.getInstance().rewriteForceProperties(this.properties); + this.properties = analyzeTTLAndRewriteForceProperties(properties); this.extProperties = extProperties; this.clusterKeysColumnNames = Utils.copyRequiredList(clusterKeyColumnNames); } @@ -234,8 +233,7 @@ public CreateTableInfo( this.partitionTableInfo = partitionTableInfo; this.distribution = distribution; this.rollups = Utils.copyRequiredList(rollups); - this.properties = properties; - PropertyAnalyzer.getInstance().rewriteForceProperties(this.properties); + this.properties = analyzeTTLAndRewriteForceProperties(properties); this.extProperties = extProperties; this.clusterKeysColumnNames = Utils.copyRequiredList(clusterKeyColumnNames); } @@ -255,8 +253,17 @@ public CreateTableInfo( this.keys = keys; this.comment = comment; this.distribution = distribution; - this.properties = properties; - PropertyAnalyzer.getInstance().rewriteForceProperties(this.properties); + this.properties = analyzeTTLAndRewriteForceProperties(properties); + } + + private static Map analyzeTTLAndRewriteForceProperties(Map properties) { + try { + PropertyAnalyzer.analyzeTTL(properties); + } catch (org.apache.doris.common.AnalysisException e) { + throw new AnalysisException(e.getMessage(), e.getCause()); + } + PropertyAnalyzer.getInstance().rewriteForceProperties(properties); + return properties; } /** @@ -488,6 +495,11 @@ public void validate(ConnectContext ctx) { if (engineName.equalsIgnoreCase(ENGINE_OLAP)) { boolean enableDuplicateWithoutKeysByDefault = false; + try { + PropertyAnalyzer.analyzeTTL(properties); + } catch (org.apache.doris.common.AnalysisException e) { + throw new AnalysisException(e.getMessage(), e.getCause()); + } properties = PropertyAnalyzer.getInstance().rewriteOlapProperties(ctlName, dbName, properties); // In fuzzy tests, randomly set storage_format=V3 (ext_meta) for some tables. diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ModifyTablePropertiesOp.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ModifyTablePropertiesOp.java index 6376d24a46aa86..b992873d6e453c 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ModifyTablePropertiesOp.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/ModifyTablePropertiesOp.java @@ -76,6 +76,8 @@ public void validate(ConnectContext ctx) throws UserException { throw new AnalysisException("Properties is not set"); } + PropertyAnalyzer.analyzeTTLAlter(properties); + if (properties.size() != 1 && !TableProperty.isSamePrefixProperties( properties, DynamicPartitionProperty.DYNAMIC_PARTITION_PROPERTY_PREFIX) @@ -341,9 +343,6 @@ public void validate(ConnectContext ctx) throws UserException { } this.needTableStable = false; this.opType = AlterOpType.MODIFY_TABLE_PROPERTY_SYNC; - } else if (properties.containsKey(PropertyAnalyzer.PROPERTIES_FILE_CACHE_TTL_SECONDS)) { - this.needTableStable = false; - this.opType = AlterOpType.MODIFY_TABLE_PROPERTY_SYNC; } else if (properties.containsKey(PropertyAnalyzer.PROPERTIES_PARTITION_RETENTION_COUNT)) { // do a check here for valid value int retentionCount = -1; diff --git a/fe/fe-core/src/test/java/org/apache/doris/analysis/ModifyTablePropertiesTtlTest.java b/fe/fe-core/src/test/java/org/apache/doris/analysis/ModifyTablePropertiesTtlTest.java new file mode 100644 index 00000000000000..751f079c872fb9 --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/analysis/ModifyTablePropertiesTtlTest.java @@ -0,0 +1,76 @@ +// 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.analysis; + +import org.apache.doris.cloud.alter.CloudSchemaChangeHandler; +import org.apache.doris.common.AnalysisException; +import org.apache.doris.common.util.PropertyAnalyzer; +import org.apache.doris.nereids.trees.plans.commands.info.ModifyTablePropertiesOp; + +import com.google.common.collect.Maps; +import org.junit.Assert; +import org.junit.Test; + +import java.util.Map; + +public class ModifyTablePropertiesTtlTest { + @Test + public void testLegacyAlterRejectsFileCacheTtl() { + ModifyTablePropertiesClause clause = new ModifyTablePropertiesClause(ttlProperties("31536000")); + + AnalysisException exception = Assert.assertThrows(AnalysisException.class, clause::analyze); + assertTtlAlterRejected(exception); + } + + @Test + public void testNereidsAlterRejectsFileCacheTtl() { + ModifyTablePropertiesOp op = new ModifyTablePropertiesOp(ttlProperties("31536000")); + + AnalysisException exception = Assert.assertThrows(AnalysisException.class, () -> op.validate(null)); + assertTtlAlterRejected(exception); + } + + @Test + public void testMixedPropertiesStillRejectsFileCacheTTL() { + Map properties = ttlProperties("31536000"); + properties.put(PropertyAnalyzer.PROPERTIES_DISABLE_AUTO_COMPACTION, "true"); + + AnalysisException exception = Assert.assertThrows(AnalysisException.class, + () -> new ModifyTablePropertiesClause(properties).analyze()); + assertTtlAlterRejected(exception); + } + + @Test + public void testCloudSchemaChangeRejectsFileCacheTtlBeforeExecution() { + CloudSchemaChangeHandler handler = new CloudSchemaChangeHandler(); + + AnalysisException exception = Assert.assertThrows(AnalysisException.class, + () -> handler.updateTableProperties(null, "tbl", ttlProperties("31536000"))); + assertTtlAlterRejected(exception); + } + + private static Map ttlProperties(String ttlSeconds) { + Map properties = Maps.newHashMap(); + properties.put(PropertyAnalyzer.PROPERTIES_FILE_CACHE_TTL_SECONDS, ttlSeconds); + return properties; + } + + private static void assertTtlAlterRejected(AnalysisException exception) { + Assert.assertTrue(exception.getMessage().contains(PropertyAnalyzer.FILE_CACHE_TTL_ALTER_RESTRICTION_MSG)); + } +} diff --git a/fe/fe-core/src/test/java/org/apache/doris/catalog/CreateTableTest.java b/fe/fe-core/src/test/java/org/apache/doris/catalog/CreateTableTest.java index bec16a17a647b4..86c840e7021305 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/catalog/CreateTableTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/catalog/CreateTableTest.java @@ -24,6 +24,7 @@ import org.apache.doris.common.ExceptionChecker; import org.apache.doris.common.FeConstants; import org.apache.doris.common.UserException; +import org.apache.doris.common.util.PropertyAnalyzer; import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.resource.Tag; import org.apache.doris.utframe.TestWithFeService; @@ -248,6 +249,34 @@ public void testNormal() throws DdlException, ConfigException { + "distributed by hash(k2) buckets 1\n" + "properties('replication_num' = '1'); ")); } + @Test + public void testFileCacheTtlCreateValidation() throws Exception { + ExceptionChecker.expectThrowsWithMsg(AnalysisException.class, + PropertyAnalyzer.FILE_CACHE_TTL_CREATE_RESTRICTION_MSG, + () -> createTable("create table test.tbl_ttl_low\n" + + "(k1 int)\n" + + "duplicate key(k1)\n" + + "distributed by hash(k1) buckets 1\n" + + "properties('replication_num' = '1', 'file_cache_ttl_seconds' = '31535999');")); + + createTable("create table test.tbl_ttl_min\n" + + "(k1 int)\n" + + "duplicate key(k1)\n" + + "distributed by hash(k1) buckets 1\n" + + "properties('replication_num' = '1', 'file_cache_ttl_seconds' = '31536000');"); + Database db = Env.getCurrentInternalCatalog().getDbOrDdlException("test"); + OlapTable ttlTable = (OlapTable) db.getTableOrDdlException("tbl_ttl_min"); + Assert.assertNotNull(ttlTable); + + createTable("create table test.tbl_ttl_default\n" + + "(k1 int)\n" + + "duplicate key(k1)\n" + + "distributed by hash(k1) buckets 1\n" + + "properties('replication_num' = '1');"); + OlapTable defaultTable = (OlapTable) db.getTableOrDdlException("tbl_ttl_default"); + Assert.assertEquals(0L, defaultTable.getTTLSeconds()); + } + @Test public void testAbnormal() throws DdlException, ConfigException { ExceptionChecker.expectThrowsWithMsg(DdlException.class, diff --git a/fe/fe-core/src/test/java/org/apache/doris/common/PropertyAnalyzerTest.java b/fe/fe-core/src/test/java/org/apache/doris/common/PropertyAnalyzerTest.java index 926d803f931e09..806147322fa3b3 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/common/PropertyAnalyzerTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/common/PropertyAnalyzerTest.java @@ -175,6 +175,41 @@ public void testStorageFormat() throws AnalysisException { PropertyAnalyzer.analyzeStorageFormat(propertiesV1); } + @Test + public void testAnalyzeTtl() throws AnalysisException { + Map properties = Maps.newHashMap(); + Assert.assertEquals(0L, PropertyAnalyzer.analyzeTTL(properties)); + + properties.put(PropertyAnalyzer.PROPERTIES_FILE_CACHE_TTL_SECONDS, + String.valueOf(PropertyAnalyzer.MIN_FILE_CACHE_TTL_SECONDS)); + Assert.assertEquals(PropertyAnalyzer.MIN_FILE_CACHE_TTL_SECONDS, PropertyAnalyzer.analyzeTTL(properties)); + + properties.put(PropertyAnalyzer.PROPERTIES_FILE_CACHE_TTL_SECONDS, String.valueOf(Long.MAX_VALUE)); + Assert.assertEquals(Long.MAX_VALUE, PropertyAnalyzer.analyzeTTL(properties)); + + properties.put(PropertyAnalyzer.PROPERTIES_FILE_CACHE_TTL_SECONDS, + String.valueOf(PropertyAnalyzer.MIN_FILE_CACHE_TTL_SECONDS - 1)); + AnalysisException exception = Assert.assertThrows(AnalysisException.class, + () -> PropertyAnalyzer.analyzeTTL(properties)); + Assert.assertTrue(exception.getMessage().contains(PropertyAnalyzer.FILE_CACHE_TTL_CREATE_RESTRICTION_MSG)); + + properties.put(PropertyAnalyzer.PROPERTIES_FILE_CACHE_TTL_SECONDS, "0"); + exception = Assert.assertThrows(AnalysisException.class, () -> PropertyAnalyzer.analyzeTTL(properties)); + Assert.assertTrue(exception.getMessage().contains(PropertyAnalyzer.FILE_CACHE_TTL_CREATE_RESTRICTION_MSG)); + + properties.put(PropertyAnalyzer.PROPERTIES_FILE_CACHE_TTL_SECONDS, "-1"); + exception = Assert.assertThrows(AnalysisException.class, () -> PropertyAnalyzer.analyzeTTL(properties)); + Assert.assertTrue(exception.getMessage().contains(PropertyAnalyzer.FILE_CACHE_TTL_CREATE_RESTRICTION_MSG)); + + properties.put(PropertyAnalyzer.PROPERTIES_FILE_CACHE_TTL_SECONDS, "invalid"); + exception = Assert.assertThrows(AnalysisException.class, () -> PropertyAnalyzer.analyzeTTL(properties)); + Assert.assertTrue(exception.getMessage().contains("formats error or is out of range")); + + properties.put(PropertyAnalyzer.PROPERTIES_FILE_CACHE_TTL_SECONDS, "9223372036854775808"); + exception = Assert.assertThrows(AnalysisException.class, () -> PropertyAnalyzer.analyzeTTL(properties)); + Assert.assertTrue(exception.getMessage().contains("formats error or is out of range")); + } + @Test public void testTag() throws AnalysisException { HashMap properties = Maps.newHashMap();