Skip to content

Commit

Permalink
Fix create table error (#4630) (#4649)
Browse files Browse the repository at this point in the history 
close #4596
  • Loading branch information
@ti-chi-bot
ti-chi-bot authored Jun 16, 2022
1 parent 7143242 commit b9080c3
Show file tree
Hide file tree
Showing 4 changed files with 159 additions and 42 deletions.
2 changes: 1 addition & 1 deletion dbms/src/DataTypes/DataTypeDecimal.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class DataTypeDecimal : public IDataType
{
if (precision > decimal_max_prec || scale > precision || scale > decimal_max_scale)
{
throw Exception(getName() + "is out of bound", ErrorCodes::ARGUMENT_OUT_OF_BOUND);
throw Exception(getName() + "is out of bound", ErrorCodes::ARGUMENT_OUT_OF_BOUND); // NOLINT
}
}

Expand Down
142 changes: 128 additions & 14 deletions dbms/src/Databases/test/gtest_database.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ extern String createDatabaseStmt(Context & context, const TiDB::DBInfo & db_info

namespace tests
{

class DatabaseTiFlash_test : public ::testing::Test
{
public:
Expand All @@ -55,7 +54,9 @@ class DatabaseTiFlash_test : public ::testing::Test
}
}

DatabaseTiFlash_test() : log(&Poco::Logger::get("DatabaseTiFlash_test")) {}
DatabaseTiFlash_test()
: log(&Poco::Logger::get("DatabaseTiFlash_test"))
{}

void SetUp() override { recreateMetadataPath(); }

Expand All @@ -70,7 +71,7 @@ class DatabaseTiFlash_test : public ::testing::Test
}
}

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

Expand Down Expand Up @@ -98,13 +99,13 @@ ASTPtr parseCreateStatement(const String & statement)
const char * pos = statement.data();
std::string error_msg;
auto ast = tryParseQuery(parser,
pos,
pos + statement.size(),
error_msg,
/*hilite=*/false,
String("in ") + __PRETTY_FUNCTION__,
/*allow_multi_statements=*/false,
0);
pos,
pos + statement.size(),
error_msg,
/*hilite=*/false,
String("in ") + __PRETTY_FUNCTION__,
/*allow_multi_statements=*/false,
0);
if (!ast)
throw Exception(error_msg, ErrorCodes::SYNTAX_ERROR);
return ast;
Expand Down Expand Up @@ -482,7 +483,8 @@ try
// Rename table to another database, and mock crash by failed point
FailPointHelper::enableFailPoint(FailPoints::exception_before_rename_table_old_meta_removed);
ASSERT_THROW(
typeid_cast<DatabaseTiFlash *>(db.get())->renameTable(ctx, tbl_name, *db2, to_tbl_name, db2_name, to_tbl_name), DB::Exception);
typeid_cast<DatabaseTiFlash *>(db.get())->renameTable(ctx, tbl_name, *db2, to_tbl_name, db2_name, to_tbl_name),
DB::Exception);

{
// After fail point triggled we should have both meta file in disk
Expand Down Expand Up @@ -628,6 +630,118 @@ try
}
CATCH

TEST_F(DatabaseTiFlash_test, 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(DatabaseTiFlash_test, ISSUE_1055)
try
{
Expand Down Expand Up @@ -664,7 +778,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 @@ -752,7 +866,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 @@ -817,7 +931,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 @@ -39,7 +39,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 @@ -61,7 +61,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 @@ -93,10 +93,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 @@ -203,9 +206,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 @@ -223,7 +226,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 @@ -236,7 +239,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 @@ -275,7 +278,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 @@ -411,7 +414,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 @@ -620,7 +623,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 @@ -704,15 +707,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 @@ -908,7 +911,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 @@ -941,7 +944,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
Loading

0 comments on commit b9080c3

Please sign in to comment.