From 13780e4827db38494f2f3fb168f3195163a18fc1 Mon Sep 17 00:00:00 2001 From: Mingyu Chen Date: Sat, 21 Oct 2023 23:15:08 +0800 Subject: [PATCH] [fix](nereids)(create-table) fix bug that replication num is not set when create table with no property (#25651) When executing create partitioned table with Nereids, and replication_num property is not set, the replication number will be 0, so the tablet will has no replica. --- .../doris/analysis/CreateTableStmt.java | 75 +----------- .../doris/common/util/PropertyAnalyzer.java | 72 ++++++++++++ .../nereids/parser/LogicalPlanBuilder.java | 11 +- .../plans/commands/info/CreateTableInfo.java | 45 +++++-- .../commands/info/FixedRangePartition.java | 10 +- .../plans/commands/info/InPartition.java | 9 +- .../commands/info/LessThanPartition.java | 10 +- .../commands/info/PartitionDefinition.java | 22 +++- .../plans/commands/info/StepPartition.java | 9 +- .../apache/doris/catalog/CreateTableTest.java | 110 +++++++++++------- .../apache/doris/common/ExceptionChecker.java | 1 + .../doris/utframe/TestWithFeService.java | 37 +++++- 12 files changed, 240 insertions(+), 171 deletions(-) diff --git a/fe/fe-core/src/main/java/org/apache/doris/analysis/CreateTableStmt.java b/fe/fe-core/src/main/java/org/apache/doris/analysis/CreateTableStmt.java index a03884f5c6..f56de294d9 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/analysis/CreateTableStmt.java +++ b/fe/fe-core/src/main/java/org/apache/doris/analysis/CreateTableStmt.java @@ -20,13 +20,11 @@ package org.apache.doris.analysis; import org.apache.doris.analysis.IndexDef.IndexType; import org.apache.doris.catalog.AggregateType; import org.apache.doris.catalog.Column; -import org.apache.doris.catalog.DatabaseIf; import org.apache.doris.catalog.DistributionInfo; import org.apache.doris.catalog.Env; import org.apache.doris.catalog.Index; import org.apache.doris.catalog.KeysType; import org.apache.doris.catalog.PrimitiveType; -import org.apache.doris.catalog.ReplicaAllocation; import org.apache.doris.catalog.Type; import org.apache.doris.common.AnalysisException; import org.apache.doris.common.Config; @@ -41,7 +39,6 @@ import org.apache.doris.common.util.ParseUtil; import org.apache.doris.common.util.PrintableMap; import org.apache.doris.common.util.PropertyAnalyzer; import org.apache.doris.common.util.Util; -import org.apache.doris.datasource.CatalogIf; import org.apache.doris.external.elasticsearch.EsUtil; import org.apache.doris.mysql.privilege.PrivPredicate; import org.apache.doris.qe.ConnectContext; @@ -51,7 +48,6 @@ import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.collect.Sets; import org.apache.commons.collections.CollectionUtils; -import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -559,7 +555,8 @@ public class CreateTableStmt extends DdlStmt { if (engineName.equals("olap")) { // before analyzing partition, handle the replication allocation info - properties = rewriteReplicaAllocationProperties(properties); + properties = PropertyAnalyzer.rewriteReplicaAllocationProperties( + tableName.getCtl(), tableName.getDb(), properties); // analyze partition if (partitionDesc != null) { if (partitionDesc instanceof ListPartitionDesc || partitionDesc instanceof RangePartitionDesc @@ -650,74 +647,6 @@ public class CreateTableStmt extends DdlStmt { } } - private Map rewriteReplicaAllocationProperties(Map properties) - throws AnalysisException { - if (Config.force_olap_table_replication_num <= 0) { - return rewriteReplicaAllocationPropertiesByDatabase(properties); - } - // if force_olap_table_replication_num is set, use this value to rewrite the replication_num or - // replication_allocation properties - Map newProperties = properties; - if (newProperties == null) { - newProperties = Maps.newHashMap(); - } - boolean rewrite = false; - if (newProperties.containsKey(PropertyAnalyzer.PROPERTIES_REPLICATION_NUM)) { - newProperties.put(PropertyAnalyzer.PROPERTIES_REPLICATION_NUM, - String.valueOf(Config.force_olap_table_replication_num)); - rewrite = true; - } - if (newProperties.containsKey(PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION)) { - newProperties.put(PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION, - new ReplicaAllocation((short) Config.force_olap_table_replication_num).toCreateStmt()); - rewrite = true; - } - if (!rewrite) { - newProperties.put(PropertyAnalyzer.PROPERTIES_REPLICATION_NUM, - String.valueOf(Config.force_olap_table_replication_num)); - } - return newProperties; - } - - private Map rewriteReplicaAllocationPropertiesByDatabase(Map properties) - throws AnalysisException { - // if table contain `replication_allocation` or `replication_allocation`,not need rewrite by db - if (properties != null && (properties.containsKey(PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION) - || properties.containsKey(PropertyAnalyzer.PROPERTIES_REPLICATION_NUM))) { - return properties; - } - CatalogIf catalog = Env.getCurrentEnv().getCatalogMgr().getCatalogNullable(tableName.getCtl()); - if (catalog == null) { - return properties; - } - DatabaseIf db = catalog.getDbNullable(tableName.getDb()); - if (db == null) { - return properties; - } - // if db not have properties,not need rewrite - if (db.getDbProperties() == null) { - return properties; - } - Map dbProperties = db.getDbProperties().getProperties(); - if (dbProperties == null) { - return properties; - } - if (properties == null) { - properties = Maps.newHashMap(); - } - if (dbProperties.containsKey(PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION) && StringUtils - .isNotEmpty(dbProperties.get(PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION))) { - properties.put(PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION, - dbProperties.get(PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION)); - } - if (dbProperties.containsKey(PropertyAnalyzer.PROPERTIES_REPLICATION_NUM) && StringUtils - .isNotEmpty(dbProperties.get(PropertyAnalyzer.PROPERTIES_REPLICATION_NUM))) { - properties.put(PropertyAnalyzer.PROPERTIES_REPLICATION_NUM, - dbProperties.get(PropertyAnalyzer.PROPERTIES_REPLICATION_NUM)); - } - return properties; - } - private void analyzeEngineName() throws AnalysisException { if (Strings.isNullOrEmpty(engineName)) { engineName = "olap"; diff --git a/fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java b/fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java index a3f2202246..5e037c2e3b 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java +++ b/fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java @@ -21,6 +21,7 @@ import org.apache.doris.analysis.DataSortInfo; import org.apache.doris.analysis.DateLiteral; import org.apache.doris.catalog.Column; import org.apache.doris.catalog.DataProperty; +import org.apache.doris.catalog.DatabaseIf; import org.apache.doris.catalog.Env; import org.apache.doris.catalog.EsResource; import org.apache.doris.catalog.KeysType; @@ -32,6 +33,7 @@ import org.apache.doris.catalog.Type; import org.apache.doris.common.AnalysisException; import org.apache.doris.common.Config; import org.apache.doris.common.DdlException; +import org.apache.doris.datasource.CatalogIf; import org.apache.doris.datasource.CatalogMgr; import org.apache.doris.policy.Policy; import org.apache.doris.policy.StoragePolicy; @@ -48,6 +50,7 @@ import com.google.common.base.Preconditions; import com.google.common.base.Strings; import com.google.common.collect.Maps; import com.google.common.collect.Sets; +import org.apache.commons.lang3.StringUtils; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -1176,4 +1179,73 @@ public class PropertyAnalyzer { } } } + + public static Map rewriteReplicaAllocationProperties( + String ctl, String db, Map properties) { + if (Config.force_olap_table_replication_num <= 0) { + return rewriteReplicaAllocationPropertiesByDatabase(ctl, db, properties); + } + // if force_olap_table_replication_num is set, use this value to rewrite the replication_num or + // replication_allocation properties + Map newProperties = properties; + if (newProperties == null) { + newProperties = Maps.newHashMap(); + } + boolean rewrite = false; + if (newProperties.containsKey(PropertyAnalyzer.PROPERTIES_REPLICATION_NUM)) { + newProperties.put(PropertyAnalyzer.PROPERTIES_REPLICATION_NUM, + String.valueOf(Config.force_olap_table_replication_num)); + rewrite = true; + } + if (newProperties.containsKey(PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION)) { + newProperties.put(PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION, + new ReplicaAllocation((short) Config.force_olap_table_replication_num).toCreateStmt()); + rewrite = true; + } + if (!rewrite) { + newProperties.put(PropertyAnalyzer.PROPERTIES_REPLICATION_NUM, + String.valueOf(Config.force_olap_table_replication_num)); + } + return newProperties; + } + + private static Map rewriteReplicaAllocationPropertiesByDatabase( + String ctl, String database, Map properties) { + // if table contain `replication_allocation` or `replication_allocation`,not need rewrite by db + if (properties != null && (properties.containsKey(PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION) + || properties.containsKey(PropertyAnalyzer.PROPERTIES_REPLICATION_NUM))) { + return properties; + } + CatalogIf catalog = Env.getCurrentEnv().getCatalogMgr().getCatalogNullable(ctl); + if (catalog == null) { + return properties; + } + DatabaseIf db = catalog.getDbNullable(database); + if (db == null) { + return properties; + } + // if db not have properties,not need rewrite + if (db.getDbProperties() == null) { + return properties; + } + Map dbProperties = db.getDbProperties().getProperties(); + if (dbProperties == null) { + return properties; + } + if (properties == null) { + properties = Maps.newHashMap(); + } + if (dbProperties.containsKey(PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION) && StringUtils + .isNotEmpty(dbProperties.get(PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION))) { + properties.put(PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION, + dbProperties.get(PropertyAnalyzer.PROPERTIES_REPLICATION_ALLOCATION)); + } + if (dbProperties.containsKey(PropertyAnalyzer.PROPERTIES_REPLICATION_NUM) && StringUtils + .isNotEmpty(dbProperties.get(PropertyAnalyzer.PROPERTIES_REPLICATION_NUM))) { + properties.put(PropertyAnalyzer.PROPERTIES_REPLICATION_NUM, + dbProperties.get(PropertyAnalyzer.PROPERTIES_REPLICATION_NUM)); + } + return properties; + } + } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java index 734a215888..b817803f23 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java @@ -1865,8 +1865,9 @@ public class LogicalPlanBuilder extends DorisParserBaseVisitor { @Override public LogicalPlan visitCreateTable(CreateTableContext ctx) { + String ctlName = null; String dbName = null; - String tableName; + String tableName = null; List nameParts = visitMultipartIdentifier(ctx.name); // TODO: support catalog if (nameParts.size() == 1) { @@ -1874,8 +1875,12 @@ public class LogicalPlanBuilder extends DorisParserBaseVisitor { } else if (nameParts.size() == 2) { dbName = nameParts.get(0); tableName = nameParts.get(1); + } else if (nameParts.size() == 3) { + ctlName = nameParts.get(0); + dbName = nameParts.get(1); + tableName = nameParts.get(2); } else { - throw new AnalysisException("nameParts in create table should be 1 or 2"); + throw new AnalysisException("nameParts in create table should be [ctl.][db.]tbl"); } KeysType keysType = null; if (ctx.DUPLICATE() != null) { @@ -1906,6 +1911,7 @@ public class LogicalPlanBuilder extends DorisParserBaseVisitor { } return new CreateTableCommand(Optional.empty(), new CreateTableInfo( ctx.EXISTS() != null, + ctlName, dbName, tableName, visitColumnDefs(ctx.columnDefs()), @@ -1923,6 +1929,7 @@ public class LogicalPlanBuilder extends DorisParserBaseVisitor { } else if (ctx.AS() != null) { return new CreateTableCommand(Optional.of(visitQuery(ctx.query())), new CreateTableInfo( ctx.EXISTS() != null, + ctlName, dbName, tableName, ctx.ctasCols != null ? visitIdentifierList(ctx.ctasCols) : null, 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 f398c7b7d1..af11d4944c 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 @@ -30,17 +30,21 @@ import org.apache.doris.catalog.Column; import org.apache.doris.catalog.Env; import org.apache.doris.catalog.Index; import org.apache.doris.catalog.KeysType; +import org.apache.doris.catalog.PartitionType; import org.apache.doris.catalog.Type; import org.apache.doris.cluster.ClusterNamespace; import org.apache.doris.common.Config; import org.apache.doris.common.FeConstants; import org.apache.doris.common.FeNameFormat; import org.apache.doris.common.util.PropertyAnalyzer; +import org.apache.doris.datasource.InternalCatalog; import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.types.DataType; import org.apache.doris.nereids.util.Utils; import org.apache.doris.qe.ConnectContext; +import com.google.common.base.Preconditions; +import com.google.common.base.Strings; import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; import com.google.common.collect.Maps; @@ -57,6 +61,7 @@ import java.util.stream.Collectors; */ public class CreateTableInfo { private final boolean ifNotExists; + private String ctlName; private String dbName; private final String tableName; private List columns; @@ -77,11 +82,13 @@ public class CreateTableInfo { /** * constructor for create table */ - public CreateTableInfo(boolean ifNotExists, String dbName, String tableName, List columns, - List indexes, String engineName, KeysType keysType, List keys, String comment, + public CreateTableInfo(boolean ifNotExists, String ctlName, String dbName, String tableName, + List columns, List indexes, String engineName, + KeysType keysType, List keys, String comment, String partitionType, List partitionColumns, List partitions, DistributionDescriptor distribution, List rollups, Map properties) { this.ifNotExists = ifNotExists; + this.ctlName = ctlName; this.dbName = dbName; this.tableName = tableName; this.ctasColumns = null; @@ -102,11 +109,12 @@ public class CreateTableInfo { /** * constructor for create table as select */ - public CreateTableInfo(boolean ifNotExists, String dbName, String tableName, List cols, + public CreateTableInfo(boolean ifNotExists, String ctlName, String dbName, String tableName, List cols, String engineName, KeysType keysType, List keys, String comment, String partitionType, List partitionColumns, List partitions, DistributionDescriptor distribution, List rollups, Map properties) { this.ifNotExists = ifNotExists; + this.ctlName = ctlName; this.dbName = dbName; this.tableName = tableName; this.ctasColumns = cols; @@ -128,6 +136,10 @@ public class CreateTableInfo { return ctasColumns; } + public String getCtlName() { + return ctlName; + } + public String getDbName() { return dbName; } @@ -163,20 +175,30 @@ public class CreateTableInfo { try { FeNameFormat.checkTableName(tableName); - if (dbName != null) { - FeNameFormat.checkDbName(dbName); - } } catch (Exception e) { throw new AnalysisException(e.getMessage(), e); } + // analyze catalog name + if (Strings.isNullOrEmpty(ctlName)) { + if (ctx.getCurrentCatalog() != null) { + ctlName = ctx.getCurrentCatalog().getName(); + } else { + ctlName = InternalCatalog.INTERNAL_CATALOG_NAME; + } + } + // analyze table name - if (dbName == null) { + if (Strings.isNullOrEmpty(dbName)) { dbName = ClusterNamespace.getFullName(ctx.getClusterName(), ctx.getDatabase()); } else { dbName = ClusterNamespace.getFullName(ctx.getClusterName(), dbName); } + Preconditions.checkState(!Strings.isNullOrEmpty(ctlName), "catalog name is null or empty"); + Preconditions.checkState(!Strings.isNullOrEmpty(dbName), "database name is null or empty"); + properties = PropertyAnalyzer.rewriteReplicaAllocationProperties(ctlName, dbName, properties); + boolean enableDuplicateWithoutKeysByDefault = false; if (properties != null) { try { @@ -367,14 +389,15 @@ public class CreateTableInfo { * check partitions types. */ private boolean checkPartitionsTypes() { - if (partitionType.equalsIgnoreCase("RANGE")) { + if (partitionType.equalsIgnoreCase(PartitionType.RANGE.name())) { if (partitions.stream().allMatch(p -> p instanceof StepPartition)) { return true; } return partitions.stream().allMatch(p -> (p instanceof LessThanPartition) || (p instanceof FixedRangePartition)); } - return partitionType.equalsIgnoreCase("LIST") && partitions.stream().allMatch(p -> p instanceof InPartition); + return partitionType.equalsIgnoreCase(PartitionType.LIST.name()) + && partitions.stream().allMatch(p -> p instanceof InPartition); } private void validatePartitionColumn(ColumnDefinition column, ConnectContext ctx) { @@ -395,7 +418,7 @@ public class CreateTableInfo { if (!ctx.getSessionVariable().isAllowPartitionColumnNullable() && column.isNullable()) { throw new AnalysisException("The partition column must be NOT NULL"); } - if (partitionType.equalsIgnoreCase("LIST") && column.isNullable()) { + if (partitionType.equalsIgnoreCase(PartitionType.LIST.name()) && column.isNullable()) { throw new AnalysisException("The list partition column must be NOT NULL"); } } @@ -415,7 +438,7 @@ public class CreateTableInfo { List partitionDescs = partitions.stream() .map(PartitionDefinition::translateToCatalogStyle).collect(Collectors.toList()); try { - if (partitionType.equals("RANGE")) { + if (partitionType.equals(PartitionType.RANGE.name())) { partitionDesc = new RangePartitionDesc(partitionColumns, partitionDescs); } else { partitionDesc = new ListPartitionDesc(partitionColumns, partitionDescs); diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/FixedRangePartition.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/FixedRangePartition.java index 634f5c235d..a1d74d2069 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/FixedRangePartition.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/FixedRangePartition.java @@ -20,9 +20,6 @@ package org.apache.doris.nereids.trees.plans.commands.info; import org.apache.doris.analysis.PartitionKeyDesc; import org.apache.doris.analysis.PartitionValue; import org.apache.doris.analysis.SinglePartitionDesc; -import org.apache.doris.catalog.ReplicaAllocation; -import org.apache.doris.common.util.PropertyAnalyzer; -import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.trees.expressions.Expression; import org.apache.doris.nereids.types.DataType; @@ -39,7 +36,6 @@ public class FixedRangePartition extends PartitionDefinition { private final String partitionName; private List lowerBounds; private List upperBounds; - private ReplicaAllocation replicaAllocation = ReplicaAllocation.DEFAULT_ALLOCATION; public FixedRangePartition(String partitionName, List lowerBounds, List upperBounds) { this.partitionName = partitionName; @@ -49,11 +45,7 @@ public class FixedRangePartition extends PartitionDefinition { @Override public void validate(Map properties) { - try { - replicaAllocation = PropertyAnalyzer.analyzeReplicaAllocation(properties, ""); - } catch (Exception e) { - throw new AnalysisException(e.getMessage(), e.getCause()); - } + super.validate(properties); final DataType type = partitionTypes.get(0); lowerBounds = lowerBounds.stream().map(e -> e.castTo(type)).collect(Collectors.toList()); upperBounds = upperBounds.stream().map(e -> e.castTo(type)).collect(Collectors.toList()); diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/InPartition.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/InPartition.java index 900f521cab..84821d9921 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/InPartition.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/InPartition.java @@ -21,8 +21,6 @@ import org.apache.doris.analysis.AllPartitionDesc; import org.apache.doris.analysis.PartitionKeyDesc; import org.apache.doris.analysis.PartitionValue; import org.apache.doris.analysis.SinglePartitionDesc; -import org.apache.doris.catalog.ReplicaAllocation; -import org.apache.doris.common.util.PropertyAnalyzer; import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.trees.expressions.Expression; @@ -38,7 +36,6 @@ import java.util.stream.Collectors; public class InPartition extends PartitionDefinition { private final String partitionName; private final List> values; - private ReplicaAllocation replicaAllocation = ReplicaAllocation.DEFAULT_ALLOCATION; public InPartition(String partitionName, List> values) { this.partitionName = partitionName; @@ -47,11 +44,7 @@ public class InPartition extends PartitionDefinition { @Override public void validate(Map properties) { - try { - replicaAllocation = PropertyAnalyzer.analyzeReplicaAllocation(properties, ""); - } catch (Exception e) { - throw new AnalysisException(e.getMessage(), e.getCause()); - } + super.validate(properties); if (values.stream().anyMatch(l -> l.stream().anyMatch(MaxValue.class::isInstance))) { throw new AnalysisException("MAXVALUE cannot be used in 'in partition'"); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/LessThanPartition.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/LessThanPartition.java index 2537c5d1a1..11a6f4f50d 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/LessThanPartition.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/LessThanPartition.java @@ -20,9 +20,6 @@ package org.apache.doris.nereids.trees.plans.commands.info; import org.apache.doris.analysis.PartitionKeyDesc; import org.apache.doris.analysis.PartitionValue; import org.apache.doris.analysis.SinglePartitionDesc; -import org.apache.doris.catalog.ReplicaAllocation; -import org.apache.doris.common.util.PropertyAnalyzer; -import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.trees.expressions.Expression; import com.google.common.collect.Maps; @@ -37,7 +34,6 @@ import java.util.stream.Collectors; public class LessThanPartition extends PartitionDefinition { private final String partitionName; private final List values; - private ReplicaAllocation replicaAllocation = ReplicaAllocation.DEFAULT_ALLOCATION; public LessThanPartition(String partitionName, List values) { this.partitionName = partitionName; @@ -46,11 +42,7 @@ public class LessThanPartition extends PartitionDefinition { @Override public void validate(Map properties) { - try { - replicaAllocation = PropertyAnalyzer.analyzeReplicaAllocation(properties, ""); - } catch (Exception e) { - throw new AnalysisException(e.getMessage(), e.getCause()); - } + super.validate(properties); } public String getPartitionName() { diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/PartitionDefinition.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/PartitionDefinition.java index a6edda1b83..2d89ed9490 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/PartitionDefinition.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/PartitionDefinition.java @@ -19,6 +19,8 @@ package org.apache.doris.nereids.trees.plans.commands.info; import org.apache.doris.analysis.AllPartitionDesc; import org.apache.doris.analysis.PartitionValue; +import org.apache.doris.catalog.ReplicaAllocation; +import org.apache.doris.common.util.PropertyAnalyzer; import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.trees.expressions.Expression; import org.apache.doris.nereids.trees.expressions.literal.Literal; @@ -34,16 +36,30 @@ import java.util.Map; */ public abstract class PartitionDefinition { protected List partitionTypes; - protected Map propreties; + protected Map properties; + protected ReplicaAllocation replicaAllocation = ReplicaAllocation.DEFAULT_ALLOCATION; public PartitionDefinition withProperties(Map properties) { - this.propreties = properties; + this.properties = properties; return this; } public abstract AllPartitionDesc translateToCatalogStyle(); - public abstract void validate(Map properties); + /** + * Validate the properties. + * Derived class can override this method to do more validation. + */ + public void validate(Map properties) { + try { + replicaAllocation = PropertyAnalyzer.analyzeReplicaAllocation(properties, ""); + if (replicaAllocation.isNotSet()) { + replicaAllocation = ReplicaAllocation.DEFAULT_ALLOCATION; + } + } catch (Exception e) { + throw new AnalysisException(e.getMessage(), e.getCause()); + } + } /** * get partition name diff --git a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/StepPartition.java b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/StepPartition.java index 6588ad1dc3..a02013d377 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/StepPartition.java +++ b/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/info/StepPartition.java @@ -20,8 +20,6 @@ package org.apache.doris.nereids.trees.plans.commands.info; import org.apache.doris.analysis.MultiPartitionDesc; import org.apache.doris.analysis.PartitionKeyDesc; import org.apache.doris.analysis.PartitionValue; -import org.apache.doris.catalog.ReplicaAllocation; -import org.apache.doris.common.util.PropertyAnalyzer; import org.apache.doris.nereids.exceptions.AnalysisException; import org.apache.doris.nereids.trees.expressions.Expression; @@ -39,7 +37,6 @@ public class StepPartition extends PartitionDefinition { private final List toExpression; private final long unit; private final String unitString; - private ReplicaAllocation replicaAllocation = ReplicaAllocation.DEFAULT_ALLOCATION; public StepPartition(List fromExpression, List toExpression, long unit, String unitString) { @@ -51,15 +48,11 @@ public class StepPartition extends PartitionDefinition { @Override public void validate(Map properties) { + super.validate(properties); if (fromExpression.stream().anyMatch(MaxValue.class::isInstance) || toExpression.stream().anyMatch(MaxValue.class::isInstance)) { throw new AnalysisException("MAXVALUE cannot be used in step partition"); } - try { - replicaAllocation = PropertyAnalyzer.analyzeReplicaAllocation(properties, ""); - } catch (Exception e) { - throw new AnalysisException(e.getMessage(), e.getCause()); - } } /** 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 f8b3797321..3bf6af35c2 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 @@ -17,9 +17,6 @@ package org.apache.doris.catalog; -import org.apache.doris.analysis.AlterTableStmt; -import org.apache.doris.analysis.CreateDbStmt; -import org.apache.doris.analysis.CreateTableStmt; import org.apache.doris.common.AnalysisException; import org.apache.doris.common.Config; import org.apache.doris.common.ConfigBase; @@ -27,51 +24,27 @@ import org.apache.doris.common.ConfigException; import org.apache.doris.common.DdlException; import org.apache.doris.common.ExceptionChecker; import org.apache.doris.common.UserException; -import org.apache.doris.qe.ConnectContext; -import org.apache.doris.utframe.UtFrameUtils; +import org.apache.doris.utframe.TestWithFeService; -import org.junit.AfterClass; import org.junit.Assert; -import org.junit.BeforeClass; -import org.junit.Test; +import org.junit.jupiter.api.Test; -import java.io.File; import java.util.HashSet; import java.util.Set; import java.util.UUID; -public class CreateTableTest { +public class CreateTableTest extends TestWithFeService { private static String runningDir = "fe/mocked/CreateTableTest2/" + UUID.randomUUID().toString() + "/"; - private static ConnectContext connectContext; - - @BeforeClass - public static void beforeClass() throws Exception { - Config.disable_storage_medium_check = true; - UtFrameUtils.createDorisClusterWithMultiTag(runningDir, 3); - - // create connect context - connectContext = UtFrameUtils.createDefaultCtx(); - // create database - String createDbStmtStr = "create database test;"; - CreateDbStmt createDbStmt = (CreateDbStmt) UtFrameUtils.parseAndAnalyzeStmt(createDbStmtStr, connectContext); - Env.getCurrentEnv().createDb(createDbStmt); + @Override + protected int backendNum() { + return 3; } - @AfterClass - public static void tearDown() { - File file = new File(runningDir); - file.delete(); - } - - private static void createTable(String sql) throws Exception { - CreateTableStmt createTableStmt = (CreateTableStmt) UtFrameUtils.parseAndAnalyzeStmt(sql, connectContext); - Env.getCurrentEnv().createTable(createTableStmt); - } - - private static void alterTable(String sql) throws Exception { - AlterTableStmt alterTableStmt = (AlterTableStmt) UtFrameUtils.parseAndAnalyzeStmt(sql, connectContext); - Env.getCurrentEnv().alterTable(alterTableStmt); + @Override + protected void runBeforeAll() throws Exception { + Config.allow_replica_on_same_host = true; + createDatabase("test"); } @Test @@ -761,7 +734,7 @@ public class CreateTableTest { // can still set replication_num manually. ExceptionChecker.expectThrowsWithMsg(UserException.class, "Failed to find enough host with tag", () -> { - alterTable("alter table test.test_replica modify partition p1 set ('replication_num' = '4')"); + alterTableSync("alter table test.test_replica modify partition p1 set ('replication_num' = '4')"); }); Database db = Env.getCurrentInternalCatalog().getDbOrDdlException("default_cluster:test"); @@ -792,17 +765,17 @@ public class CreateTableTest { Assert.assertEquals(2, (int) tbl1.getDefaultReplicaAllocation().getTotalReplicaNum()); ExceptionChecker.expectThrowsNoException( - () -> alterTable("alter table test.tbl_min_load_replica_num_1\n" + () -> alterTableSync("alter table test.tbl_min_load_replica_num_1\n" + " set ( 'min_load_replica_num' = '2');")); Assert.assertEquals(2, tbl1.getMinLoadReplicaNum()); ExceptionChecker.expectThrowsWithMsg(DdlException.class, "Failed to check min load replica num", - () -> alterTable("alter table test.tbl_min_load_replica_num_1\n" + () -> alterTableSync("alter table test.tbl_min_load_replica_num_1\n" + " set ( 'min_load_replica_num' = '3');")); Assert.assertEquals(2, tbl1.getMinLoadReplicaNum()); ExceptionChecker.expectThrowsWithMsg(DdlException.class, "min_load_replica_num should > 0 or =-1", - () -> alterTable("alter table test.tbl_min_load_replica_num_1\n" + () -> alterTableSync("alter table test.tbl_min_load_replica_num_1\n" + " set ( 'min_load_replica_num' = '-3');")); ExceptionChecker.expectThrowsWithMsg(DdlException.class, "Failed to check min load replica num", @@ -881,7 +854,7 @@ public class CreateTableTest { + ");\n")); ExceptionChecker.expectThrowsWithMsg(DdlException.class, "Failed to check min load replica num", - () -> alterTable("alter table test.tbl_min_load_replica_num_6\n" + () -> alterTableSync("alter table test.tbl_min_load_replica_num_6\n" + " set ( 'min_load_replica_num' = '3');")); ExceptionChecker.expectThrowsWithMsg(DdlException.class, "Failed to check min load replica num", @@ -905,4 +878,57 @@ public class CreateTableTest { + "\"dynamic_partition.start\" = \"-3\"\n" + ");\n")); } + + @Test + public void testCreateTableWithNerieds() throws Exception { + ExceptionChecker.expectThrowsWithMsg(org.apache.doris.nereids.exceptions.AnalysisException.class, + "Failed to check min load replica num", + () -> createTable("create table test.tbl_min_load_replica_num_2_nereids\n" + + "(k1 int, k2 int)\n" + + "duplicate key(k1)\n" + + "distributed by hash(k1) buckets 1\n" + + "properties(\n" + + " 'replication_num' = '2',\n" + + " 'min_load_replica_num' = '3'\n" + + ");", true)); + + ExceptionChecker.expectThrowsNoException( + () -> createTable("create table test.tbl_no_properties\n" + + "(k1 int, k2 int)\n" + + "duplicate key(k1)\n" + + "distributed by hash(k1) buckets 1", true)); + + ExceptionChecker.expectThrowsNoException( + () -> createTable("create table test.tbl_range_part_no_properties\n" + + "(k1 int not null, k2 int)\n" + + "partition by range(k1)\n" + + "(partition p1 values less than ('100')," + + "partition p2 values less than ('200'))\n" + + "distributed by hash(k1) buckets 1", true)); + + ExceptionChecker.expectThrowsNoException( + () -> createTable("create table test.tbl_in_part_no_properties\n" + + "(k1 int not null, k2 int)\n" + + "partition by list(k1)\n" + + "(partition p1 values in ('100')," + + "partition p2 values in ('200'))\n" + + "distributed by hash(k1) buckets 3", true)); + + ExceptionChecker.expectThrowsNoException( + () -> createTable("create table test.tbl_fixed_part_no_properties\n" + + "(k1 int not null, k2 int)\n" + + "partition by range(k1)\n" + + "(partition p1 values [('100'),('200'))," + + "partition p2 values [('200'),('300')))\n" + + "distributed by hash(k1) buckets 10", true)); + + createDatabaseWithSql("create database db2 properties('replication_num' = '4')"); + ExceptionChecker.expectThrowsWithMsg(org.apache.doris.nereids.exceptions.AnalysisException.class, + "replication num should be less than the number of available backends. " + + "replication num is 4, available backend num is 3", + () -> createTable("create table db2.tbl_4_replica\n" + + "(k1 int, k2 int)\n" + + "duplicate key(k1)\n" + + "distributed by hash(k1) buckets 1\n", true)); + } } diff --git a/fe/fe-core/src/test/java/org/apache/doris/common/ExceptionChecker.java b/fe/fe-core/src/test/java/org/apache/doris/common/ExceptionChecker.java index 26d5d1dc47..5353cfd027 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/common/ExceptionChecker.java +++ b/fe/fe-core/src/test/java/org/apache/doris/common/ExceptionChecker.java @@ -34,6 +34,7 @@ public class ExceptionChecker { try { runnable.run(); } catch (Throwable e) { + e.printStackTrace(); throw new AssertionFailedError(e.getMessage()); } } diff --git a/fe/fe-core/src/test/java/org/apache/doris/utframe/TestWithFeService.java b/fe/fe-core/src/test/java/org/apache/doris/utframe/TestWithFeService.java index 5237909dd4..160bdee462 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/utframe/TestWithFeService.java +++ b/fe/fe-core/src/test/java/org/apache/doris/utframe/TestWithFeService.java @@ -59,8 +59,10 @@ import org.apache.doris.common.util.SqlParserUtils; import org.apache.doris.nereids.CascadesContext; import org.apache.doris.nereids.StatementContext; import org.apache.doris.nereids.glue.LogicalPlanAdapter; +import org.apache.doris.nereids.parser.NereidsParser; import org.apache.doris.nereids.rules.RuleType; import org.apache.doris.nereids.trees.expressions.StatementScopeIdGenerator; +import org.apache.doris.nereids.trees.plans.commands.CreateTableCommand; import org.apache.doris.nereids.trees.plans.logical.LogicalPlan; import org.apache.doris.nereids.util.MemoTestUtils; import org.apache.doris.planner.Planner; @@ -101,7 +103,6 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; import java.util.Comparator; -import java.util.ConcurrentModificationException; import java.util.List; import java.util.Map; import java.util.Set; @@ -562,6 +563,11 @@ public abstract class TestWithFeService { Env.getCurrentEnv().createDb(createDbStmt); } + public void createDatabaseWithSql(String createDbSql) throws Exception { + CreateDbStmt createDbStmt = (CreateDbStmt) parseAndAnalyzeStmt(createDbSql); + Env.getCurrentEnv().createDb(createDbStmt); + } + public void dropDatabase(String db) throws Exception { String createDbStmtStr = "DROP DATABASE " + db; DropDbStmt createDbStmt = (DropDbStmt) parseAndAnalyzeStmt(createDbStmtStr); @@ -597,10 +603,14 @@ public abstract class TestWithFeService { } public void createTable(String sql) throws Exception { + createTable(sql, false); + } + + public void createTable(String sql, boolean enableNerieds) throws Exception { try { Config.enable_odbc_table = true; - createTables(sql); - } catch (ConcurrentModificationException e) { + createTables(enableNerieds, sql); + } catch (Exception e) { e.printStackTrace(); throw e; } @@ -624,9 +634,24 @@ public abstract class TestWithFeService { } public void createTables(String... sqls) throws Exception { - for (String sql : sqls) { - CreateTableStmt stmt = (CreateTableStmt) parseAndAnalyzeStmt(sql); - Env.getCurrentEnv().createTable(stmt); + createTables(false, sqls); + } + + public void createTables(boolean enableNereids, String... sqls) throws Exception { + if (enableNereids) { + for (String sql : sqls) { + NereidsParser nereidsParser = new NereidsParser(); + LogicalPlan parsed = nereidsParser.parseSingle(sql); + StmtExecutor stmtExecutor = new StmtExecutor(connectContext, sql); + if (parsed instanceof CreateTableCommand) { + ((CreateTableCommand) parsed).run(connectContext, stmtExecutor); + } + } + } else { + for (String sql : sqls) { + CreateTableStmt stmt = (CreateTableStmt) parseAndAnalyzeStmt(sql); + Env.getCurrentEnv().createTable(stmt); + } } updateReplicaPathHash(); }