diff --git a/CHANGELOG.md b/CHANGELOG.md index 7d88bc270e..62c82df007 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,11 @@ Please mark all change in change log and use the ticket from JIRA. - \#340 - Test cases run failed on 0.6.0 - \#353 - Rename config.h.in to version.h.in - \#374 - sdk_simple return empty result +- \#377 - Create partition success if tag name only contains spaces - \#397 - sdk_simple return incorrect result +- \#399 - Create partition should be failed if partition tag existed +- \#412 - Message returned is confused when partition created with null partition name +- \#416 - Drop the same partition success repeatally ## Feature - \#12 - Pure CPU version for Milvus diff --git a/core/src/db/DBImpl.cpp b/core/src/db/DBImpl.cpp index 2559b3a46b..7b0103f52e 100644 --- a/core/src/db/DBImpl.cpp +++ b/core/src/db/DBImpl.cpp @@ -279,6 +279,11 @@ DBImpl::DropPartitionByTag(const std::string& table_id, const std::string& parti std::string partition_name; auto status = meta_ptr_->GetPartitionName(table_id, partition_tag, partition_name); + if (!status.ok()) { + ENGINE_LOG_ERROR << status.message(); + return status; + } + return DropPartition(partition_name); } @@ -853,8 +858,12 @@ DBImpl::GetPartitionsByTags(const std::string& table_id, const std::vectorShowPartitions(table_id, partiton_array); for (auto& tag : partition_tags) { + // trim side-blank of tag, only compare valid characters + // for example: " ab cd " is treated as "ab cd" + std::string valid_tag = tag; + server::StringHelpFunctions::TrimStringBlank(valid_tag); for (auto& schema : partiton_array) { - if (server::StringHelpFunctions::IsRegexMatch(schema.partition_tag_, tag)) { + if (server::StringHelpFunctions::IsRegexMatch(schema.partition_tag_, valid_tag)) { partition_name_array.insert(schema.table_id_); } } diff --git a/core/src/db/meta/MySQLMetaImpl.cpp b/core/src/db/meta/MySQLMetaImpl.cpp index bf83447806..4406b87f7e 100644 --- a/core/src/db/meta/MySQLMetaImpl.cpp +++ b/core/src/db/meta/MySQLMetaImpl.cpp @@ -22,6 +22,7 @@ #include "metrics/Metrics.h" #include "utils/Exception.h" #include "utils/Log.h" +#include "utils/StringHelpFunctions.h" #include #include @@ -1162,17 +1163,23 @@ MySQLMetaImpl::CreatePartition(const std::string& table_id, const std::string& p // not allow create partition under partition if (!table_schema.owner_table_.empty()) { - return Status(DB_ERROR, "Nested partition is not allow"); + return Status(DB_ERROR, "Nested partition is not allowed"); + } + + // trim side-blank of tag, only compare valid characters + // for example: " ab cd " is treated as "ab cd" + std::string valid_tag = tag; + server::StringHelpFunctions::TrimStringBlank(valid_tag); + + // not allow duplicated partition + std::string exist_partition; + GetPartitionName(table_id, valid_tag, exist_partition); + if (!exist_partition.empty()) { + return Status(DB_ERROR, "Duplicate partition is not allowed"); } if (partition_name == "") { - // not allow duplicated partition - std::string exist_partition; - GetPartitionName(table_id, tag, exist_partition); - if (!exist_partition.empty()) { - return Status(DB_ERROR, "Duplicated partition is not allow"); - } - + // generate unique partition name NextTableId(table_schema.table_id_); } else { table_schema.table_id_ = partition_name; @@ -1182,9 +1189,14 @@ MySQLMetaImpl::CreatePartition(const std::string& table_id, const std::string& p table_schema.flag_ = 0; table_schema.created_on_ = utils::GetMicroSecTimeStamp(); table_schema.owner_table_ = table_id; - table_schema.partition_tag_ = tag; + table_schema.partition_tag_ = valid_tag; - return CreateTable(table_schema); + status = CreateTable(table_schema); + if (status.code() == DB_ALREADY_EXIST) { + return Status(DB_ALREADY_EXIST, "Partition already exists"); + } + + return status; } Status @@ -1231,6 +1243,12 @@ MySQLMetaImpl::GetPartitionName(const std::string& table_id, const std::string& try { server::MetricCollector metric; mysqlpp::StoreQueryResult res; + + // trim side-blank of tag, only compare valid characters + // for example: " ab cd " is treated as "ab cd" + std::string valid_tag = tag; + server::StringHelpFunctions::TrimStringBlank(valid_tag); + { mysqlpp::ScopedConnection connectionPtr(*mysql_connection_pool_, safe_grab_); @@ -1240,7 +1258,7 @@ MySQLMetaImpl::GetPartitionName(const std::string& table_id, const std::string& mysqlpp::Query allPartitionsQuery = connectionPtr->query(); allPartitionsQuery << "SELECT table_id FROM " << META_TABLES << " WHERE owner_table = " << mysqlpp::quote - << table_id << " AND partition_tag = " << mysqlpp::quote << tag << " AND state <> " + << table_id << " AND partition_tag = " << mysqlpp::quote << valid_tag << " AND state <> " << std::to_string(TableSchema::TO_DELETE) << ";"; ENGINE_LOG_DEBUG << "MySQLMetaImpl::AllTables: " << allPartitionsQuery.str(); @@ -1252,7 +1270,7 @@ MySQLMetaImpl::GetPartitionName(const std::string& table_id, const std::string& const mysqlpp::Row& resRow = res[0]; resRow["table_id"].to_string(partition_name); } else { - return Status(DB_NOT_FOUND, "Partition " + tag + " of table " + table_id + " not found"); + return Status(DB_NOT_FOUND, "Partition " + valid_tag + " of table " + table_id + " not found"); } } catch (std::exception& e) { return HandleException("GENERAL ERROR WHEN GET PARTITION NAME", e.what()); diff --git a/core/src/db/meta/SqliteMetaImpl.cpp b/core/src/db/meta/SqliteMetaImpl.cpp index 22e953fe9d..12128c074d 100644 --- a/core/src/db/meta/SqliteMetaImpl.cpp +++ b/core/src/db/meta/SqliteMetaImpl.cpp @@ -22,6 +22,7 @@ #include "metrics/Metrics.h" #include "utils/Exception.h" #include "utils/Log.h" +#include "utils/StringHelpFunctions.h" #include #include @@ -757,17 +758,23 @@ SqliteMetaImpl::CreatePartition(const std::string& table_id, const std::string& // not allow create partition under partition if(!table_schema.owner_table_.empty()) { - return Status(DB_ERROR, "Nested partition is not allow"); + return Status(DB_ERROR, "Nested partition is not allowed"); + } + + // trim side-blank of tag, only compare valid characters + // for example: " ab cd " is treated as "ab cd" + std::string valid_tag = tag; + server::StringHelpFunctions::TrimStringBlank(valid_tag); + + // not allow duplicated partition + std::string exist_partition; + GetPartitionName(table_id, valid_tag, exist_partition); + if(!exist_partition.empty()) { + return Status(DB_ERROR, "Duplicate partition is not allowed"); } if (partition_name == "") { - // not allow duplicated partition - std::string exist_partition; - GetPartitionName(table_id, tag, exist_partition); - if(!exist_partition.empty()) { - return Status(DB_ERROR, "Duplicated partition is not allow"); - } - + // generate unique partition name NextTableId(table_schema.table_id_); } else { table_schema.table_id_ = partition_name; @@ -777,9 +784,14 @@ SqliteMetaImpl::CreatePartition(const std::string& table_id, const std::string& table_schema.flag_ = 0; table_schema.created_on_ = utils::GetMicroSecTimeStamp(); table_schema.owner_table_ = table_id; - table_schema.partition_tag_ = tag; + table_schema.partition_tag_ = valid_tag; - return CreateTable(table_schema); + status = CreateTable(table_schema); + if (status.code() == DB_ALREADY_EXIST) { + return Status(DB_ALREADY_EXIST, "Partition already exists"); + } + + return status; } Status @@ -814,13 +826,18 @@ SqliteMetaImpl::GetPartitionName(const std::string& table_id, const std::string& try { server::MetricCollector metric; + // trim side-blank of tag, only compare valid characters + // for example: " ab cd " is treated as "ab cd" + std::string valid_tag = tag; + server::StringHelpFunctions::TrimStringBlank(valid_tag); + auto name = ConnectorPtr->select(columns(&TableSchema::table_id_), where(c(&TableSchema::owner_table_) == table_id - and c(&TableSchema::partition_tag_) == tag)); + and c(&TableSchema::partition_tag_) == valid_tag)); if (name.size() > 0) { partition_name = std::get<0>(name[0]); } else { - return Status(DB_NOT_FOUND, "Table " + table_id + "'s partition " + tag + " not found"); + return Status(DB_NOT_FOUND, "Table " + table_id + "'s partition " + valid_tag + " not found"); } } catch (std::exception &e) { return HandleException("Encounter exception when get partition name", e.what()); diff --git a/core/src/server/grpc_impl/request/CreatePartitionRequest.cpp b/core/src/server/grpc_impl/request/CreatePartitionRequest.cpp index bc2e7dc105..3bd4a86ef6 100644 --- a/core/src/server/grpc_impl/request/CreatePartitionRequest.cpp +++ b/core/src/server/grpc_impl/request/CreatePartitionRequest.cpp @@ -51,7 +51,7 @@ CreatePartitionRequest::OnExecute() { return status; } - status = ValidationUtil::ValidateTableName(partition_param_->partition_name()); + status = ValidationUtil::ValidatePartitionName(partition_param_->partition_name()); if (!status.ok()) { return status; } diff --git a/core/src/server/grpc_impl/request/DropPartitionRequest.cpp b/core/src/server/grpc_impl/request/DropPartitionRequest.cpp index 217ca63a2a..0e29b6abe8 100644 --- a/core/src/server/grpc_impl/request/DropPartitionRequest.cpp +++ b/core/src/server/grpc_impl/request/DropPartitionRequest.cpp @@ -22,6 +22,7 @@ #include "utils/ValidationUtil.h" #include +#include namespace milvus { namespace server { @@ -38,23 +39,40 @@ DropPartitionRequest::Create(const ::milvus::grpc::PartitionParam* partition_par Status DropPartitionRequest::OnExecute() { - if (!partition_param_->partition_name().empty()) { - auto status = ValidationUtil::ValidateTableName(partition_param_->partition_name()); - if (!status.ok()) { - return status; - } - return DBWrapper::DB()->DropPartition(partition_param_->partition_name()); - } else { - auto status = ValidationUtil::ValidateTableName(partition_param_->table_name()); + std::string table_name = partition_param_->table_name(); + std::string partition_name = partition_param_->partition_name(); + std::string partition_tag = partition_param_->tag(); + if (!partition_name.empty()) { + auto status = ValidationUtil::ValidateTableName(partition_name); if (!status.ok()) { return status; } - status = ValidationUtil::ValidatePartitionTags({partition_param_->tag()}); + // check partition existence + engine::meta::TableSchema table_info; + table_info.table_id_ = partition_name; + status = DBWrapper::DB()->DescribeTable(table_info); + if (!status.ok()) { + if (status.code() == DB_NOT_FOUND) { + return Status(SERVER_TABLE_NOT_EXIST, + "Table " + table_name + "'s partition " + partition_name + " not found"); + } else { + return status; + } + } + + return DBWrapper::DB()->DropPartition(partition_name); + } else { + auto status = ValidationUtil::ValidateTableName(table_name); if (!status.ok()) { return status; } - return DBWrapper::DB()->DropPartitionByTag(partition_param_->table_name(), partition_param_->tag()); + + status = ValidationUtil::ValidatePartitionTags({partition_tag}); + if (!status.ok()) { + return status; + } + return DBWrapper::DB()->DropPartitionByTag(table_name, partition_tag); } } diff --git a/core/src/utils/ValidationUtil.cpp b/core/src/utils/ValidationUtil.cpp index 080de77e17..12b2372fc5 100644 --- a/core/src/utils/ValidationUtil.cpp +++ b/core/src/utils/ValidationUtil.cpp @@ -18,6 +18,7 @@ #include "utils/ValidationUtil.h" #include "Log.h" #include "db/engine/ExecutionEngine.h" +#include "utils/StringHelpFunctions.h" #include #ifdef MILVUS_GPU_VERSION @@ -168,11 +169,26 @@ ValidationUtil::ValidateSearchNprobe(int64_t nprobe, const engine::meta::TableSc return Status::OK(); } +Status +ValidationUtil::ValidatePartitionName(const std::string& partition_name) { + if (partition_name.empty()) { + std::string msg = "Partition name should not be empty."; + SERVER_LOG_ERROR << msg; + return Status(SERVER_INVALID_TABLE_NAME, msg); + } + + return ValidateTableName(partition_name); +} + Status ValidationUtil::ValidatePartitionTags(const std::vector& partition_tags) { - for (auto& tag : partition_tags) { - if (tag.empty()) { - std::string msg = "Invalid partition tag: " + tag + ". " + "Partition tag should not be empty."; + for (const std::string& tag : partition_tags) { + // trim side-blank of tag, only compare valid characters + // for example: " ab cd " is treated as "ab cd" + std::string valid_tag = tag; + StringHelpFunctions::TrimStringBlank(valid_tag); + if (valid_tag.empty()) { + std::string msg = "Invalid partition tag: " + valid_tag + ". " + "Partition tag should not be empty."; SERVER_LOG_ERROR << msg; return Status(SERVER_INVALID_NPROBE, msg); } diff --git a/core/src/utils/ValidationUtil.h b/core/src/utils/ValidationUtil.h index 201ccef3bd..ab32c35c40 100644 --- a/core/src/utils/ValidationUtil.h +++ b/core/src/utils/ValidationUtil.h @@ -55,6 +55,9 @@ class ValidationUtil { static Status ValidateSearchNprobe(int64_t nprobe, const engine::meta::TableSchema& table_schema); + static Status + ValidatePartitionName(const std::string& partition_name); + static Status ValidatePartitionTags(const std::vector& partition_tags); diff --git a/core/unittest/db/test_db.cpp b/core/unittest/db/test_db.cpp index d8614dd5d1..343e924e8e 100644 --- a/core/unittest/db/test_db.cpp +++ b/core/unittest/db/test_db.cpp @@ -461,6 +461,13 @@ TEST_F(DBTest, PARTITION_TEST) { stat = db_->CreatePartition(table_name, partition_name, partition_tag); ASSERT_TRUE(stat.ok()); + // not allow nested partition + stat = db_->CreatePartition(partition_name, "dumy", "dummy"); + ASSERT_FALSE(stat.ok()); + + // not allow duplicated partition + stat = db_->CreatePartition(table_name, partition_name, partition_tag); + ASSERT_FALSE(stat.ok()); std::vector xb; BuildVectors(INSERT_BATCH, xb); diff --git a/core/unittest/db/test_db_mysql.cpp b/core/unittest/db/test_db_mysql.cpp index f828431838..93eea2fd27 100644 --- a/core/unittest/db/test_db_mysql.cpp +++ b/core/unittest/db/test_db_mysql.cpp @@ -297,6 +297,14 @@ TEST_F(MySqlDBTest, PARTITION_TEST) { stat = db_->CreatePartition(table_name, partition_name, partition_tag); ASSERT_TRUE(stat.ok()); + // not allow nested partition + stat = db_->CreatePartition(partition_name, "dumy", "dummy"); + ASSERT_FALSE(stat.ok()); + + // not allow duplicated partition + stat = db_->CreatePartition(table_name, partition_name, partition_tag); + ASSERT_FALSE(stat.ok()); + std::vector xb; BuildVectors(INSERT_BATCH, xb); diff --git a/core/unittest/server/test_rpc.cpp b/core/unittest/server/test_rpc.cpp index 34d4b0ad3a..5753c68422 100644 --- a/core/unittest/server/test_rpc.cpp +++ b/core/unittest/server/test_rpc.cpp @@ -409,7 +409,7 @@ TEST_F(RpcHandlerTest, PARTITION_TEST) { partition_parm.set_partition_name(partition_name); handler->DropPartition(&context, &partition_parm, &response); - ASSERT_EQ(response.error_code(), ::grpc::Status::OK.error_code()); + ASSERT_NE(response.error_code(), ::grpc::Status::OK.error_code()); } TEST_F(RpcHandlerTest, CMD_TEST) {