diff --git a/be/src/information_schema/schema_columns_scanner.cpp b/be/src/information_schema/schema_columns_scanner.cpp index 083736bb3dcd90..2d4f3750ff2840 100644 --- a/be/src/information_schema/schema_columns_scanner.cpp +++ b/be/src/information_schema/schema_columns_scanner.cpp @@ -174,6 +174,29 @@ std::string SchemaColumnsScanner::_to_mysql_data_type_string(TColumnDesc& desc) } } +namespace { + +// A struct field name goes into COLUMN_TYPE next to the separators that give the text its +// shape. A name holding one of them would make the text impossible to split back into +// fields, so wrap such a name in back quotes and double any back quote inside it. +std::string quote_struct_field_name(const std::string& name) { + if (name.find_first_of(",:<>`") == std::string::npos) { + return name; + } + std::string quoted = "`"; + for (char ch : name) { + if (ch == '`') { + quoted += "``"; + } else { + quoted += ch; + } + } + quoted += "`"; + return quoted; +} + +} // namespace + std::string SchemaColumnsScanner::_type_to_string(TColumnDesc& desc) { switch (desc.columnType) { case TPrimitiveType::BOOLEAN: @@ -284,11 +307,13 @@ std::string SchemaColumnsScanner::_type_to_string(TColumnDesc& desc) { case TPrimitiveType::STRUCT: { // for old be service we should compitable std::string ret = "struct<"; - if (!desc.children.empty()) { - for (int i = 0; i < desc.children.size() - 1; ++i) { - ret += _type_to_string(desc.children[i]) + ","; + // Name every field, a client rebuilds the schema from this text. + for (size_t i = 0; i < desc.children.size(); ++i) { + if (i != 0) { + ret += ","; } - ret += _type_to_string(desc.children[desc.children.size() - 1]); + ret += quote_struct_field_name(desc.children[i].columnName) + ":" + + _type_to_string(desc.children[i]); } ret += ">"; return ret; diff --git a/be/test/exec/schema_scanner/schema_columns_scanner_test.cpp b/be/test/exec/schema_scanner/schema_columns_scanner_test.cpp new file mode 100644 index 00000000000000..69f519b02b1d80 --- /dev/null +++ b/be/test/exec/schema_scanner/schema_columns_scanner_test.cpp @@ -0,0 +1,111 @@ +// 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. + +#include "information_schema/schema_columns_scanner.h" + +#include +#include +#include + +#include + +namespace doris { + +namespace { + +TColumnDesc make_desc(const std::string& name, TPrimitiveType::type type) { + TColumnDesc desc; + desc.__set_columnName(name); + desc.__set_columnType(type); + return desc; +} + +} // namespace + +// COLUMN_TYPE is the text information_schema clients read a schema back from, so a +// STRUCT has to name its fields there. The names arrive lower cased from the FE, while +// SHOW CREATE TABLE keeps the spelling the user wrote. +class SchemaColumnsScannerTest : public testing::Test { +protected: + std::string type_to_string(TColumnDesc& desc) { return _scanner._type_to_string(desc); } + +private: + SchemaColumnsScanner _scanner; +}; + +TEST_F(SchemaColumnsScannerTest, struct_type_string_carries_field_names) { + TColumnDesc desc = make_desc("st", TPrimitiveType::STRUCT); + desc.__set_children( + {make_desc("f1", TPrimitiveType::INT), make_desc("f2", TPrimitiveType::STRING)}); + + EXPECT_EQ("struct", type_to_string(desc)); +} + +TEST_F(SchemaColumnsScannerTest, nested_struct_type_string_carries_field_names) { + TColumnDesc element = make_desc("item", TPrimitiveType::STRUCT); + element.__set_children({make_desc("deep", TPrimitiveType::INT)}); + + // An array element has no name of its own, only the struct fields are named. + TColumnDesc arr = make_desc("arr", TPrimitiveType::ARRAY); + arr.__set_children({element}); + + TColumnDesc outer = make_desc("outer", TPrimitiveType::STRUCT); + outer.__set_children({arr}); + + EXPECT_EQ("struct>>", type_to_string(outer)); +} + +TEST_F(SchemaColumnsScannerTest, struct_field_name_holding_a_separator_is_quoted) { + // These names are legal, they are declared with back quotes. Printed raw they would make + // the text impossible to split back into fields. + TColumnDesc desc = make_desc("st", TPrimitiveType::STRUCT); + desc.__set_children( + {make_desc("a,b", TPrimitiveType::INT), make_desc("c:d", TPrimitiveType::INT), + make_desc("e", TPrimitiveType::INT), make_desc("g`h", TPrimitiveType::INT)}); + + EXPECT_EQ("struct<`a,b`:int(11),`c:d`:int(11),`e`:int(11),`g``h`:int(11)>", + type_to_string(desc)); +} + +TEST_F(SchemaColumnsScannerTest, plain_struct_field_name_stays_bare) { + TColumnDesc desc = make_desc("st", TPrimitiveType::STRUCT); + desc.__set_children({make_desc("a-b", TPrimitiveType::INT), + make_desc("f 2", TPrimitiveType::INT), + make_desc("order", TPrimitiveType::INT)}); + + // A hyphen, a space or a keyword carries no meaning in this text, so leave them alone. + EXPECT_EQ("struct", type_to_string(desc)); +} + +TEST_F(SchemaColumnsScannerTest, empty_struct_type_string) { + TColumnDesc desc = make_desc("st", TPrimitiveType::STRUCT); + EXPECT_EQ("struct<>", type_to_string(desc)); +} + +// Array and map keep printing types only, they have no field names to carry. +TEST_F(SchemaColumnsScannerTest, array_and_map_type_string_unchanged) { + TColumnDesc arr = make_desc("arr", TPrimitiveType::ARRAY); + arr.__set_children({make_desc("item", TPrimitiveType::INT)}); + EXPECT_EQ("array", type_to_string(arr)); + + TColumnDesc mp = make_desc("mp", TPrimitiveType::MAP); + mp.__set_children( + {make_desc("key", TPrimitiveType::STRING), make_desc("value", TPrimitiveType::INT)}); + EXPECT_EQ("map", type_to_string(mp)); +} + +} // namespace doris diff --git a/fe/fe-catalog/src/main/java/org/apache/doris/catalog/Column.java b/fe/fe-catalog/src/main/java/org/apache/doris/catalog/Column.java index eefee5b729cbf6..08cdd051189264 100644 --- a/fe/fe-catalog/src/main/java/org/apache/doris/catalog/Column.java +++ b/fe/fe-catalog/src/main/java/org/apache/doris/catalog/Column.java @@ -884,18 +884,25 @@ public String toSql(boolean isUniqueTable) { } public String toSql(boolean isUniqueTable, boolean isCompatible) { - return toSql(isUniqueTable, isCompatible, false); + return toSql(isUniqueTable, isCompatible, false, false); } - /** Use a placeholder only for internal CREATE TABLE LIKE parsing; restore the expression before analysis. */ - public String toSql(boolean isUniqueTable, boolean isCompatible, boolean useGeneratedColumnPlaceholder) { + /** + * Use a placeholder only for internal CREATE TABLE LIKE parsing; restore the expression before analysis. + * Pass the SQL mode the statement will be parsed under, a comment is escaped differently under + * NO_BACKSLASH_ESCAPES. + */ + public String toSql(boolean isUniqueTable, boolean isCompatible, boolean useGeneratedColumnPlaceholder, + boolean noBackslashEscapes) { StringBuilder sb = new StringBuilder(); sb.append("`").append(name).append("` "); String typeStr = type.toSql(); // show change datetimeV2/dateV2 to datetime/date if (isCompatible) { - sb.append(type.hideVersionForVersionColumn(true)); + // isToSql = true, showNestedComment = true + // SHOW CREATE TABLE and CREATE TABLE LIKE need the nested comment. + sb.append(type.hideVersionForVersionColumn(true, true, noBackslashEscapes)); } else { sb.append(typeStr); } diff --git a/fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java b/fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java index 63c3bf15ec3aea..9797e574614c76 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java +++ b/fe/fe-core/src/main/java/org/apache/doris/catalog/Env.java @@ -4330,6 +4330,13 @@ public static void getCreateTableLikeStmt(CreateTableLikeInfo createTableLikeInf sb.append(" (\n"); int idx = 0; List columns = table.getBaseSchema(false); + // The emitted statement is parsed again by CREATE TABLE LIKE and by anyone replaying + // SHOW CREATE TABLE, so escape comments for the mode that parse will run under. Read it + // once here: a synced statement is built on the journal replay thread, which has no + // session, and reading the mode per column would clone the whole SessionVariable each time. + // Synced DDL travels to another cluster, so pin it to the default mode instead, otherwise + // the master and a replaying follower would ship different text for the same table. + boolean noBackslashEscapes = !getDdlForSync && SqlModeHelper.hasNoBackSlashEscapes(); for (Column column : columns) { if (idx++ != 0) { sb.append(",\n"); @@ -4338,7 +4345,8 @@ public static void getCreateTableLikeStmt(CreateTableLikeInfo createTableLikeInf // sqlalchemy requires this to parse SHOW CREATE TABLE stmt. if (table.isManagedTable()) { sb.append(" ").append( - column.toSql(((OlapTable) table).getKeysType() == KeysType.UNIQUE_KEYS, true, true)); + column.toSql(((OlapTable) table).getKeysType() == KeysType.UNIQUE_KEYS, true, true, + noBackslashEscapes)); } else { sb.append(" ").append(column.toSql()); } @@ -4684,6 +4692,13 @@ public static void getDdlStmt(Command command, String dbName, TableIf table, Lis sb.append(" (\n"); int idx = 0; List columns = table.getBaseSchema(false); + // The emitted statement is parsed again by CREATE TABLE LIKE and by anyone replaying + // SHOW CREATE TABLE, so escape comments for the mode that parse will run under. Read it + // once here: a synced statement is built on the journal replay thread, which has no + // session, and reading the mode per column would clone the whole SessionVariable each time. + // Synced DDL travels to another cluster, so pin it to the default mode instead, otherwise + // the master and a replaying follower would ship different text for the same table. + boolean noBackslashEscapes = !getDdlForSync && SqlModeHelper.hasNoBackSlashEscapes(); for (Column column : columns) { if (idx++ != 0) { sb.append(",\n"); @@ -4692,7 +4707,8 @@ public static void getDdlStmt(Command command, String dbName, TableIf table, Lis // sqlalchemy requires this to parse SHOW CREATE TABLE stmt. if (table.isManagedTable()) { sb.append(" ").append( - column.toSql(((OlapTable) table).getKeysType() == KeysType.UNIQUE_KEYS, true)); + column.toSql(((OlapTable) table).getKeysType() == KeysType.UNIQUE_KEYS, true, false, + noBackslashEscapes)); } else { sb.append(" ").append(column.toSql()); } diff --git a/fe/fe-core/src/test/java/org/apache/doris/catalog/TypeTest.java b/fe/fe-core/src/test/java/org/apache/doris/catalog/TypeTest.java index 2a8f3f299eb0fc..49e2192ff1889a 100644 --- a/fe/fe-core/src/test/java/org/apache/doris/catalog/TypeTest.java +++ b/fe/fe-core/src/test/java/org/apache/doris/catalog/TypeTest.java @@ -282,4 +282,56 @@ public void testDatetimeV2ScaleMatching() { Assertions.assertFalse(dtv2s6.matchesType(timestampNs)); Assertions.assertFalse(Type.matchExactType(timestampNs, wildcardDatetimeV2, false)); } + + // ===================== STRUCT nested comment in DDL ===================== + + /** SHOW CREATE TABLE has to print the comment of every nested field. */ + @Test + public void testStructDdlKeepsNestedFieldComment() { + StructType structType = new StructType( + new StructField("a", Type.INT, null, true), + new StructField("b", Type.STRING, "field doc", true)); + Column column = new Column("s", structType, false, null, true, null, "top-level"); + + Assertions.assertEquals( + "`s` struct NULL COMMENT \"top-level\"", + column.toSql(false, true)); + } + + /** A nested comment has to stay a readable literal, quotes and back slashes included. */ + @Test + public void testStructDdlEscapesNestedFieldComment() { + StructType structType = new StructType( + new StructField("b", Type.STRING, "owner''s \\path and \"quotes\"", true)); + Column column = new Column("s", structType, false, null, true, null, null); + + Assertions.assertEquals( + "`s` struct NULL", + column.toSql(false, true)); + } + + /** The comment has to be escaped for the SQL mode the statement will be parsed under. */ + @Test + public void testStructDdlFollowsNoBackslashEscapesMode() { + StructType structType = new StructType( + new StructField("b", Type.STRING, "C:\\tmp", true)); + Column column = new Column("s", structType, false, null, true, null, null); + + // Default mode reads a doubled back slash as one, NO_BACKSLASH_ESCAPES reads it as two. + Assertions.assertEquals("`s` struct NULL", + column.toSql(false, true, false, false)); + Assertions.assertEquals("`s` struct NULL", + column.toSql(false, true, false, true)); + } + + /** Nested comments belong to DDL, but DESCRIBE only prints them when asked for. */ + @Test + public void testStructDescribeHidesNestedCommentUnlessRequested() { + StructType structType = new StructType( + new StructField("b", Type.STRING, "field doc", true)); + Assertions.assertEquals("struct", + structType.hideVersionForVersionColumn(true, false, false)); + Assertions.assertEquals("struct", + structType.hideVersionForVersionColumn(true, true, false)); + } } diff --git a/regression-test/data/datatype_p0/nested_types/ddl/test_struct_nested_comment_ddl.out b/regression-test/data/datatype_p0/nested_types/ddl/test_struct_nested_comment_ddl.out new file mode 100644 index 00000000000000..6c69214f997f51 --- /dev/null +++ b/regression-test/data/datatype_p0/nested_types/ddl/test_struct_nested_comment_ddl.out @@ -0,0 +1,14 @@ +-- This file is automatically generated. You should know what you did if you want to edit this +-- !show_create -- +struct_nested_comment CREATE TABLE `struct_nested_comment` (\n `id` int NULL,\n `s` struct NULL COMMENT "top-level",\n `n` struct> NULL,\n `arr` array> NULL\n) ENGINE=OLAP\nDUPLICATE KEY(`id`)\nDISTRIBUTED BY HASH(`id`) BUCKETS 1\nPROPERTIES (\n"replication_allocation" = "tag.location.default: 1",\n"min_load_replica_num" = "-1",\n"is_being_synced" = "false",\n"storage_medium" = "hdd",\n"storage_format" = "V2",\n"inverted_index_storage_format" = "V3",\n"light_schema_change" = "true",\n"disable_auto_compaction" = "false",\n"group_commit_interval_ms" = "10000",\n"group_commit_data_bytes" = "134217728"\n); + +-- !column_type -- +arr array array> +id int int(11) +n struct struct> +s struct struct + +-- !column_type_quoting -- +id int(11) +s struct<`a,b`:int(11),`c:d`:int(11),`e`:int(11),plain:int(11)> + diff --git a/regression-test/data/datatype_p0/nested_types/meta/test_complextype_information_schema.out b/regression-test/data/datatype_p0/nested_types/meta/test_complextype_information_schema.out index 4a6f66c4e29e3c..18d1d45a5b36db 100644 --- a/regression-test/data/datatype_p0/nested_types/meta/test_complextype_information_schema.out +++ b/regression-test/data/datatype_p0/nested_types/meta/test_complextype_information_schema.out @@ -41,21 +41,21 @@ map map -- !sql -- bigint bigint(20) -struct struct -struct struct -struct struct -struct struct -struct struct -struct struct -struct struct -struct struct -struct struct -struct struct -struct struct -struct struct -struct struct -struct struct -struct struct -struct struct -struct struct +struct struct +struct struct +struct struct +struct struct +struct struct +struct struct +struct struct +struct struct +struct struct +struct struct +struct struct +struct struct +struct struct +struct struct +struct struct +struct struct +struct struct diff --git a/regression-test/data/external_table_p0/iceberg/test_iceberg_nested_schema_evolution_ddl.out b/regression-test/data/external_table_p0/iceberg/test_iceberg_nested_schema_evolution_ddl.out index 93826ea80b7925..74eb938f555471 100644 --- a/regression-test/data/external_table_p0/iceberg/test_iceberg_nested_schema_evolution_ddl.out +++ b/regression-test/data/external_table_p0/iceberg/test_iceberg_nested_schema_evolution_ddl.out @@ -1,12 +1,13 @@ -- This file is automatically generated. You should know what you did if you want to edit this -- !desc -- id int(11) -s struct -arr array> -m map> +s struct +arr array> +m map> arr_scalar array m_scalar map -- !query_rows -- 1 \N 10 \N \N 100 \N \N 1000 \N \N 7 70 2 first 20 after_a c2 200 202 201 2000 2002 2001 8 80 + diff --git a/regression-test/data/external_table_p0/iceberg/test_iceberg_nested_schema_evolution_spark_doris_interop.out b/regression-test/data/external_table_p0/iceberg/test_iceberg_nested_schema_evolution_spark_doris_interop.out index 54a02e79582045..df27c2a4e7c331 100644 --- a/regression-test/data/external_table_p0/iceberg/test_iceberg_nested_schema_evolution_spark_doris_interop.out +++ b/regression-test/data/external_table_p0/iceberg/test_iceberg_nested_schema_evolution_spark_doris_interop.out @@ -5,9 +5,9 @@ -- !spark_driven_schema -- id int(11) -info struct -events array> -attrs map> +info struct +events array> +attrs map> -- !spark_driven_rows_before_write -- 1 \N 10 \N spark_before \N \N 100 \N \N \N 1000 \N \N @@ -25,3 +25,4 @@ attrs map> -- !deep_nested_rows -- 1 last-old middle-old first-old old-note 1.5 12.34 2 last-new middle-new first-new new-note 2.5 56.78 + diff --git a/regression-test/suites/datatype_p0/nested_types/ddl/test_struct_nested_comment_ddl.groovy b/regression-test/suites/datatype_p0/nested_types/ddl/test_struct_nested_comment_ddl.groovy new file mode 100644 index 00000000000000..4f8904b5a26450 --- /dev/null +++ b/regression-test/suites/datatype_p0/nested_types/ddl/test_struct_nested_comment_ddl.groovy @@ -0,0 +1,83 @@ +// 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_struct_nested_comment_ddl", "p0") { + sql "DROP TABLE IF EXISTS struct_nested_comment" + sql "DROP TABLE IF EXISTS struct_nested_comment_replay" + sql "DROP TABLE IF EXISTS struct_nested_comment_like" + sql "DROP TABLE IF EXISTS struct_field_name_quoting" + + // The comment on b carries a single quote and a back slash, both have to survive the DDL. + sql """ + CREATE TABLE struct_nested_comment ( + id INT, + s STRUCT COMMENT "top-level", + n STRUCT>, + arr ARRAY> + ) + DUPLICATE KEY(id) + DISTRIBUTED BY HASH(id) BUCKETS 1 + PROPERTIES ("replication_num" = "1") + """ + + // SHOW CREATE TABLE used to drop every nested field comment. + qt_show_create "SHOW CREATE TABLE struct_nested_comment" + def createStmt = (sql "SHOW CREATE TABLE struct_nested_comment")[0][1].toString() + + // The printed DDL still has to parse back. + def replayStmt = createStmt.replace("`struct_nested_comment`", "`struct_nested_comment_replay`") + sql replayStmt + assertEquals(replayStmt, (sql "SHOW CREATE TABLE struct_nested_comment_replay")[0][1].toString()) + + // CREATE TABLE LIKE re-parses the same generated DDL, so a dropped nested comment is + // gone from the new table for good, not just hidden from the user. + sql "CREATE TABLE struct_nested_comment_like LIKE struct_nested_comment" + def likeStmt = (sql "SHOW CREATE TABLE struct_nested_comment_like")[0][1].toString() + assertEquals(createStmt.replace("`struct_nested_comment`", "`struct_nested_comment_like`"), likeStmt) + + // The comment is escaped for the mode the statement will be read under, so a back slash + // survives CREATE TABLE LIKE under NO_BACKSLASH_ESCAPES too. + sql "SET sql_mode = 'NO_BACKSLASH_ESCAPES'" + def nbseStmt = (sql "SHOW CREATE TABLE struct_nested_comment")[0][1].toString() + sql "DROP TABLE IF EXISTS struct_nested_comment_like" + sql "CREATE TABLE struct_nested_comment_like LIKE struct_nested_comment" + assertEquals(nbseStmt.replace("`struct_nested_comment`", "`struct_nested_comment_like`"), + (sql "SHOW CREATE TABLE struct_nested_comment_like")[0][1].toString()) + sql "SET sql_mode = ''" + + // Field names holding a separator are legal and have to stay tellable apart in COLUMN_TYPE. + sql """ + CREATE TABLE struct_field_name_quoting ( + id INT, + s STRUCT<`a,b`:INT, `c:d`:INT, `e`:INT, plain:INT> + ) + DUPLICATE KEY(id) + DISTRIBUTED BY HASH(id) BUCKETS 1 + PROPERTIES ("replication_num" = "1") + """ + + // COLUMN_TYPE has to describe a struct with its field names. + sql "use information_schema" + qt_column_type """ + SELECT column_name, data_type, column_type FROM columns + WHERE table_name = 'struct_nested_comment' ORDER BY column_name + """ + qt_column_type_quoting """ + SELECT column_name, column_type FROM columns + WHERE table_name = 'struct_field_name_quoting' ORDER BY column_name + """ +}