Merge pull request #426 from yhmo/0.6.0

#377 Create partition success if tag name only contains spaces

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

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::vector<std::
     auto status = meta_ptr_->ShowPartitions(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_);
             }
         }

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 <mysql++/mysql++.h>
 #include <string.h>
@@ -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");
     }
 
-    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");
-        }
+    // 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 == "") {
+        // 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());

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 <sqlite_orm.h>
 #include <unistd.h>
@@ -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");
     }
 
-    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");
-        }
+    // 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 == "") {
+        // 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;
+
+    status = CreateTable(table_schema);
+    if (status.code() == DB_ALREADY_EXIST) {
+        return Status(DB_ALREADY_EXIST, "Partition already exists");
+    }
 
-    return CreateTable(table_schema);
+    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());

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;
         }

core/src/server/grpc_impl/request/DropPartitionRequest.cpp

@@ -22,6 +22,7 @@
 #include "utils/ValidationUtil.h"
 
 #include <memory>
+#include <string>
 
 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());
+    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;
         }
-        return DBWrapper::DB()->DropPartition(partition_param_->partition_name());
+
+        // 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(partition_param_->table_name());
+        auto status = ValidationUtil::ValidateTableName(table_name);
         if (!status.ok()) {
             return status;
         }
 
-        status = ValidationUtil::ValidatePartitionTags({partition_param_->tag()});
+        status = ValidationUtil::ValidatePartitionTags({partition_tag});
         if (!status.ok()) {
             return status;
         }
-        return DBWrapper::DB()->DropPartitionByTag(partition_param_->table_name(), partition_param_->tag());
+        return DBWrapper::DB()->DropPartitionByTag(table_name, partition_tag);
     }
 }
 

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 <arpa/inet.h>
 #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<std::string>& 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);
         }

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<std::string>& partition_tags);
 

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<float> xb;
         BuildVectors(INSERT_BATCH, xb);

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<float> xb;
         BuildVectors(INSERT_BATCH, xb);

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) {