From 94ef57c459eb2d1919bfa015d81137f6007c1f93 Mon Sep 17 00:00:00 2001 From: Kostas Kyrimis Date: Wed, 30 Nov 2022 17:24:46 +0200 Subject: [PATCH] Fix small bugs --- src/storage/v3/request_helper.cpp | 3 +-- src/storage/v3/request_helper.hpp | 15 +++++------ src/storage/v3/shard_rsm.cpp | 43 +++++++++++++++++++------------ tests/simulation/shard_rsm.cpp | 21 ++++++++++----- 4 files changed, 47 insertions(+), 35 deletions(-) diff --git a/src/storage/v3/request_helper.cpp b/src/storage/v3/request_helper.cpp index 260147a25..07ea99d9d 100644 --- a/src/storage/v3/request_helper.cpp +++ b/src/storage/v3/request_helper.cpp @@ -14,7 +14,6 @@ #include #include -#include "pretty_print_ast_to_original_expression.hpp" #include "storage/v3/bindings/db_accessor.hpp" #include "storage/v3/bindings/pretty_print_ast_to_original_expression.hpp" #include "storage/v3/expr.hpp" @@ -245,7 +244,7 @@ ShardResult> CollectAllPropertiesFromAccessor(const auto pks = PrimaryKeysFromAccessor(acc, view, schema); if (pks) { - ret.GetValue().merge(*pks); + ret.GetValue().merge(std::move(*pks)); } return ret; diff --git a/src/storage/v3/request_helper.hpp b/src/storage/v3/request_helper.hpp index 13871d219..bbe4894e9 100644 --- a/src/storage/v3/request_helper.hpp +++ b/src/storage/v3/request_helper.hpp @@ -32,7 +32,7 @@ using EdgeFiller = using msgs::Value; template -concept ObjectAccessor = utils::SameAsAnyOf>; +concept OrderableObject = utils::SameAsAnyOf>; inline bool TypedValueCompare(const TypedValue &a, const TypedValue &b) { // in ordering null comes after everything else @@ -126,7 +126,7 @@ class TypedValueVectorCompare final { std::vector ordering_; }; -template +template struct Element { std::vector properties_order_by; TObjectAccessor object_acc; @@ -164,9 +164,6 @@ std::vector> OrderByVertices(DbAccessor &dba, TIterable return ordered; } -template -concept EdgeObjectAccessor = utils::SameAsAnyOf>; - std::vector> OrderByEdges(DbAccessor &dba, std::vector &iterable, std::vector &order_by_edges, const VertexAccessor &vertex_acc); @@ -198,9 +195,9 @@ std::vector EvaluateEdgeExpressions(DbAccessor &dba, const VertexAcc const std::vector &expressions); template -concept TAccessor = utils::SameAsAnyOf; +concept PropertiesAccessor = utils::SameAsAnyOf; -template +template ShardResult> CollectSpecificPropertiesFromAccessor(const TAccessor &acc, const std::vector &props, View view) { @@ -222,7 +219,7 @@ ShardResult> CollectSpecificPropertiesFromAccessor(c ShardResult> CollectAllPropertiesFromAccessor(const VertexAccessor &acc, View view, const Schemas::Schema &schema); namespace impl { -template +template ShardResult> CollectAllPropertiesImpl(const TAccessor &acc, View view) { std::map ret; auto props = acc.Properties(view); @@ -240,7 +237,7 @@ ShardResult> CollectAllPropertiesImpl(const TAccesso } } // namespace impl -template +template ShardResult> CollectAllPropertiesFromAccessor(const TAccessor &acc, View view) { return impl::CollectAllPropertiesImpl(acc, view); } diff --git a/src/storage/v3/shard_rsm.cpp b/src/storage/v3/shard_rsm.cpp index b628127af..4ea874770 100644 --- a/src/storage/v3/shard_rsm.cpp +++ b/src/storage/v3/shard_rsm.cpp @@ -517,10 +517,13 @@ msgs::WriteResponses ShardRsm::ApplyWrite(msgs::CommitRequest &&req) { msgs::ReadResponses ShardRsm::HandleRead(msgs::GetPropertiesRequest &&req) { if (!req.vertex_ids.empty() && !req.vertices_and_edges.empty()) { - auto error = CreateErrorResponse( - {common::ErrorCode::VERTEX_HAS_EDGES, std::experimental::source_location::current()}, req.transaction_id, ""); + auto shard_error = SHARD_ERROR(ErrorCode::NONEXISTENT_OBJECT); + auto error = CreateErrorResponse(shard_error, req.transaction_id, ""); return msgs::GetPropertiesResponse{.error = {}}; } + if (req.property_ids && req.property_ids->empty()) { + return {}; + } auto shard_acc = shard_->Access(req.transaction_id); auto dba = DbAccessor{&shard_acc}; @@ -535,8 +538,9 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::GetPropertiesRequest &&req) { return result; }; - auto collect_props = [&req](const VertexAccessor &v_acc, const std::optional &e_acc) { - if (req.property_ids) { + auto collect_props = [&req](const VertexAccessor &v_acc, + const std::optional &e_acc) -> ShardResult> { + if (!req.property_ids) { if (e_acc) { return CollectAllPropertiesFromAccessor(*e_acc, view); } @@ -544,9 +548,9 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::GetPropertiesRequest &&req) { } if (e_acc) { - return CollectSpecificPropertiesFromAccessor(v_acc, *req.property_ids, view); + return CollectSpecificPropertiesFromAccessor(*e_acc, *req.property_ids, view); } - return CollectSpecificPropertiesFromAccessor(*e_acc, *req.property_ids, view); + return CollectSpecificPropertiesFromAccessor(v_acc, *req.property_ids, view); }; auto find_edge = [](const VertexAccessor &v, msgs::EdgeId e) -> std::optional { @@ -577,9 +581,9 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::GetPropertiesRequest &&req) { return {maybe_id.GetError()}; } const auto &id = maybe_id.GetValue(); - std::optional e_type; + std::optional e_id; if (e_acc) { - e_type = msgs::EdgeId{e_acc->Gid().AsUint()}; + e_id = msgs::EdgeId{e_acc->Gid().AsUint()}; } msgs::VertexId v_id{msgs::Label{id.primary_label}, ConvertValueVector(id.primary_key)}; auto maybe_props = collect_props(v_acc, e_acc); @@ -587,7 +591,7 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::GetPropertiesRequest &&req) { return {maybe_props.GetError()}; } auto props = transform_props(std::move(maybe_props.GetValue())); - auto result = msgs::GetPropertiesResultRow{.vertex = std::move(v_id), .edge = e_type, .props = std::move(props)}; + auto result = msgs::GetPropertiesResultRow{.vertex = std::move(v_id), .edge = e_id, .props = std::move(props)}; if (has_expr_to_evaluate) { std::vector e_results; if (e_acc) { @@ -610,11 +614,11 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::GetPropertiesRequest &&req) { return limit; }; - auto collect_response = [get_limit, &req](auto &elements, auto result_row_functor) { + auto collect_response = [get_limit, &req](auto &elements, auto create_result_row) { msgs::GetPropertiesResponse response; const auto limit = get_limit(elements); for (size_t index = 0; index != limit; ++index) { - auto result_row = result_row_functor(elements[index]); + auto result_row = create_result_row(elements[index]); if (result_row.HasError()) { return msgs::GetPropertiesResponse{.error = CreateErrorResponse(result_row.GetError(), req.transaction_id, "")}; } @@ -626,15 +630,15 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::GetPropertiesRequest &&req) { std::vector vertices; std::vector edges; - auto parse_and_filter = [dba, &vertices](auto &cont, auto projection, auto filter, auto maybe_get_edge) mutable { - for (const auto &elem : cont) { + auto parse_and_filter = [dba, &vertices](auto &container, auto projection, auto filter, auto maybe_get_edge) mutable { + for (const auto &elem : container) { const auto &[label, pk_v] = projection(elem); auto pk = ConvertPropertyVector(pk_v); auto v_acc = dba.FindVertex(pk, view); if (!v_acc || filter(*v_acc, maybe_get_edge(elem))) { continue; } - vertices.push_back({*v_acc}); + vertices.push_back(*v_acc); } }; auto identity = [](auto &elem) { return elem; }; @@ -648,11 +652,15 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::GetPropertiesRequest &&req) { auto filter_edge = [dba, &edges, &req, find_edge](const auto &acc, const auto &edge) mutable { auto e_acc = find_edge(acc, edge); - if (!req.filter || !e_acc || !FilterOnEdge(dba, acc, *e_acc, {*req.filter})) { - return false; + if (!e_acc) { + return true; + } + + if (req.filter && !FilterOnEdge(dba, acc, *e_acc, {*req.filter})) { + return true; } edges.push_back(*e_acc); - return true; + return false; }; // Handler logic here @@ -673,6 +681,7 @@ msgs::ReadResponses ShardRsm::HandleRead(msgs::GetPropertiesRequest &&req) { return collect_response(vertices, [emplace_result_row](auto &acc) mutable { return emplace_result_row(acc, std::nullopt); }); } + if (!req.order_by.empty()) { auto elements = OrderByEdges(dba, edges, req.order_by, vertices); return collect_response(elements, [emplace_result_row](auto &element) mutable { diff --git a/tests/simulation/shard_rsm.cpp b/tests/simulation/shard_rsm.cpp index 80a5cd87d..ba36157d0 100644 --- a/tests/simulation/shard_rsm.cpp +++ b/tests/simulation/shard_rsm.cpp @@ -489,7 +489,9 @@ msgs::GetPropertiesResponse AttemptToGetProperties(ShardClient &client, std::vec std::optional order_by = std::nullopt) { msgs::GetPropertiesRequest req{}; req.transaction_id.logical_id = GetTransactionId(); - req.property_ids = std::move(properties); + if (!properties.empty()) { + req.property_ids = std::move(properties); + } if (filter_prop) { std::string filter_expr = (!edge) ? "MG_SYMBOL_NODE.prop1 >= " : "MG_SYMBOL_EDGE.e_prop = "; @@ -1294,13 +1296,15 @@ void TestGetProperties(ShardClient &client) { const auto prop_id_5 = PropertyId::FromUint(5); // Vertices { - // No properties - const auto result = AttemptToGetProperties(client, {}, {v_id, v_id_2}, {}, std::nullopt, unique_prop_val_2); + const auto result = AttemptToGetProperties(client, {}, {v_id, v_id_2}, {}); MG_ASSERT(!result.error); - MG_ASSERT(result.result_row.empty()); + MG_ASSERT(result.result_row.size() == 2); + for (const auto &elem : result.result_row) { + MG_ASSERT(elem.props.size() == 3); + } } { - // All properties + // Specific properties const auto result = AttemptToGetProperties(client, {prop_id_2, prop_id_4, prop_id_5}, {v_id, v_id_2, v_id_3}, {}); MG_ASSERT(!result.error); MG_ASSERT(!result.result_row.empty()); @@ -1393,11 +1397,14 @@ void TestGetProperties(ShardClient &client) { unique_edge_prop_id, edge_prop_val_2, {edge_type_id})); const auto edge_prop_id = PropertyId::FromUint(unique_edge_prop_id); std::vector edge_ids = {{edge_gid}, {edge_gid_2}}; - // no properties + // all properties { const auto result = AttemptToGetProperties(client, {}, {v_id_2, v_id_3}, edge_ids); MG_ASSERT(!result.error); - MG_ASSERT(result.result_row.empty()); + MG_ASSERT(result.result_row.size() == 2); + for (const auto &elem : result.result_row) { + MG_ASSERT(elem.props.size() == 1); + } } // properties for two vertices {