From 23ef79e900de0f52548b12d491268529d2ee4e94 Mon Sep 17 00:00:00 2001 From: Lloyd-Pottiger <60744015+Lloyd-Pottiger@users.noreply.github.com> Date: Thu, 14 Apr 2022 13:10:35 +0800 Subject: [PATCH 1/4] This is an automated cherry-pick of #4630 Signed-off-by: ti-chi-bot --- dbms/src/Databases/test/gtest_database.cpp | 124 ++++++++++++++++++++- dbms/src/Storages/Transaction/TiDB.cpp | 10 ++ dbms/src/Storages/Transaction/TiDB.h | 24 ++-- 3 files changed, 142 insertions(+), 16 deletions(-) diff --git a/dbms/src/Databases/test/gtest_database.cpp b/dbms/src/Databases/test/gtest_database.cpp index 526a18be483..11601ece05d 100644 --- a/dbms/src/Databases/test/gtest_database.cpp +++ b/dbms/src/Databases/test/gtest_database.cpp @@ -70,7 +70,7 @@ class DatabaseTiFlash_test : public ::testing::Test } } - void recreateMetadataPath() const + static void recreateMetadataPath() { String path = TiFlashTestEnv::getContext().getPath(); @@ -628,7 +628,123 @@ try } CATCH +<<<<<<< HEAD TEST_F(DatabaseTiFlash_test, ISSUE_1055) +======= +TEST_F(DatabaseTiFlashTest, ISSUE4596) +try +{ + const String db_name = "db_1"; + auto ctx = TiFlashTestEnv::getContext(); + + { + // Create database + const String statement = "CREATE DATABASE " + db_name + " ENGINE=TiFlash"; + ASTPtr ast = parseCreateStatement(statement); + InterpreterCreateQuery interpreter(ast, ctx); + interpreter.setInternal(true); + interpreter.setForceRestoreData(false); + interpreter.execute(); + } + + auto db = ctx.getDatabase(db_name); + + const String tbl_name = "t_111"; + { + /// Create table + ParserCreateQuery parser; + const String stmt = fmt::format("CREATE TABLE `{}`.`{}` ", db_name, tbl_name) + + R"stmt( + (`id` Int32,`b` String) Engine = DeltaMerge((`id`), + '{ + "cols":[{ + "comment":"", + "default":null, + "default_bit":null, + "id":1, + "name":{ + "L":"id", + "O":"id" + }, + "offset":0, + "origin_default":null, + "state":5, + "type":{ + "Charset":"binary", + "Collate":"binary", + "Decimal":0, + "Elems":null, + "Flag":515, + "Flen":16, + "Tp":3 + } + }, + { + "comment":"", + "default":"", + "default_bit":null, + "id":15, + "name":{ + "L":"b", + "O":"b" + }, + "offset":12, + "origin_default":"", + "state":5, + "type":{ + "Charset":"binary", + "Collate":"binary", + "Decimal":0, + "Elems":null, + "Flag":4225, + "Flen":-1, + "Tp":251 + } + }], + "comment":"", + "id":330, + "index_info":[], + "is_common_handle":false, + "name":{ + "L":"test", + "O":"test" + }, + "partition":null, + "pk_is_handle":true, + "schema_version":465, + "state":5, + "update_timestamp":99999 + }' + ) + )stmt"; + ASTPtr ast = parseQuery(parser, stmt, 0); + + InterpreterCreateQuery interpreter(ast, ctx); + interpreter.setInternal(true); + interpreter.setForceRestoreData(false); + interpreter.execute(); + } + + EXPECT_FALSE(db->empty(ctx)); + EXPECT_TRUE(db->isTableExist(ctx, tbl_name)); + + { + // Get storage from database + auto storage = db->tryGetTable(ctx, tbl_name); + ASSERT_NE(storage, nullptr); + + EXPECT_EQ(storage->getName(), MutableSupport::delta_tree_storage_name); + EXPECT_EQ(storage->getTableName(), tbl_name); + + auto managed_storage = std::dynamic_pointer_cast(storage); + EXPECT_EQ(managed_storage->getDatabaseName(), db_name); + EXPECT_EQ(managed_storage->getTableInfo().name, "test"); + } +} +CATCH + +TEST_F(DatabaseTiFlashTest, ISSUE1055) +>>>>>>> 51dd32f4d9 (Fix create table error (#4630)) try { CHECK_TESTS_WITH_DATA_ENABLED; @@ -664,7 +780,7 @@ try DatabaseLoading::loadTable(ctx, *db, meta_path, db_name, db_data_path, "TiFlash", "t_45.sql", false); // Get storage from database - const auto tbl_name = "t_45"; + const auto * tbl_name = "t_45"; auto storage = db->tryGetTable(ctx, tbl_name); ASSERT_NE(storage, nullptr); EXPECT_EQ(storage->getName(), MutableSupport::delta_tree_storage_name); @@ -752,7 +868,7 @@ try auto db = ctx.getDatabase(name_mapper.mapDatabaseName(*db_info)); ASSERT_NE(db, nullptr); EXPECT_EQ(db->getEngineName(), "TiFlash"); - auto flash_db = typeid_cast(db.get()); + auto * flash_db = typeid_cast(db.get()); auto & db_info_get = flash_db->getDatabaseInfo(); ASSERT_EQ(db_info_get.name, expect_name); } @@ -817,7 +933,7 @@ try )", }; - for (auto & statement : statements) + for (const auto & statement : statements) { { // Cleanup: Drop database if exists diff --git a/dbms/src/Storages/Transaction/TiDB.cpp b/dbms/src/Storages/Transaction/TiDB.cpp index 36b18764fcb..f7ba1a25f13 100644 --- a/dbms/src/Storages/Transaction/TiDB.cpp +++ b/dbms/src/Storages/Transaction/TiDB.cpp @@ -87,6 +87,7 @@ Field ColumnInfo::defaultValueToField() const case TypeVarString: case TypeString: { +<<<<<<< HEAD auto v = value.convert(); if (hasBinaryFlag()) { @@ -96,6 +97,15 @@ Field ColumnInfo::defaultValueToField() const v.append(flen - v.length(), '\0'); } return v; +======= + // For some binary column(like varchar(20)), we have to pad trailing zeros according to the specified type length. + // User may define default value `0x1234` for a `BINARY(4)` column, TiDB stores it in a string "\u12\u34" (sized 2). + // But it actually means `0x12340000`. + // And for some binary column(like longblob), we do not need to pad trailing zeros. + // And the `Flen` is set to -1, therefore we need to check `Flen >= 0` here. + if (Int32 vlen = v.length(); flen >= 0 && vlen < flen) + v.append(flen - vlen, '\0'); +>>>>>>> 51dd32f4d9 (Fix create table error (#4630)) } case TypeJSON: // JSON can't have a default value diff --git a/dbms/src/Storages/Transaction/TiDB.h b/dbms/src/Storages/Transaction/TiDB.h index 1f40c86b8cd..e8bc74df32e 100644 --- a/dbms/src/Storages/Transaction/TiDB.h +++ b/dbms/src/Storages/Transaction/TiDB.h @@ -79,7 +79,7 @@ enum TP #ifdef M #error "Please undefine macro M first." #endif -#define M(tt, v, cf, ct, w) Type##tt = v, +#define M(tt, v, cf, ct, w) Type##tt = (v), COLUMN_TYPES(M) #undef M }; @@ -111,7 +111,7 @@ enum ColumnFlag #ifdef M #error "Please undefine macro M first." #endif -#define M(cf, v) ColumnFlag##cf = v, +#define M(cf, v) ColumnFlag##cf = (v), COLUMN_FLAGS(M) #undef M }; @@ -140,7 +140,7 @@ enum CodecFlag #ifdef M #error "Please undefine macro M first." #endif -#define M(cf, v) CodecFlag##cf = v, +#define M(cf, v) CodecFlag##cf = (v), CODEC_FLAGS(M) #undef M }; @@ -185,10 +185,10 @@ struct ColumnInfo #ifdef M #error "Please undefine macro M first." #endif -#define M(f, v) \ - inline bool has##f##Flag() const { return (flag & v) != 0; } \ - inline void set##f##Flag() { flag |= v; } \ - inline void clear##f##Flag() { flag &= (~v); } +#define M(f, v) \ + inline bool has##f##Flag() const { return (flag & (v)) != 0; } \ + inline void set##f##Flag() { flag |= (v); } \ + inline void clear##f##Flag() { flag &= (~(v)); } COLUMN_FLAGS(M) #undef M @@ -213,7 +213,7 @@ struct PartitionDefinition { PartitionDefinition() = default; - PartitionDefinition(Poco::JSON::Object::Ptr json); + explicit PartitionDefinition(Poco::JSON::Object::Ptr json); Poco::JSON::Object::Ptr getJSONObject() const; @@ -229,7 +229,7 @@ struct PartitionInfo { PartitionInfo() = default; - PartitionInfo(Poco::JSON::Object::Ptr json); + explicit PartitionInfo(Poco::JSON::Object::Ptr json); Poco::JSON::Object::Ptr getJSONObject() const; @@ -252,7 +252,7 @@ struct DBInfo SchemaState state; DBInfo() = default; - DBInfo(const String & json) { deserialize(json); } + explicit DBInfo(const String & json) { deserialize(json); } String serialize() const; @@ -357,9 +357,9 @@ struct TableInfo ::TiDB::StorageEngine engine_type = ::TiDB::StorageEngine::UNSPECIFIED; ColumnID getColumnID(const String & name) const; - String getColumnName(const ColumnID id) const; + String getColumnName(ColumnID id) const; - const ColumnInfo & getColumnInfo(const ColumnID id) const; + const ColumnInfo & getColumnInfo(ColumnID id) const; std::optional> getPKHandleColumn() const; From 893306ac39110fa6ff2aef0acc85805cef6003fa Mon Sep 17 00:00:00 2001 From: Lloyd-Pottiger Date: Thu, 14 Apr 2022 16:53:34 +0800 Subject: [PATCH 2/4] reslove conflict --- dbms/src/Databases/test/gtest_database.cpp | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/dbms/src/Databases/test/gtest_database.cpp b/dbms/src/Databases/test/gtest_database.cpp index 11601ece05d..e9ae9ae37dc 100644 --- a/dbms/src/Databases/test/gtest_database.cpp +++ b/dbms/src/Databases/test/gtest_database.cpp @@ -628,10 +628,7 @@ try } CATCH -<<<<<<< HEAD -TEST_F(DatabaseTiFlash_test, ISSUE_1055) -======= -TEST_F(DatabaseTiFlashTest, ISSUE4596) +TEST_F(DatabaseTiFlash_test, ISSUE4596) try { const String db_name = "db_1"; @@ -743,8 +740,7 @@ try } CATCH -TEST_F(DatabaseTiFlashTest, ISSUE1055) ->>>>>>> 51dd32f4d9 (Fix create table error (#4630)) +TEST_F(DatabaseTiFlash_test, ISSUE_1055) try { CHECK_TESTS_WITH_DATA_ENABLED; From 2cc0e57ef9dc2ef7a1a8b124cffe6b8ddb830f51 Mon Sep 17 00:00:00 2001 From: Lloyd-Pottiger Date: Thu, 14 Apr 2022 16:54:17 +0800 Subject: [PATCH 3/4] reslove conflict --- dbms/src/Storages/Transaction/TiDB.cpp | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/dbms/src/Storages/Transaction/TiDB.cpp b/dbms/src/Storages/Transaction/TiDB.cpp index f7ba1a25f13..aa6138cdae8 100644 --- a/dbms/src/Storages/Transaction/TiDB.cpp +++ b/dbms/src/Storages/Transaction/TiDB.cpp @@ -87,25 +87,18 @@ Field ColumnInfo::defaultValueToField() const case TypeVarString: case TypeString: { -<<<<<<< HEAD auto v = value.convert(); if (hasBinaryFlag()) { - // For binary column, we have to pad trailing zeros according to the specified type length. + // For some binary column(like varchar(20)), we have to pad trailing zeros according to the specified type length. // User may define default value `0x1234` for a `BINARY(4)` column, TiDB stores it in a string "\u12\u34" (sized 2). // But it actually means `0x12340000`. - v.append(flen - v.length(), '\0'); + // And for some binary column(like longblob), we do not need to pad trailing zeros. + // And the `Flen` is set to -1, therefore we need to check `Flen >= 0` here. + if (Int32 vlen = v.length(); flen >= 0 && vlen < flen) + v.append(flen - vlen, '\0'); } return v; -======= - // For some binary column(like varchar(20)), we have to pad trailing zeros according to the specified type length. - // User may define default value `0x1234` for a `BINARY(4)` column, TiDB stores it in a string "\u12\u34" (sized 2). - // But it actually means `0x12340000`. - // And for some binary column(like longblob), we do not need to pad trailing zeros. - // And the `Flen` is set to -1, therefore we need to check `Flen >= 0` here. - if (Int32 vlen = v.length(); flen >= 0 && vlen < flen) - v.append(flen - vlen, '\0'); ->>>>>>> 51dd32f4d9 (Fix create table error (#4630)) } case TypeJSON: // JSON can't have a default value From aafa0402aca8cf2b6ec2967928483cbb5343e8ad Mon Sep 17 00:00:00 2001 From: Lloyd-Pottiger <60744015+Lloyd-Pottiger@users.noreply.github.com> Date: Tue, 20 Sep 2022 13:09:11 +0800 Subject: [PATCH 4/4] Update dbms/src/Databases/test/gtest_database.cpp --- dbms/src/Databases/test/gtest_database.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dbms/src/Databases/test/gtest_database.cpp b/dbms/src/Databases/test/gtest_database.cpp index e9ae9ae37dc..1ff6b76a725 100644 --- a/dbms/src/Databases/test/gtest_database.cpp +++ b/dbms/src/Databases/test/gtest_database.cpp @@ -650,7 +650,7 @@ try { /// Create table ParserCreateQuery parser; - const String stmt = fmt::format("CREATE TABLE `{}`.`{}` ", db_name, tbl_name) + + const String stmt = "CREATE TABLE `" + db_name + "`.`" + tbl_name + "` " + R"stmt( (`id` Int32,`b` String) Engine = DeltaMerge((`id`), '{