fix(manifest): add unit tests for manifest by cpp. (#19)

- fix serialization, deserialization bug. - fix result's value bug. - make format include test dir. Co-authored-by: Xwg <xuanwei.zhang@gatech.com>
milvus-io · Jul 13, 2023 · a9e4c21 · a9e4c21
1 parent 9903deb
commit a9e4c21
Show file tree

Hide file tree

Showing 8 changed files with 118 additions and 39 deletions.
diff --git a/cpp/Makefile b/cpp/Makefile
@@ -9,7 +9,10 @@ build:
 	conan install . -if build --build=missing
 	conan install .
 	conan build .
-
+debug:
+	conan install . -if build --build=missing -s build_type=Debug
+	conan install .
+	conan build . -s build_type=Debug
 clean:
 	rm -rf build
 
@@ -20,6 +23,7 @@ test:
 
 format:
 	find ./src -type f ! -name "*.pb.h"  -iname *.h -o -iname *.cpp | xargs clang-format -i
+	find ./test -type f ! -name "*.pb.h"  -iname *.h -o -iname *.cpp | xargs clang-format -i
 
 
 tidy-check: build

diff --git a/cpp/src/common/result.h b/cpp/src/common/result.h
@@ -73,7 +73,13 @@ class Result {
     return std::move(value_.value());
   }
 
-  Status& status() { return status_.value(); }
+  Status& status() {
+    if (!status_.has_value() && value_.has_value()) {
+      status_ = Status::OK();
+    }
+    assert(status_.has_value());
+    return status_.value();
+  }
 
   template <typename E>
   static Result<E> FromArrowResult(arrow::Result<E> arrow_result);

diff --git a/cpp/src/common/utils.cpp b/cpp/src/common/utils.cpp
@@ -21,6 +21,7 @@ Result<schema_proto::LogicType> ToProtobufType(arrow::Type::type type) {
 
 std::unique_ptr<schema_proto::KeyValueMetadata> ToProtobufMetadata(const arrow::KeyValueMetadata* metadata) {
   auto proto_metadata = std::make_unique<schema_proto::KeyValueMetadata>();
+  assert(metadata != nullptr);
   for (const auto& key : metadata->keys()) {
     proto_metadata->add_keys(key);
   }
@@ -45,6 +46,10 @@ Result<std::unique_ptr<schema_proto::Field>> ToProtobufField(const arrow::Field*
 
 Status SetTypeValues(schema_proto::DataType* proto_type, const arrow::DataType* type) {
   switch (type->id()) {
+    case arrow::Type::INT64: {
+      proto_type->set_logic_type(schema_proto::LogicType::INT64);
+      break;
+    }
     case arrow::Type::FIXED_SIZE_BINARY: {
       auto real_type = dynamic_cast<const arrow::FixedSizeBinaryType*>(type);
       auto fixed_size_binary_type = new schema_proto::FixedSizeBinaryType();

diff --git a/cpp/src/file/fragment.h b/cpp/src/file/fragment.h
@@ -33,6 +33,10 @@ class Fragment {
 
   void set_id(int64_t id);
 
+  bool operator==(const Fragment& other) const {
+    return (fragment_id_ == other.fragment_id_ && files_ == other.files_);
+  }
+
   private:
   std::int64_t fragment_id_;
   std::vector<std::string> files_;

diff --git a/cpp/src/storage/schema.cpp b/cpp/src/storage/schema.cpp
@@ -38,6 +38,7 @@ Result<std::unique_ptr<schema_proto::Schema>> Schema::ToProtobuf() {
 
 Status Schema::FromProtobuf(const schema_proto::Schema& schema) {
   ASSIGN_OR_RETURN_NOT_OK(schema_, FromProtobufSchema(schema.arrow_schema()));
+  options_ = std::make_shared<SchemaOptions>();
   options_->FromProtobuf(schema.schema_options());
   RETURN_NOT_OK(BuildScalarSchema());
   RETURN_NOT_OK(BuildVectorSchema());

diff --git a/cpp/test/CMakeLists.txt b/cpp/test/CMakeLists.txt
@@ -5,7 +5,7 @@ add_executable(
   options_test.cpp
   schema_test.cpp
   manifest_test.cpp
-)
+  space_test.cpp)
 
 target_link_libraries(
   milvus_test storage GTest::gtest_main

diff --git a/cpp/test/manifest_test.cpp b/cpp/test/manifest_test.cpp
@@ -1,44 +1,104 @@
+#include <arrow/type.h>
+#include <arrow/array/builder_primitive.h>
+#include <numeric>
 #include "gtest/gtest.h"
 #include "storage/manifest.h"
 #include "gmock/gmock.h"
 #include "google/protobuf/util/message_differencer.h"
+#include <arrow/util/key_value_metadata.h>
 
 using ::testing::ElementsAre;
 
 namespace milvus_storage {
-// TEST(ManifestTest, ManifestGetSetTest) {
-//   std::shared_ptr<SpaceOptions> options = std::make_shared<SpaceOptions>();
-//   std::shared_ptr<Schema> schema = std::make_shared<Schema>();
-//   Manifest manifest(options, schema);
-//   manifest.add_scalar_files({"scalar_file1", "scalar_file2"});
-//   manifest.add_vector_files({"vector_file1", "vector_file2"});
-//   manifest.add_delete_file("delete_file");
-//   ASSERT_THAT(manifest.scalar_files(), ElementsAre("scalar_file1", "scalar_file2"));
-//   ASSERT_THAT(manifest.vector_files(), ElementsAre("vector_file1", "vector_file2"));
-//   ASSERT_THAT(manifest.delete_files(), ElementsAre("delete_file"));
-//   ASSERT_EQ(manifest.schema(), schema);
-//   ASSERT_EQ(manifest.space_options(), options);
-// }
-//
-// TEST(ManifestTest, ManifestProtoTest) {
-//   std::shared_ptr<SpaceOptions> options = std::make_shared<SpaceOptions>();
-//   options->uri = "file:///tmp/test";
-//   std::shared_ptr<Schema> schema = std::make_shared<Schema>();
-//   Manifest manifest(options, schema);
-//   auto proto_manifest = manifest.ToProtobuf();
-//   manifest.add_scalar_files({"scalar_file"});
-//   manifest.add_vector_files({"vector_file"});
-//   manifest.add_delete_file("delete_file");
-//
-//   manifest_proto::Manifest expected_proto_manifest;
-//   expected_proto_manifest.set_allocated_options(options->ToProtobuf().release());
-//   auto status = schema->ToProtobuf();
-//   ASSERT_TRUE(status.ok());
-//   expected_proto_manifest.set_allocated_schema(status.value().release());
-//   expected_proto_manifest.add_scalar_files("scalar_file");
-//   expected_proto_manifest.add_vector_files("vector_file");
-//   expected_proto_manifest.add_delete_files("delete_file");
-//
-//   ASSERT_TRUE(google::protobuf::util::MessageDifferencer::Equals(expected_proto_manifest, proto_manifest.value()));
-// }
+TEST(ManifestTest, ManifestGetSetTest) {
+  std::shared_ptr<Options> options = std::make_shared<Options>();
+  std::shared_ptr<Schema> schema = std::make_shared<Schema>();
+  Manifest manifest(options, schema);
+
+  Fragment fragment1(1);
+  fragment1.add_file("scalar_file1");
+  fragment1.add_file("scalar_file2");
+  manifest.add_scalar_fragment(std::move(fragment1));
+
+  Fragment fragment2(2);
+  fragment2.add_file("vector_file1");
+  fragment2.add_file("vector_file2");
+
+  manifest.add_vector_fragment(std::move(fragment2));
+
+  Fragment fragment3(3);
+  fragment3.add_file("delete_file");
+  manifest.add_delete_fragment(std::move(fragment3));
+
+  ASSERT_THAT(manifest.scalar_fragments(), ElementsAre(fragment1));
+  ASSERT_THAT(manifest.vector_fragments(), ElementsAre(fragment2));
+  ASSERT_THAT(manifest.delete_fragments(), ElementsAre(fragment3));
+  ASSERT_EQ(manifest.schema(), schema);
+  ASSERT_EQ(manifest.space_options(), options);
+}
+
+TEST(ManifestTest, ManifestProtoTest) {
+  // Create Fields
+  std::shared_ptr<arrow::KeyValueMetadata> metadata = arrow::KeyValueMetadata::Make(
+      std::vector<std::string>{"key1", "key2"}, std::vector<std::string>{"value1", "value2"});
+
+  std::shared_ptr<arrow::Field> pk_field = arrow::field("pk_field", arrow::int64(), /*nullable=*/false, metadata);
+
+  std::shared_ptr<arrow::Field> ts_field = arrow::field("ts_field", arrow::int64(), /*nullable=*/false, metadata);
+
+  std::shared_ptr<arrow::Field> vec_field =
+      arrow::field("vec_field", arrow::fixed_size_binary(10), /*nullable=*/false, metadata);
+
+  // Create Arrow Schema
+  arrow::SchemaBuilder schema_builder;
+  auto status = schema_builder.AddField(pk_field);
+  ASSERT_TRUE(status.ok());
+  status = schema_builder.AddField(ts_field);
+  ASSERT_TRUE(status.ok());
+  status = schema_builder.AddField(vec_field);
+  ASSERT_TRUE(status.ok());
+  auto schema_metadata =
+      arrow::KeyValueMetadata(std::vector<std::string>{"key1", "key2"}, std::vector<std::string>{"value1", "value2"});
+  auto metadata_status = schema_builder.AddMetadata(schema_metadata);
+  ASSERT_TRUE(metadata_status.ok());
+  auto arrow_schema = schema_builder.Finish().ValueOrDie();
+
+  // Create Options
+  auto options = std::make_shared<Options>();
+  options->uri = "file:///tmp/test";
+  auto schema_options = std::make_shared<SchemaOptions>();
+  schema_options->primary_column = "pk_field";
+  schema_options->version_column = "ts_field";
+  schema_options->vector_column = "vec_field";
+
+  // Create Schema
+  auto space_schema1 = std::make_shared<Schema>(arrow_schema, schema_options);
+  auto space_schema2 = std::make_shared<Schema>(arrow_schema, schema_options);
+
+  auto sp_status = space_schema1->Validate();
+  ASSERT_TRUE(sp_status.ok());
+
+  // Create Manifest
+  Manifest manifest1(options, space_schema1);
+  // Add Fragments
+  Fragment fragment1(1);
+  fragment1.add_file("test_file1");
+  fragment1.add_file("test_file2");
+  manifest1.add_scalar_fragment(std::move(fragment1));
+  manifest1.add_vector_fragment(std::move(fragment1));
+  manifest1.add_delete_fragment(std::move(fragment1));
+  // Serialize Manifest
+  auto proto_manifest = manifest1.ToProtobuf();
+  ASSERT_TRUE(proto_manifest.ok());
+
+  // Create Manifest
+  Manifest manifest2(options, space_schema2);
+  // Deserialize Manifest
+  manifest2.FromProtobuf(proto_manifest.value());
+
+  // Compare Manifests
+  ASSERT_EQ(manifest2.delete_fragments().size(), manifest1.delete_fragments().size());
+  ASSERT_EQ(manifest2.scalar_fragments().size(), manifest1.scalar_fragments().size());
+  ASSERT_EQ(manifest2.vector_fragments().size(), manifest1.vector_fragments().size());
+}
 }  // namespace milvus_storage
diff --git a/cpp/test/space_test.cpp b/cpp/test/space_test.cpp
@@ -7,7 +7,6 @@
 #include "arrow/array/builder_binary.h"
 #include "arrow/array/builder_primitive.h"
 #include "filter/constant_filter.h"
-#include "storage/default_space.h"
 #include "gtest/gtest.h"
 
 TEST(SpaceTest, SpaceCtor) {