From 6096a96385b6c1e3ea5d0682a153f0a633435de9 Mon Sep 17 00:00:00 2001 From: Kishore Nallan Date: Mon, 8 May 2023 22:06:30 +0530 Subject: [PATCH 1/5] Verify status of ext snapshot dir copy. --- include/raft_server.h | 3 +++ src/raft_server.cpp | 19 +++++++++++++++---- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/include/raft_server.h b/include/raft_server.h index 40a09de2..8280338b 100644 --- a/include/raft_server.h +++ b/include/raft_server.h @@ -126,6 +126,7 @@ private: std::string raft_dir_path; std::string ext_snapshot_path; + std::atomic ext_snapshot_succeeded; int election_timeout_interval_ms; @@ -205,6 +206,8 @@ public: void set_ext_snapshot_path(const std::string &snapshot_path); + bool get_ext_snapshot_succeeded(); + const std::string& get_ext_snapshot_path() const; // for timed snapshots diff --git a/src/raft_server.cpp b/src/raft_server.cpp index 6c8228fd..a8432bee 100644 --- a/src/raft_server.cpp +++ b/src/raft_server.cpp @@ -429,8 +429,10 @@ void* ReplicationState::save_snapshot(void* arg) { const butil::FilePath& src_snapshot_dir = butil::FilePath(sa->state_dir_path + "/snapshot"); const butil::FilePath& src_meta_dir = butil::FilePath(sa->state_dir_path + "/meta"); - butil::CopyDirectory(src_snapshot_dir, dest_state_dir, true); - butil::CopyDirectory(src_meta_dir, dest_state_dir, true); + bool snapshot_copied = butil::CopyDirectory(src_snapshot_dir, dest_state_dir, true); + bool meta_copied = butil::CopyDirectory(src_meta_dir, dest_state_dir, true); + + sa->replication_state->ext_snapshot_succeeded = snapshot_copied && meta_copied; // notify on demand closure that external snapshotting is done sa->replication_state->notify(); @@ -949,6 +951,10 @@ void ReplicationState::do_snapshot(const std::string& nodes) { last_snapshot_ts = current_ts; } +bool ReplicationState::get_ext_snapshot_succeeded() { + return ext_snapshot_succeeded; +} + void TimedSnapshotClosure::Run() { // Auto delete this after Done() std::unique_ptr self_guard(this); @@ -973,12 +979,17 @@ void OnDemandSnapshotClosure::Run() { nlohmann::json response; uint32_t status_code; - if(status().ok()) { + if(status().ok() && replication_state->get_ext_snapshot_succeeded()) { LOG(INFO) << "On demand snapshot succeeded!"; status_code = 201; response["success"] = true; } else { - LOG(ERROR) << "On demand snapshot failed, error: " << status().error_str() << ", code: " << status().error_code(); + LOG(ERROR) << "On demand snapshot failed, error: "; + if(replication_state->get_ext_snapshot_succeeded()) { + LOG(ERROR) << status().error_str() << ", code: " << status().error_code(); + } else { + LOG(ERROR) << "Copy failed."; + } status_code = 500; response["success"] = false; response["error"] = status().error_str(); From d9f3bbb96e1bef33634a820845c83e5277d72852 Mon Sep 17 00:00:00 2001 From: Kishore Nallan Date: Tue, 9 May 2023 09:02:28 +0530 Subject: [PATCH 2/5] Move the sort index lookup outside loop. --- src/index.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/index.cpp b/src/index.cpp index 483e4fc9..c7a33928 100644 --- a/src/index.cpp +++ b/src/index.cpp @@ -1151,6 +1151,8 @@ void Index::do_facets(std::vector & facets, facet_query_t & facet_query, const auto& fquery_hashes = facet_infos[findex].hashes; const bool should_compute_stats = facet_infos[findex].should_compute_stats; + auto sort_index_it = sort_index.find(a_facet.field_name); + size_t mod_value = 100 / facet_sample_percent; facet_map_t::iterator facet_map_it; @@ -1209,14 +1211,11 @@ void Index::do_facets(std::vector & facets, facet_query_t & facet_query, compute_facet_stats(a_facet, fhash, facet_field.type); } if(a_facet.is_range_query) { - auto sort_index_it = sort_index.find(a_facet.field_name); - if(sort_index_it != sort_index.end()){ auto doc_id_val_map = sort_index_it->second; auto doc_seq_id_it = doc_id_val_map->find(doc_seq_id); if(doc_seq_id_it != doc_id_val_map->end()){ - int64_t doc_val = doc_seq_id_it->second; std::pair range_pair {}; if(a_facet.get_range(doc_val, range_pair)) { From edf2d658ca9213df81d161319a1cc2560ca1794b Mon Sep 17 00:00:00 2001 From: Kishore Nallan Date: Wed, 10 May 2023 20:34:52 +0530 Subject: [PATCH 3/5] Improve error message for nested array object numerical field. --- src/validator.cpp | 76 +++++++++++++++++- test/collection_nested_fields_test.cpp | 105 +++++++++++++++++-------- 2 files changed, 146 insertions(+), 35 deletions(-) diff --git a/src/validator.cpp b/src/validator.cpp index f66eaf9d..844ef3bf 100644 --- a/src/validator.cpp +++ b/src/validator.cpp @@ -159,11 +159,19 @@ Option validator_t::coerce_string(const DIRTY_VALUES& dirty_values, co auto& item = is_array ? array_iter.value() : document[field_name]; if(dirty_values == DIRTY_VALUES::REJECT) { + if(a_field.nested && item.is_array()) { + return Option<>(400, "Field `" + field_name + "` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well."); + } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " string."); } if(dirty_values == DIRTY_VALUES::DROP) { if(!a_field.optional) { + if(a_field.nested && item.is_array()) { + return Option<>(400, "Field `" + field_name + "` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well."); + } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " string."); } @@ -195,7 +203,7 @@ Option validator_t::coerce_string(const DIRTY_VALUES& dirty_values, co if(!a_field.optional) { if(a_field.nested && item.is_array()) { return Option<>(400, "Field `" + field_name + "` has an incorrect type. " - "Hint: field inside an array of objects must be an array type as well."); + "Hint: field inside an array of objects must be an array type as well."); } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " string."); } @@ -210,7 +218,7 @@ Option validator_t::coerce_string(const DIRTY_VALUES& dirty_values, co // COERCE_OR_REJECT / non-optional + DROP if(a_field.nested && item.is_array()) { return Option<>(400, "Field `" + field_name + "` has an incorrect type. " - "Hint: field inside an array of objects must be an array type as well."); + "Hint: field inside an array of objects must be an array type as well."); } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " string."); } @@ -226,11 +234,19 @@ Option validator_t::coerce_int32_t(const DIRTY_VALUES& dirty_values, c auto& item = is_array ? array_iter.value() : document[field_name]; if(dirty_values == DIRTY_VALUES::REJECT) { + if(a_field.nested && item.is_array()) { + return Option<>(400, "Field `" + field_name + "` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well."); + } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " int32."); } if(dirty_values == DIRTY_VALUES::DROP) { if(!a_field.optional) { + if(a_field.nested && item.is_array()) { + return Option<>(400, "Field `" + field_name + "` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well."); + } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " int32."); } @@ -261,6 +277,10 @@ Option validator_t::coerce_int32_t(const DIRTY_VALUES& dirty_values, c else { if(dirty_values == DIRTY_VALUES::COERCE_OR_DROP) { if(!a_field.optional) { + if(a_field.nested && item.is_array()) { + return Option<>(400, "Field `" + field_name + "` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well."); + } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " int32."); } @@ -272,6 +292,10 @@ Option validator_t::coerce_int32_t(const DIRTY_VALUES& dirty_values, c } } else { // COERCE_OR_REJECT / non-optional + DROP + if(a_field.nested && item.is_array()) { + return Option<>(400, "Field `" + field_name + "` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well."); + } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " int32."); } } @@ -294,11 +318,19 @@ Option validator_t::coerce_int64_t(const DIRTY_VALUES& dirty_values, c auto& item = is_array ? array_iter.value() : document[field_name]; if(dirty_values == DIRTY_VALUES::REJECT) { + if(a_field.nested && item.is_array()) { + return Option<>(400, "Field `" + field_name + "` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well."); + } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " int64."); } if(dirty_values == DIRTY_VALUES::DROP) { if(!a_field.optional) { + if(a_field.nested && item.is_array()) { + return Option<>(400, "Field `" + field_name + "` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well."); + } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " int64."); } @@ -328,6 +360,10 @@ Option validator_t::coerce_int64_t(const DIRTY_VALUES& dirty_values, c else { if(dirty_values == DIRTY_VALUES::COERCE_OR_DROP) { if(!a_field.optional) { + if(a_field.nested && item.is_array()) { + return Option<>(400, "Field `" + field_name + "` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well."); + } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " int64."); } @@ -339,6 +375,10 @@ Option validator_t::coerce_int64_t(const DIRTY_VALUES& dirty_values, c } } else { // COERCE_OR_REJECT / non-optional + DROP + if(a_field.nested && item.is_array()) { + return Option<>(400, "Field `" + field_name + "` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well."); + } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " int64."); } } @@ -353,11 +393,19 @@ Option validator_t::coerce_bool(const DIRTY_VALUES& dirty_values, cons auto& item = is_array ? array_iter.value() : document[field_name]; if(dirty_values == DIRTY_VALUES::REJECT) { + if(a_field.nested && item.is_array()) { + return Option<>(400, "Field `" + field_name + "` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well."); + } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " bool."); } if(dirty_values == DIRTY_VALUES::DROP) { if(!a_field.optional) { + if(a_field.nested && item.is_array()) { + return Option<>(400, "Field `" + field_name + "` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well."); + } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " bool."); } @@ -393,6 +441,10 @@ Option validator_t::coerce_bool(const DIRTY_VALUES& dirty_values, cons else { if(dirty_values == DIRTY_VALUES::COERCE_OR_DROP) { if(!a_field.optional) { + if(a_field.nested && item.is_array()) { + return Option<>(400, "Field `" + field_name + "` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well."); + } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " bool."); } @@ -404,6 +456,10 @@ Option validator_t::coerce_bool(const DIRTY_VALUES& dirty_values, cons } } else { // COERCE_OR_REJECT / non-optional + DROP + if(a_field.nested && item.is_array()) { + return Option<>(400, "Field `" + field_name + "` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well."); + } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " bool."); } } @@ -478,11 +534,19 @@ Option validator_t::coerce_float(const DIRTY_VALUES& dirty_values, con auto& item = is_array ? array_iter.value() : document[field_name]; if(dirty_values == DIRTY_VALUES::REJECT) { + if(a_field.nested && item.is_array()) { + return Option<>(400, "Field `" + field_name + "` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well."); + } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " float."); } if(dirty_values == DIRTY_VALUES::DROP) { if(!a_field.optional) { + if(a_field.nested && item.is_array()) { + return Option<>(400, "Field `" + field_name + "` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well."); + } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " float."); } @@ -508,6 +572,10 @@ Option validator_t::coerce_float(const DIRTY_VALUES& dirty_values, con else { if(dirty_values == DIRTY_VALUES::COERCE_OR_DROP) { if(!a_field.optional) { + if(a_field.nested && item.is_array()) { + return Option<>(400, "Field `" + field_name + "` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well."); + } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " float."); } @@ -519,6 +587,10 @@ Option validator_t::coerce_float(const DIRTY_VALUES& dirty_values, con } } else { // COERCE_OR_REJECT / non-optional + DROP + if(a_field.nested && item.is_array()) { + return Option<>(400, "Field `" + field_name + "` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well."); + } return Option<>(400, "Field `" + field_name + "` must be " + suffix + " float."); } } diff --git a/test/collection_nested_fields_test.cpp b/test/collection_nested_fields_test.cpp index e50cf0e2..4614aa1d 100644 --- a/test/collection_nested_fields_test.cpp +++ b/test/collection_nested_fields_test.cpp @@ -2542,8 +2542,9 @@ TEST_F(CollectionNestedFieldsTest, HighlightArrayOfObjects) { nlohmann::json schema = R"({ "name": "coll1", "enable_nested_fields": true, - "fields": [ - {"name": ".*", "type": "auto"} + "fields": [ + {"name": "variants", "type": "object[]", "facet": true, "index": true}, + {"name": "variants.sellingPrice", "type": "int32", "facet": true} ] })"_json; @@ -2552,45 +2553,83 @@ TEST_F(CollectionNestedFieldsTest, HighlightArrayOfObjects) { Collection* coll1 = op.get(); auto doc1 = R"({ - "details": [ - {"foo": "John Smith"}, - {"name": "James Peterson"}, - {"bar": "John Galt"} - ] + "variants": [ + { + "sellingPrice": 2300, + "timestamp": 10000, + "is_deleted": false, + "price": 50.50 + }, + { + "sellingPrice": 1200, + "timestamp": 10000, + "is_deleted": false, + "price": 150.50 + } + ] + })"_json; auto add_op = coll1->add(doc1.dump(), CREATE); - ASSERT_TRUE(add_op.ok()); + ASSERT_FALSE(add_op.ok()); + ASSERT_EQ("Field `variants.sellingPrice` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well.", add_op.error()); - auto results = coll1->search("james", {"details.name"}, "", {}, {}, {0}, 10, 1, FREQUENCY, - {true}, 1, spp::sparse_hash_set(), - spp::sparse_hash_set(), 10, "", 30, 4).get(); - ASSERT_EQ(1, results["found"].get()); - ASSERT_EQ(3, results["hits"][0]["highlight"]["details"].size()); - ASSERT_EQ(0, results["hits"][0]["highlight"]["details"][0].size()); - ASSERT_EQ(1, results["hits"][0]["highlight"]["details"][1].size()); - ASSERT_EQ(0, results["hits"][0]["highlight"]["details"][2].size()); + schema = R"({ + "name": "coll2", + "enable_nested_fields": true, + "fields": [ + {"name": "variants", "type": "object[]", "facet": true, "index": true}, + {"name": "variants.timestamp", "type": "int64", "facet": true} + ] + })"_json; - results = coll1->search("james", {"details.name"}, "", {}, {}, {0}, 10, 1, FREQUENCY, - {true}, 1, spp::sparse_hash_set(), - spp::sparse_hash_set(), 10, "", 30, 4, "", 1, {}, {}, {}, 0, - "", "", {1}, 10000, true, false, true, "details.name").get(); + op = collectionManager.create_collection(schema); + ASSERT_TRUE(op.ok()); + Collection* coll2 = op.get(); - ASSERT_EQ(1, results["found"].get()); - ASSERT_EQ(0, results["hits"][0]["highlight"]["details"][0].size()); - ASSERT_EQ(1, results["hits"][0]["highlight"]["details"][1].size()); - ASSERT_EQ(0, results["hits"][0]["highlight"]["details"][2].size()); + add_op = coll2->add(doc1.dump(), CREATE); + ASSERT_FALSE(add_op.ok()); + ASSERT_EQ("Field `variants.timestamp` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well.", add_op.error()); - results = coll1->search("james", {"details.name"}, "", {}, {}, {0}, 10, 1, FREQUENCY, - {true}, 1, spp::sparse_hash_set(), - spp::sparse_hash_set(), 10, "", 30, 4, "", 1, {}, {}, {}, 0, - "", "", {1}, 10000, true, false, true, "details").get(); + schema = R"({ + "name": "coll3", + "enable_nested_fields": true, + "fields": [ + {"name": "variants", "type": "object[]", "facet": true, "index": true}, + {"name": "variants.is_deleted", "type": "bool", "facet": true} + ] + })"_json; - ASSERT_EQ(1, results["found"].get()); - ASSERT_EQ(3, results["hits"][0]["highlight"]["details"].size()); - ASSERT_EQ(1, results["hits"][0]["highlight"]["details"][0].size()); - ASSERT_EQ(1, results["hits"][0]["highlight"]["details"][1].size()); - ASSERT_EQ(1, results["hits"][0]["highlight"]["details"][2].size()); + op = collectionManager.create_collection(schema); + ASSERT_TRUE(op.ok()); + Collection* coll3 = op.get(); + + add_op = coll3->add(doc1.dump(), CREATE); + ASSERT_FALSE(add_op.ok()); + ASSERT_EQ("Field `variants.is_deleted` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well.", add_op.error()); + + // float + + schema = R"({ + "name": "coll4", + "enable_nested_fields": true, + "fields": [ + {"name": "variants", "type": "object[]", "facet": true, "index": true}, + {"name": "variants.price", "type": "float", "facet": true} + ] + })"_json; + + op = collectionManager.create_collection(schema); + ASSERT_TRUE(op.ok()); + Collection* coll4 = op.get(); + + add_op = coll4->add(doc1.dump(), CREATE); + ASSERT_FALSE(add_op.ok()); + ASSERT_EQ("Field `variants.price` has an incorrect type. " + "Hint: field inside an array of objects must be an array type as well.", add_op.error()); } TEST_F(CollectionNestedFieldsTest, HighlightOnFlatFieldWithSnippeting) { From 8f24a3a3f182ee954afcaf1df306efd8e053890f Mon Sep 17 00:00:00 2001 From: Kishore Nallan Date: Wed, 10 May 2023 21:51:05 +0530 Subject: [PATCH 4/5] Restore HighlightArrayOfObjects test. --- test/collection_nested_fields_test.cpp | 55 ++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/test/collection_nested_fields_test.cpp b/test/collection_nested_fields_test.cpp index 4614aa1d..f4d75396 100644 --- a/test/collection_nested_fields_test.cpp +++ b/test/collection_nested_fields_test.cpp @@ -2632,6 +2632,61 @@ TEST_F(CollectionNestedFieldsTest, HighlightArrayOfObjects) { "Hint: field inside an array of objects must be an array type as well.", add_op.error()); } +TEST_F(CollectionNestedFieldsTest, HighlightArrayOfObjects) { + nlohmann::json schema = R"({ + "name": "coll1", + "enable_nested_fields": true, + "fields": [ + {"name": ".*", "type": "auto"} + ] + })"_json; + + auto op = collectionManager.create_collection(schema); + ASSERT_TRUE(op.ok()); + Collection* coll1 = op.get(); + + auto doc1 = R"({ + "details": [ + {"foo": "John Smith"}, + {"name": "James Peterson"}, + {"bar": "John Galt"} + ] + })"_json; + + auto add_op = coll1->add(doc1.dump(), CREATE); + ASSERT_TRUE(add_op.ok()); + + auto results = coll1->search("james", {"details.name"}, "", {}, {}, {0}, 10, 1, FREQUENCY, + {true}, 1, spp::sparse_hash_set(), + spp::sparse_hash_set(), 10, "", 30, 4).get(); + ASSERT_EQ(1, results["found"].get()); + ASSERT_EQ(3, results["hits"][0]["highlight"]["details"].size()); + ASSERT_EQ(0, results["hits"][0]["highlight"]["details"][0].size()); + ASSERT_EQ(1, results["hits"][0]["highlight"]["details"][1].size()); + ASSERT_EQ(0, results["hits"][0]["highlight"]["details"][2].size()); + + results = coll1->search("james", {"details.name"}, "", {}, {}, {0}, 10, 1, FREQUENCY, + {true}, 1, spp::sparse_hash_set(), + spp::sparse_hash_set(), 10, "", 30, 4, "", 1, {}, {}, {}, 0, + "", "", {1}, 10000, true, false, true, "details.name").get(); + + ASSERT_EQ(1, results["found"].get()); + ASSERT_EQ(0, results["hits"][0]["highlight"]["details"][0].size()); + ASSERT_EQ(1, results["hits"][0]["highlight"]["details"][1].size()); + ASSERT_EQ(0, results["hits"][0]["highlight"]["details"][2].size()); + + results = coll1->search("james", {"details.name"}, "", {}, {}, {0}, 10, 1, FREQUENCY, + {true}, 1, spp::sparse_hash_set(), + spp::sparse_hash_set(), 10, "", 30, 4, "", 1, {}, {}, {}, 0, + "", "", {1}, 10000, true, false, true, "details").get(); + + ASSERT_EQ(1, results["found"].get()); + ASSERT_EQ(3, results["hits"][0]["highlight"]["details"].size()); + ASSERT_EQ(1, results["hits"][0]["highlight"]["details"][0].size()); + ASSERT_EQ(1, results["hits"][0]["highlight"]["details"][1].size()); + ASSERT_EQ(1, results["hits"][0]["highlight"]["details"][2].size()); +} + TEST_F(CollectionNestedFieldsTest, HighlightOnFlatFieldWithSnippeting) { std::vector fields = {field("title", field_types::STRING, false), field("body", field_types::STRING, false)}; From fb1c1a5f6ca549b6baba8837a586faadf36ff1e4 Mon Sep 17 00:00:00 2001 From: Kishore Nallan Date: Thu, 11 May 2023 18:47:54 +0530 Subject: [PATCH 5/5] Add default select for build --- BUILD | 3 +++ 1 file changed, 3 insertions(+) diff --git a/BUILD b/BUILD index bc88a7d8..cf5a42e7 100644 --- a/BUILD +++ b/BUILD @@ -85,12 +85,15 @@ cc_binary( ], linkopts = select({ "@platforms//os:linux": ["-static-libstdc++", "-static-libgcc"], + "//conditions:default": [], }), copts = COPTS + select({ "@platforms//os:linux": ["-DBACKWARD_HAS_DW=1", "-DBACKWARD_HAS_UNWIND=1"], + "//conditions:default": [], }), deps = [":common_deps"] + select({ "@platforms//os:linux": [":linux_deps"], + "//conditions:default": [], }), )