Skip to content

Commit

Permalink
Fix create table error (#4630) (#4650)
Browse files Browse the repository at this point in the history 
close #4596
  • Loading branch information
@ti-chi-bot
ti-chi-bot authored Apr 25, 2022
1 parent 4d42bb8 commit 68dbbb9
Show file tree
Hide file tree
Showing 4 changed files with 149 additions and 34 deletions.
6 changes: 3 additions & 3 deletions .gitmodules
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,13 @@
branch = master
[submodule "contrib/client-c"]
path = contrib/client-c
url = git@github.com:tikv/client-c.git
url = https://github.com/tikv/client-c.git
[submodule "contrib/tiflash-proxy"]
path = contrib/tiflash-proxy
url = git@github.com:pingcap/tidb-engine-ext.git
url = https://github.com/pingcap/tidb-engine-ext.git
[submodule "contrib/prometheus-cpp"]
path = contrib/prometheus-cpp
url = git@github.com:jupp0r/prometheus-cpp.git
url = https://github.com/jupp0r/prometheus-cpp.git
[submodule "contrib/junction"]
path = contrib/junction
url = https://github.com/preshing/junction.git
Expand Down
120 changes: 116 additions & 4 deletions dbms/src/Databases/test/gtest_database.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ class DatabaseTiFlashTest : public ::testing::Test
}
}

void recreateMetadataPath() const
static void recreateMetadataPath()
{
String path = TiFlashTestEnv::getContext().getPath();

Expand Down Expand Up @@ -638,6 +638,118 @@ try
}
CATCH

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<IManageableStorage>(storage);
EXPECT_EQ(managed_storage->getDatabaseName(), db_name);
EXPECT_EQ(managed_storage->getTableInfo().name, "test");
}
}
CATCH

TEST_F(DatabaseTiFlashTest, ISSUE1055)
try
{
Expand Down Expand Up @@ -674,7 +786,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);
Expand Down Expand Up @@ -762,7 +874,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<DatabaseTiFlash *>(db.get());
auto * flash_db = typeid_cast<DatabaseTiFlash *>(db.get());
auto & db_info_get = flash_db->getDatabaseInfo();
ASSERT_EQ(db_info_get.name, expect_name);
}
Expand Down Expand Up @@ -827,7 +939,7 @@ try
)",
};

for (auto & statement : statements)
for (const auto & statement : statements)
{
{
// Cleanup: Drop database if exists
Expand Down
33 changes: 18 additions & 15 deletions dbms/src/Storages/Transaction/TiDB.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ ColumnInfo::ColumnInfo(Poco::JSON::Object::Ptr json)

Field ColumnInfo::defaultValueToField() const
{
auto & value = origin_default_value;
const auto & value = origin_default_value;
if (value.isEmpty())
{
if (hasNotNullFlag())
Expand All @@ -109,7 +109,7 @@ Field ColumnInfo::defaultValueToField() const
// TODO: We shall use something like `orig_default_bit`, which will never change once created,
// rather than `default_bit`, which could be altered.
// See https://github.com/pingcap/tidb/issues/17641 and https://github.com/pingcap/tidb/issues/17642
auto & bit_value = default_bit_value;
const auto & bit_value = default_bit_value;
// TODO: There might be cases that `orig_default` is not null but `default_bit` is null,
// i.e. bit column added with an default value but later modified to another.
// For these cases, neither `orig_default` (may get corrupted) nor `default_bit` (modified) is correct.
Expand Down Expand Up @@ -141,10 +141,13 @@ Field ColumnInfo::defaultValueToField() const
auto v = value.convert<String>();
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;
}
Expand Down Expand Up @@ -251,9 +254,9 @@ UInt64 ColumnInfo::getSetValue(const String & set_str) const
throw DB::Exception(std::string(__PRETTY_FUNCTION__) + ": can't parse set type value.");
}

Int64 ColumnInfo::getTimeValue(const String & time_str) const
Int64 ColumnInfo::getTimeValue(const String & time_str) const // NOLINT
{
const static long fractional_seconds_multiplier[] = {1000000000, 100000000, 10000000, 1000000, 100000, 10000, 1000, 100, 10, 1};
const static Int64 fractional_seconds_multiplier[] = {1000000000, 100000000, 10000000, 1000000, 100000, 10000, 1000, 100, 10, 1};
bool negative = time_str[0] == '-';
Poco::StringTokenizer second_and_fsp(time_str, ".");
Poco::StringTokenizer string_tokens(second_and_fsp[0], ":");
Expand All @@ -271,7 +274,7 @@ Int64 ColumnInfo::getTimeValue(const String & time_str) const
return negative ? -ret : ret;
}

Int64 ColumnInfo::getYearValue(const String & val) const
Int64 ColumnInfo::getYearValue(const String & val) const // NOLINT
{
// do not check validation of the val because TiDB will do it
Int64 year = std::stol(val);
Expand All @@ -284,7 +287,7 @@ Int64 ColumnInfo::getYearValue(const String & val) const
return year;
}

UInt64 ColumnInfo::getBitValue(const String & val) const
UInt64 ColumnInfo::getBitValue(const String & val) const // NOLINT
{
// The `default_bit` is a base64 encoded, big endian byte array.
Poco::MemoryInputStream istr(val.data(), val.size());
Expand Down Expand Up @@ -323,7 +326,7 @@ try
if (!elems.empty())
{
Poco::JSON::Array::Ptr elem_arr = new Poco::JSON::Array();
for (auto & elem : elems)
for (const auto & elem : elems)
elem_arr->add(elem.first);
tp_json->set("Elems", elem_arr);
}
Expand Down Expand Up @@ -459,7 +462,7 @@ try

Poco::JSON::Array::Ptr def_arr = new Poco::JSON::Array();

for (auto & part_def : definitions)
for (const auto & part_def : definitions)
{
def_arr->add(part_def.getJSONObject());
}
Expand Down Expand Up @@ -668,7 +671,7 @@ try
json->set("tbl_name", tbl_name_json);

Poco::JSON::Array::Ptr cols_array = new Poco::JSON::Array();
for (auto & col : idx_cols)
for (const auto & col : idx_cols)
{
auto col_obj = col.getJSONObject();
cols_array->add(col_obj);
Expand Down Expand Up @@ -752,15 +755,15 @@ try
json->set("name", name_json);

Poco::JSON::Array::Ptr cols_arr = new Poco::JSON::Array();
for (auto & col_info : columns)
for (const auto & col_info : columns)
{
auto col_obj = col_info.getJSONObject();
cols_arr->add(col_obj);
}

json->set("cols", cols_arr);
Poco::JSON::Array::Ptr index_arr = new Poco::JSON::Array();
for (auto & index_info : index_infos)
for (const auto & index_info : index_infos)
{
auto index_info_obj = index_info.getJSONObject();
index_arr->add(index_info_obj);
Expand Down Expand Up @@ -956,7 +959,7 @@ ColumnID TableInfo::getColumnID(const String & name) const

String TableInfo::getColumnName(const ColumnID id) const
{
for (auto & col : columns)
for (const auto & col : columns)
{
if (id == col.id)
{
Expand Down Expand Up @@ -989,7 +992,7 @@ std::optional<std::reference_wrapper<const ColumnInfo>> TableInfo::getPKHandleCo
if (!pk_is_handle)
return std::nullopt;

for (auto & col : columns)
for (const auto & col : columns)
{
if (col.hasPriKeyFlag())
return std::optional<std::reference_wrapper<const ColumnInfo>>(col);
Expand Down
24 changes: 12 additions & 12 deletions dbms/src/Storages/Transaction/TiDB.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,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
};
Expand Down Expand Up @@ -109,7 +109,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
};
Expand Down Expand Up @@ -138,7 +138,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
};
Expand Down Expand Up @@ -183,10 +183,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

Expand All @@ -211,7 +211,7 @@ struct PartitionDefinition
{
PartitionDefinition() = default;

PartitionDefinition(Poco::JSON::Object::Ptr json);
explicit PartitionDefinition(Poco::JSON::Object::Ptr json);

Poco::JSON::Object::Ptr getJSONObject() const;

Expand All @@ -227,7 +227,7 @@ struct PartitionInfo
{
PartitionInfo() = default;

PartitionInfo(Poco::JSON::Object::Ptr json);
explicit PartitionInfo(Poco::JSON::Object::Ptr json);

Poco::JSON::Object::Ptr getJSONObject() const;

Expand All @@ -250,7 +250,7 @@ struct DBInfo
SchemaState state;

DBInfo() = default;
DBInfo(const String & json) { deserialize(json); }
explicit DBInfo(const String & json) { deserialize(json); }

String serialize() const;

Expand Down Expand Up @@ -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<std::reference_wrapper<const ColumnInfo>> getPKHandleColumn() const;

Expand Down

0 comments on commit 68dbbb9

Please sign in to comment.