From 53db9f042e8c122a01c974907e10fc25244b716b Mon Sep 17 00:00:00 2001 From: htyoung Date: Thu, 21 Dec 2023 09:57:20 +0800 Subject: [PATCH] [fix](stmt):fix CreateTableStmt toSql placed comment in wrong place (#27504) Issue Number: close #27474 Co-authored-by: tongyang.han --- .../doris/analysis/CreateTableStmt.java | 159 +++++++----------- .../doris/analysis/CreateTableStmtTest.java | 62 +++++++ 2 files changed, 123 insertions(+), 98 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 3bd12b64eefb984..210fa60857fc1b1 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 @@ -85,7 +85,6 @@ public class CreateTableStmt extends DdlStmt { // set in analyze private List columns = Lists.newArrayList(); - private List indexes = Lists.newArrayList(); static { @@ -130,48 +129,26 @@ public CreateTableStmt() { columnDefs = Lists.newArrayList(); } - public CreateTableStmt(boolean ifNotExists, - boolean isExternal, - TableName tableName, - List columnDefinitions, - String engineName, - KeysDesc keysDesc, - PartitionDesc partitionDesc, - DistributionDesc distributionDesc, - Map properties, - Map extProperties, + public CreateTableStmt(boolean ifNotExists, boolean isExternal, TableName tableName, + List columnDefinitions, String engineName, KeysDesc keysDesc, PartitionDesc partitionDesc, + DistributionDesc distributionDesc, Map properties, Map extProperties, String comment) { this(ifNotExists, isExternal, tableName, columnDefinitions, null, engineName, keysDesc, partitionDesc, distributionDesc, properties, extProperties, comment, null); } - public CreateTableStmt(boolean ifNotExists, - boolean isExternal, - TableName tableName, - List columnDefinitions, - String engineName, - KeysDesc keysDesc, - PartitionDesc partitionDesc, - DistributionDesc distributionDesc, - Map properties, - Map extProperties, + public CreateTableStmt(boolean ifNotExists, boolean isExternal, TableName tableName, + List columnDefinitions, String engineName, KeysDesc keysDesc, PartitionDesc partitionDesc, + DistributionDesc distributionDesc, Map properties, Map extProperties, String comment, List ops) { this(ifNotExists, isExternal, tableName, columnDefinitions, null, engineName, keysDesc, partitionDesc, distributionDesc, properties, extProperties, comment, ops); } - public CreateTableStmt(boolean ifNotExists, - boolean isExternal, - TableName tableName, - List columnDefinitions, - List indexDefs, - String engineName, - KeysDesc keysDesc, - PartitionDesc partitionDesc, - DistributionDesc distributionDesc, - Map properties, - Map extProperties, - String comment, List rollupAlterClauseList) { + public CreateTableStmt(boolean ifNotExists, boolean isExternal, TableName tableName, + List columnDefinitions, List indexDefs, String engineName, KeysDesc keysDesc, + PartitionDesc partitionDesc, DistributionDesc distributionDesc, Map properties, + Map extProperties, String comment, List rollupAlterClauseList) { this.tableName = tableName; if (columnDefinitions == null) { this.columnDefs = Lists.newArrayList(); @@ -198,20 +175,10 @@ public CreateTableStmt(boolean ifNotExists, } // for Nereids - public CreateTableStmt(boolean ifNotExists, - boolean isExternal, - TableName tableName, - List columns, - List indexes, - String engineName, - KeysDesc keysDesc, - PartitionDesc partitionDesc, - DistributionDesc distributionDesc, - Map properties, - Map extProperties, - String comment, - List rollupAlterClauseList, - Void unused) { + public CreateTableStmt(boolean ifNotExists, boolean isExternal, TableName tableName, List columns, + List indexes, String engineName, KeysDesc keysDesc, PartitionDesc partitionDesc, + DistributionDesc distributionDesc, Map properties, Map extProperties, + String comment, List rollupAlterClauseList, Void unused) { this.ifNotExists = ifNotExists; this.isExternal = isExternal; this.tableName = tableName; @@ -308,8 +275,8 @@ public List getIndexes() { } @Override - public void analyze(Analyzer analyzer) throws UserException, AnalysisException { - if (Strings.isNullOrEmpty(engineName) || engineName.equalsIgnoreCase("olap")) { + public void analyze(Analyzer analyzer) throws UserException { + if (Strings.isNullOrEmpty(engineName) || engineName.equalsIgnoreCase(DEFAULT_ENGINE_NAME)) { this.properties = maybeRewriteByAutoBucket(distributionDesc, properties); } @@ -319,8 +286,8 @@ public void analyze(Analyzer analyzer) throws UserException, AnalysisException { // disallow external catalog Util.prohibitExternalCatalog(tableName.getCtl(), this.getClass().getSimpleName()); - if (!Env.getCurrentEnv().getAccessManager().checkTblPriv(ConnectContext.get(), tableName.getDb(), - tableName.getTbl(), PrivPredicate.CREATE)) { + if (!Env.getCurrentEnv().getAccessManager() + .checkTblPriv(ConnectContext.get(), tableName.getDb(), tableName.getTbl(), PrivPredicate.CREATE)) { ErrorReport.reportAnalysisException(ErrorCode.ERR_SPECIFIC_ACCESS_DENIED_ERROR, "CREATE"); } @@ -328,10 +295,10 @@ public void analyze(Analyzer analyzer) throws UserException, AnalysisException { boolean enableDuplicateWithoutKeysByDefault = false; if (properties != null) { - enableDuplicateWithoutKeysByDefault = - PropertyAnalyzer.analyzeEnableDuplicateWithoutKeysByDefault(properties); + enableDuplicateWithoutKeysByDefault = PropertyAnalyzer.analyzeEnableDuplicateWithoutKeysByDefault( + properties); } - //pre-block creation with column type ALL + // pre-block creation with column type ALL for (ColumnDef columnDef : columnDefs) { if (Objects.equals(columnDef.getType(), Type.ALL)) { throw new AnalysisException("Disable to create table with `ALL` type columns."); @@ -340,14 +307,14 @@ public void analyze(Analyzer analyzer) throws UserException, AnalysisException { throw new AnalysisException("Disable to create table with `DATE` type columns, please use `DATEV2`."); } if (Objects.equals(columnDef.getType(), Type.DECIMALV2) && Config.disable_decimalv2) { - throw new AnalysisException("Disable to create table with `DECIMAL` type columns," - + "please use `DECIMALV3`."); + throw new AnalysisException( + "Disable to create table with `DECIMAL` type columns," + "please use `DECIMALV3`."); } } boolean enableUniqueKeyMergeOnWrite = false; // analyze key desc - if (engineName.equalsIgnoreCase("olap")) { + if (engineName.equalsIgnoreCase(DEFAULT_ENGINE_NAME)) { // olap table if (keysDesc == null) { List keysColumnNames = Lists.newArrayList(); @@ -361,9 +328,9 @@ public void analyze(Analyzer analyzer) throws UserException, AnalysisException { } if (hasAggregate) { for (ColumnDef columnDef : columnDefs) { - if (columnDef.getAggregateType() == null - && !columnDef.getType().isScalarType(PrimitiveType.STRING) - && !columnDef.getType().isScalarType(PrimitiveType.JSONB)) { + if (columnDef.getAggregateType() == null && !columnDef.getType() + .isScalarType(PrimitiveType.STRING) && !columnDef.getType() + .isScalarType(PrimitiveType.JSONB)) { keysColumnNames.add(columnDef.getName()); } } @@ -374,8 +341,8 @@ public void analyze(Analyzer analyzer) throws UserException, AnalysisException { keyLength += columnDef.getType().getIndexSize(); if (keysColumnNames.size() >= FeConstants.shortkey_max_column_count || keyLength > FeConstants.shortkey_maxsize_bytes) { - if (keysColumnNames.size() == 0 - && columnDef.getType().getPrimitiveType().isCharFamily()) { + if (keysColumnNames.isEmpty() && columnDef.getType().getPrimitiveType() + .isCharFamily()) { keysColumnNames.add(columnDef.getName()); } break; @@ -411,7 +378,7 @@ public void analyze(Analyzer analyzer) throws UserException, AnalysisException { } else { if (enableDuplicateWithoutKeysByDefault) { throw new AnalysisException("table property 'enable_duplicate_without_keys_by_default' only can" - + " set 'true' when create olap table by default."); + + " set 'true' when create olap table by default."); } } @@ -463,13 +430,11 @@ public void analyze(Analyzer analyzer) throws UserException, AnalysisException { } // analyze column def - if (!(engineName.equals("elasticsearch")) - && (columnDefs == null || columnDefs.isEmpty())) { + if (!(engineName.equalsIgnoreCase("elasticsearch")) && (columnDefs == null || columnDefs.isEmpty())) { ErrorReport.reportAnalysisException(ErrorCode.ERR_TABLE_MUST_HAVE_COLUMNS); } // add a hidden column as delete flag for unique table - if (Config.enable_batch_delete_by_default - && keysDesc != null + if (Config.enable_batch_delete_by_default && keysDesc != null && keysDesc.getKeysType() == KeysType.UNIQUE_KEYS) { if (enableUniqueKeyMergeOnWrite) { columnDefs.add(ColumnDef.newDeleteSignColumnDef(AggregateType.NONE)); @@ -502,14 +467,14 @@ public void analyze(Analyzer analyzer) throws UserException, AnalysisException { } Set columnSet = Sets.newTreeSet(String.CASE_INSENSITIVE_ORDER); for (ColumnDef columnDef : columnDefs) { - columnDef.analyze(engineName.equals("olap")); + columnDef.analyze(engineName.equalsIgnoreCase(DEFAULT_ENGINE_NAME)); - if (columnDef.getType().isComplexType() && engineName.equals("olap")) { - if (columnDef.getAggregateType() != null - && columnDef.getAggregateType() != AggregateType.NONE + if (columnDef.getType().isComplexType() && engineName.equalsIgnoreCase(DEFAULT_ENGINE_NAME)) { + if (columnDef.getAggregateType() != null && columnDef.getAggregateType() != AggregateType.NONE && columnDef.getAggregateType() != AggregateType.REPLACE) { - throw new AnalysisException(columnDef.getType().getPrimitiveType() - + " column can't support aggregation " + columnDef.getAggregateType()); + throw new AnalysisException( + columnDef.getType().getPrimitiveType() + " column can't support aggregation " + + columnDef.getAggregateType()); } if (columnDef.isKey()) { throw new AnalysisException(columnDef.getType().getPrimitiveType() @@ -526,17 +491,17 @@ public void analyze(Analyzer analyzer) throws UserException, AnalysisException { } } - if (engineName.equals("olap")) { + if (engineName.equalsIgnoreCase(DEFAULT_ENGINE_NAME)) { // before analyzing partition, handle the replication allocation info - properties = PropertyAnalyzer.rewriteReplicaAllocationProperties( - tableName.getCtl(), tableName.getDb(), properties); + properties = PropertyAnalyzer.rewriteReplicaAllocationProperties(tableName.getCtl(), tableName.getDb(), + properties); // analyze partition if (partitionDesc != null) { if (partitionDesc instanceof ListPartitionDesc || partitionDesc instanceof RangePartitionDesc) { partitionDesc.analyze(columnDefs, properties); } else { - throw new AnalysisException("Currently only support range" - + " and list partition with engine type olap"); + throw new AnalysisException( + "Currently only support range" + " and list partition with engine type olap"); } } @@ -553,10 +518,10 @@ public void analyze(Analyzer analyzer) throws UserException, AnalysisException { for (ColumnDef columnDef : columnDefs) { if (columnDef.getAggregateType() == AggregateType.REPLACE || columnDef.getAggregateType() == AggregateType.REPLACE_IF_NOT_NULL) { - throw new AnalysisException("Create aggregate keys table with value columns of which" - + " aggregate type is " + columnDef.getAggregateType() - + " should not contain random" - + " distribution desc"); + throw new AnalysisException( + "Create aggregate keys table with value columns of which" + " aggregate type is " + + columnDef.getAggregateType() + " should not contain random" + + " distribution desc"); } } } @@ -565,8 +530,8 @@ public void analyze(Analyzer analyzer) throws UserException, AnalysisException { EsUtil.analyzePartitionAndDistributionDesc(partitionDesc, distributionDesc); } else { if (partitionDesc != null || distributionDesc != null) { - throw new AnalysisException("Create " + engineName - + " table should not contain partition or distribution desc"); + throw new AnalysisException( + "Create " + engineName + " table should not contain partition or distribution desc"); } } @@ -586,7 +551,7 @@ public void analyze(Analyzer analyzer) throws UserException, AnalysisException { for (IndexDef indexDef : indexDefs) { indexDef.analyze(); - if (!engineName.equalsIgnoreCase("olap")) { + if (!engineName.equalsIgnoreCase(DEFAULT_ENGINE_NAME)) { throw new AnalysisException("index only support in olap engine at current version."); } for (String indexColName : indexDef.getColumns()) { @@ -599,13 +564,11 @@ public void analyze(Analyzer analyzer) throws UserException, AnalysisException { } } if (!found) { - throw new AnalysisException("Column does not exist in table. invalid column: " - + indexColName); + throw new AnalysisException("Column does not exist in table. invalid column: " + indexColName); } } - indexes.add(new Index(Env.getCurrentEnv().getNextId(), indexDef.getIndexName(), - indexDef.getColumns(), indexDef.getIndexType(), - indexDef.getProperties(), indexDef.getComment())); + indexes.add(new Index(Env.getCurrentEnv().getNextId(), indexDef.getIndexName(), indexDef.getColumns(), + indexDef.getIndexType(), indexDef.getProperties(), indexDef.getComment())); distinct.add(indexDef.getIndexName()); distinctCol.add(Pair.of(indexDef.getIndexType(), indexDef.getColumns().stream().map(String::toUpperCase).collect(Collectors.toList()))); @@ -687,12 +650,16 @@ public String toSql() { } } sb.append("\n)"); - sb.append(" ENGINE = ").append(engineName); + sb.append(" ENGINE = ").append(engineName.toLowerCase()); if (keysDesc != null) { sb.append("\n").append(keysDesc.toSql()); } + if (!Strings.isNullOrEmpty(comment)) { + sb.append("\nCOMMENT \"").append(comment).append("\""); + } + if (partitionDesc != null) { sb.append("\n").append(partitionDesc.toSql()); } @@ -701,7 +668,7 @@ public String toSql() { sb.append("\n").append(distributionDesc.toSql()); } - if (rollupAlterClauseList != null && rollupAlterClauseList.size() != 0) { + if (rollupAlterClauseList != null && !rollupAlterClauseList.isEmpty()) { sb.append("\n rollup("); StringBuilder opsSb = new StringBuilder(); for (int i = 0; i < rollupAlterClauseList.size(); i++) { @@ -719,20 +686,16 @@ public String toSql() { // which is implemented in Catalog.getDdlStmt() if (properties != null && !properties.isEmpty()) { sb.append("\nPROPERTIES ("); - sb.append(new PrintableMap(properties, " = ", true, true, true)); + sb.append(new PrintableMap<>(properties, " = ", true, true, true)); sb.append(")"); } if (extProperties != null && !extProperties.isEmpty()) { sb.append("\n").append(engineName.toUpperCase()).append(" PROPERTIES ("); - sb.append(new PrintableMap(extProperties, " = ", true, true, true)); + sb.append(new PrintableMap<>(extProperties, " = ", true, true, true)); sb.append(")"); } - if (!Strings.isNullOrEmpty(comment)) { - sb.append("\nCOMMENT \"").append(comment).append("\""); - } - return sb.toString(); } diff --git a/fe/fe-core/src/test/java/org/apache/doris/analysis/CreateTableStmtTest.java b/fe/fe-core/src/test/java/org/apache/doris/analysis/CreateTableStmtTest.java index 30da40339763012..ccd865dd6d4493e 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/analysis/CreateTableStmtTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/analysis/CreateTableStmtTest.java @@ -456,4 +456,66 @@ public void testToSql() { Assert.assertEquals(createTableStmt.toSql(), createTableSql); } + + @Test + public void testToSqlWithComment() { + List columnDefs = new ArrayList<>(); + columnDefs.add(new ColumnDef("a", TypeDef.create(PrimitiveType.BIGINT), false)); + columnDefs.add(new ColumnDef("b", TypeDef.create(PrimitiveType.INT), false)); + String engineName = "olap"; + ArrayList aggKeys = Lists.newArrayList("a"); + KeysDesc keysDesc = new KeysDesc(KeysType.AGG_KEYS, aggKeys); + Map properties = new HashMap() { + { + put("replication_num", String.valueOf(Math.max(1, + Config.min_replication_num_per_tablet))); + } + }; + TableName tableName = new TableName("internal", "demo", "testToSqlWithComment1"); + CreateTableStmt createTableStmt = new CreateTableStmt(true, false, + tableName, columnDefs, engineName, keysDesc, null, null, + properties, null, "xxx", null); + String createTableSql = "CREATE TABLE IF NOT EXISTS `demo`.`testToSqlWithComment1` (\n" + + " `a` BIGINT NOT NULL COMMENT \"\",\n" + + " `b` INT NOT NULL COMMENT \"\"\n" + + ") ENGINE = olap\n" + + "AGGREGATE KEY(`a`)\n" + + "COMMENT \"xxx\"\n" + + "PROPERTIES (\"replication_num\" = \"1\")"; + Assert.assertEquals(createTableStmt.toSql(), createTableSql); + + + columnDefs.add(new ColumnDef("c", TypeDef.create(PrimitiveType.STRING), true)); + columnDefs.add(new ColumnDef("d", TypeDef.create(PrimitiveType.DOUBLE), true)); + columnDefs.add(new ColumnDef("e", TypeDef.create(PrimitiveType.DECIMAL128), false)); + columnDefs.add(new ColumnDef("f", TypeDef.create(PrimitiveType.DATE), false)); + columnDefs.add(new ColumnDef("g", TypeDef.create(PrimitiveType.SMALLINT), false)); + columnDefs.add(new ColumnDef("h", TypeDef.create(PrimitiveType.BOOLEAN), false)); + aggKeys = Lists.newArrayList("a", "d", "f"); + keysDesc = new KeysDesc(KeysType.DUP_KEYS, aggKeys); + properties = new HashMap() { + { + put("replication_num", String.valueOf(Math.max(10, + Config.min_replication_num_per_tablet))); + } + }; + tableName = new TableName("internal", "demo", "testToSqlWithComment2"); + createTableStmt = new CreateTableStmt(false, false, + tableName, columnDefs, engineName, keysDesc, null, null, + properties, null, "xxx", null); + createTableSql = "CREATE TABLE `demo`.`testToSqlWithComment2` (\n" + + " `a` BIGINT NOT NULL COMMENT \"\",\n" + + " `b` INT NOT NULL COMMENT \"\",\n" + + " `c` TEXT NULL COMMENT \"\",\n" + + " `d` DOUBLE NULL COMMENT \"\",\n" + + " `e` DECIMALV3(38, 0) NOT NULL COMMENT \"\",\n" + + " `f` DATE NOT NULL COMMENT \"\",\n" + + " `g` SMALLINT NOT NULL COMMENT \"\",\n" + + " `h` BOOLEAN NOT NULL COMMENT \"\"\n" + + ") ENGINE = olap\n" + + "DUPLICATE KEY(`a`, `d`, `f`)\n" + + "COMMENT \"xxx\"\n" + + "PROPERTIES (\"replication_num\" = \"10\")"; + Assert.assertEquals(createTableStmt.toSql(), createTableSql); + } }