From 9f4a7dcddf00a3af38cb9cd588b6fbd67544c6ef Mon Sep 17 00:00:00 2001 From: Teon Banek Date: Tue, 27 Aug 2019 14:24:33 +0200 Subject: [PATCH] Remove PropertyValue::Null Reviewers: ipaljak, mferencevic Reviewed By: ipaljak Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D2332 --- src/durability/single_node/state_delta.lcp | 4 +- src/durability/single_node_ha/state_delta.lcp | 4 +- src/glue/communication.cpp | 2 +- src/query/plan/operator.cpp | 2 +- src/query/typed_value.cpp | 4 +- src/storage/common/types/property_value.cpp | 5 +-- src/storage/common/types/property_value.hpp | 3 -- .../common/types/property_value_store.cpp | 6 +-- src/storage/common/types/slk.cpp | 2 +- src/storage/single_node/record_accessor.cpp | 8 ++-- .../single_node_ha/record_accessor.cpp | 8 ++-- tests/unit/bfs_common.hpp | 2 +- tests/unit/graph_db_accessor_index_api.cpp | 6 +-- tests/unit/property_value_store.cpp | 8 ++-- tests/unit/query_expression_evaluator.cpp | 38 +++++++++---------- .../unit/query_plan_accumulate_aggregate.cpp | 2 +- tests/unit/query_plan_bag_semantics.cpp | 2 +- tests/unit/query_pretty_print.cpp | 2 +- 18 files changed, 52 insertions(+), 56 deletions(-) diff --git a/src/durability/single_node/state_delta.lcp b/src/durability/single_node/state_delta.lcp index 8d652c193..a32ecdc41 100644 --- a/src/durability/single_node/state_delta.lcp +++ b/src/durability/single_node/state_delta.lcp @@ -42,7 +42,7 @@ cpp<# (property-name "std::string") (properties "std::vector") (property-names "std::vector") - (value "PropertyValue" :initval "PropertyValue::Null") + (value "PropertyValue") (label "::storage::Label") (label-name "std::string") (check-empty :bool)) @@ -65,7 +65,7 @@ in StateDeltas.") create-edge ;; edge_id, from_vertex_id, to_vertex_id, edge_type, edge_type_name set-property-vertex ;; vertex_id, property, property_name, property_value set-property-edge ;; edge_id, property, property_name, property_value - ;; remove property is done by setting a PropertyValue::Null + ;; remove property is done by setting a PropertyValue to Null add-label ;; vertex_id, label, label_name remove-label ;; vertex_id, label, label_name remove-vertex ;; vertex_id, check_empty diff --git a/src/durability/single_node_ha/state_delta.lcp b/src/durability/single_node_ha/state_delta.lcp index 29ffe2ce1..80ccc2f3b 100644 --- a/src/durability/single_node_ha/state_delta.lcp +++ b/src/durability/single_node_ha/state_delta.lcp @@ -37,7 +37,7 @@ cpp<# (property-name "std::string") (properties "std::vector") (property-names "std::vector") - (value "PropertyValue" :initval "PropertyValue::Null") + (value "PropertyValue") (label "::storage::Label") (label-name "std::string") (check-empty :bool)) @@ -61,7 +61,7 @@ in StateDeltas.") create-edge ;; edge_id, from_vertex_id, to_vertex_id, edge_type, edge_type_name set-property-vertex ;; vertex_id, property, property_name, property_value set-property-edge ;; edge_id, property, property_name, property_value - ;; remove property is done by setting a PropertyValue::Null + ;; remove property is done by setting a PropertyValue to Null add-label ;; vertex_id, label, label_name remove-label ;; vertex_id, label, label_name remove-vertex ;; vertex_id, check_empty diff --git a/src/glue/communication.cpp b/src/glue/communication.cpp index 8bc97c004..f50f06cbc 100644 --- a/src/glue/communication.cpp +++ b/src/glue/communication.cpp @@ -124,7 +124,7 @@ communication::bolt::Path ToBoltPath(const query::Path &path) { PropertyValue ToPropertyValue(const Value &value) { switch (value.type()) { case Value::Type::Null: - return PropertyValue::Null; + return PropertyValue(); case Value::Type::Bool: return PropertyValue(value.ValueBool()); case Value::Type::Int: diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index 9a38aee62..6bd77ba18 100644 --- a/src/query/plan/operator.cpp +++ b/src/query/plan/operator.cpp @@ -428,7 +428,7 @@ UniqueCursorPtr ScanAllByLabelPropertyValue::MakeCursor( utils::MemoryResource *mem) const { auto vertices = [this](Frame &frame, ExecutionContext &context) -> std::optionalVertices( - label_, property_, PropertyValue::Null, false))> { + label_, property_, PropertyValue(), false))> { auto *db = context.db_accessor; ExpressionEvaluator evaluator(&frame, context.symbol_table, context.evaluation_context, diff --git a/src/query/typed_value.cpp b/src/query/typed_value.cpp index 16b493572..f504c5f6a 100644 --- a/src/query/typed_value.cpp +++ b/src/query/typed_value.cpp @@ -104,7 +104,7 @@ TypedValue::TypedValue(PropertyValue &&other, utils::MemoryResource *memory) } } - other = PropertyValue::Null; + other = PropertyValue(); } TypedValue::TypedValue(const TypedValue &other) @@ -190,7 +190,7 @@ TypedValue::TypedValue(TypedValue &&other, utils::MemoryResource *memory) TypedValue::operator PropertyValue() const { switch (type_) { case TypedValue::Type::Null: - return PropertyValue::Null; + return PropertyValue(); case TypedValue::Type::Bool: return PropertyValue(bool_v); case TypedValue::Type::Int: diff --git a/src/storage/common/types/property_value.cpp b/src/storage/common/types/property_value.cpp index 04728ada1..fe585e795 100644 --- a/src/storage/common/types/property_value.cpp +++ b/src/storage/common/types/property_value.cpp @@ -113,14 +113,13 @@ PropertyValue &PropertyValue::operator=(PropertyValue &&other) noexcept { } // reset the type of other - other = PropertyValue::Null; + other.DestroyValue(); + other.type_ = Type::Null; } return *this; } -const PropertyValue PropertyValue::Null = PropertyValue(); - void PropertyValue::DestroyValue() { switch (type_) { // destructor for primitive types does nothing diff --git a/src/storage/common/types/property_value.hpp b/src/storage/common/types/property_value.hpp index cff638713..a6f97e916 100644 --- a/src/storage/common/types/property_value.hpp +++ b/src/storage/common/types/property_value.hpp @@ -29,9 +29,6 @@ class PropertyValue { /** A value type. Each type corresponds to exactly one C++ type */ enum class Type : unsigned { Null, String, Bool, Int, Double, List, Map }; - // single static reference to Null, used whenever Null should be returned - static const PropertyValue Null; - /** Checks if the given PropertyValue::Types are comparable */ static bool AreComparableTypes(Type a, Type b) { auto is_numeric = [](const Type t) { diff --git a/src/storage/common/types/property_value_store.cpp b/src/storage/common/types/property_value_store.cpp index 087aebf4c..ea7d47804 100644 --- a/src/storage/common/types/property_value_store.cpp +++ b/src/storage/common/types/property_value_store.cpp @@ -59,7 +59,7 @@ PropertyValue PropertyValueStore::at(const Property &key) const { auto GetValue = [&key](const auto &props) { for (const auto &kv : props) if (kv.first == key) return kv.second; - return PropertyValue::Null; + return PropertyValue(); }; if (key.Location() == Location::Memory) return GetValue(props_); @@ -72,7 +72,7 @@ PropertyValue PropertyValueStore::at(const Property &key) const { DiskKey(std::to_string(version_key_), std::to_string(key.Id())); auto serialized_prop = DiskStorage().Get(disk_key); if (serialized_prop) return DeserializeProp(serialized_prop.value()); - return PropertyValue::Null; + return PropertyValue(); } void PropertyValueStore::set(const Property &key, const char *value) { @@ -228,7 +228,7 @@ PropertyValue PropertyValueStore::DeserializeProp( Value dv; if (!decoder.ReadValue(&dv)) { DLOG(WARNING) << "Unable to read property value"; - return PropertyValue::Null; + return PropertyValue(); } return glue::ToPropertyValue(dv); } diff --git a/src/storage/common/types/slk.cpp b/src/storage/common/types/slk.cpp index e21e18840..499a3db49 100644 --- a/src/storage/common/types/slk.cpp +++ b/src/storage/common/types/slk.cpp @@ -51,7 +51,7 @@ void Load(PropertyValue *value, slk::Reader *reader) { slk::Load(&type, reader); switch (type) { case static_cast(0): - *value = PropertyValue::Null; + *value = PropertyValue(); return; case static_cast(1): { bool v; diff --git a/src/storage/single_node/record_accessor.cpp b/src/storage/single_node/record_accessor.cpp index bb4bdd730..fba12f206 100644 --- a/src/storage/single_node/record_accessor.cpp +++ b/src/storage/single_node/record_accessor.cpp @@ -47,9 +47,9 @@ void RecordAccessor::PropsErase(storage::Property key) { auto &dba = db_accessor(); auto delta = StateDelta::PropsSetVertex(dba.transaction_id(), gid(), key, - dba.PropertyName(key), PropertyValue::Null); + dba.PropertyName(key), PropertyValue()); auto previous_value = PropsAt(key); - update().properties_.set(key, PropertyValue::Null); + update().properties_.set(key, PropertyValue()); dba.UpdateOnRemoveProperty(key, previous_value, *this, &update()); dba.wal().Emplace(delta); } @@ -59,8 +59,8 @@ void RecordAccessor::PropsErase(storage::Property key) { auto &dba = db_accessor(); auto delta = StateDelta::PropsSetEdge(dba.transaction_id(), gid(), key, - dba.PropertyName(key), PropertyValue::Null); - update().properties_.set(key, PropertyValue::Null); + dba.PropertyName(key), PropertyValue()); + update().properties_.set(key, PropertyValue()); db_accessor().wal().Emplace(delta); } diff --git a/src/storage/single_node_ha/record_accessor.cpp b/src/storage/single_node_ha/record_accessor.cpp index 48dc65518..f319a7863 100644 --- a/src/storage/single_node_ha/record_accessor.cpp +++ b/src/storage/single_node_ha/record_accessor.cpp @@ -47,9 +47,9 @@ void RecordAccessor::PropsErase(storage::Property key) { auto &dba = db_accessor(); auto delta = StateDelta::PropsSetVertex(dba.transaction_id(), gid(), key, - dba.PropertyName(key), PropertyValue::Null); + dba.PropertyName(key), PropertyValue()); auto previous_value = PropsAt(key); - update().properties_.set(key, PropertyValue::Null); + update().properties_.set(key, PropertyValue()); dba.UpdateOnRemoveProperty(key, previous_value, *this, &update()); dba.raft()->Emplace(delta); } @@ -59,8 +59,8 @@ void RecordAccessor::PropsErase(storage::Property key) { auto &dba = db_accessor(); auto delta = StateDelta::PropsSetEdge(dba.transaction_id(), gid(), key, - dba.PropertyName(key), PropertyValue::Null); - update().properties_.set(key, PropertyValue::Null); + dba.PropertyName(key), PropertyValue()); + update().properties_.set(key, PropertyValue()); dba.raft()->Emplace(delta); } diff --git a/tests/unit/bfs_common.hpp b/tests/unit/bfs_common.hpp index 5fc4400ae..083b40f66 100644 --- a/tests/unit/bfs_common.hpp +++ b/tests/unit/bfs_common.hpp @@ -358,7 +358,7 @@ void BfsTest(Database *db, int lower_bound, int upper_bound, input_op = YieldEntities(dba_ptr.get(), vertices, edges, blocked_sym, nullptr); filter_expr = IF(AND(NEQ(inner_node, blocked), NEQ(inner_edge, blocked)), - LITERAL(true), LITERAL(PropertyValue::Null)); + LITERAL(true), LITERAL(PropertyValue())); break; case FilterLambdaType::USE_CTX: // We only block vertex #5 and run BFS. diff --git a/tests/unit/graph_db_accessor_index_api.cpp b/tests/unit/graph_db_accessor_index_api.cpp index 99ce98d2f..46a8f1cb7 100644 --- a/tests/unit/graph_db_accessor_index_api.cpp +++ b/tests/unit/graph_db_accessor_index_api.cpp @@ -415,11 +415,11 @@ TEST_F(GraphDbAccessorIndexRange, RangeIterationCurrentState) { TEST_F(GraphDbAccessorIndexRange, RangeInterationIncompatibleTypes) { using std::nullopt; - // using PropertyValue::Null as a bound fails with an assertion + // using PropertyValue set to Null as a bound fails with an assertion ::testing::FLAGS_gtest_death_test_style = "threadsafe"; - EXPECT_DEATH(Vertices(nullopt, Inclusive(PropertyValue::Null)), + EXPECT_DEATH(Vertices(nullopt, Inclusive(PropertyValue())), "not a valid index bound"); - EXPECT_DEATH(Vertices(Inclusive(PropertyValue::Null), nullopt), + EXPECT_DEATH(Vertices(Inclusive(PropertyValue()), nullopt), "not a valid index bound"); std::vector incompatible_with_int{ "string", true, std::vector{1}}; diff --git a/tests/unit/property_value_store.cpp b/tests/unit/property_value_store.cpp index 1eff609e8..035738690 100644 --- a/tests/unit/property_value_store.cpp +++ b/tests/unit/property_value_store.cpp @@ -94,10 +94,10 @@ TEST_F(PropertyValueStoreTest, AtNull) { } TEST_F(PropertyValueStoreTest, SetNull) { - Set(11, Location::Memory, PropertyValue::Null); + Set(11, Location::Memory, PropertyValue()); EXPECT_EQ(0, props_.size()); - Set(100, Location::Disk, PropertyValue::Null); + Set(100, Location::Disk, PropertyValue()); EXPECT_EQ(0, props_.size()); } @@ -232,7 +232,7 @@ TEST_F(PropertyValueStoreTest, Size) { TEST_F(PropertyValueStoreTest, InsertRetrieveListMemory) { Set(0, Location::Memory, std::vector{1, true, 2.5, "something", - PropertyValue::Null}); + PropertyValue()}); auto p = At(0, Location::Memory); EXPECT_EQ(p.type(), PropertyValue::Type::List); @@ -252,7 +252,7 @@ TEST_F(PropertyValueStoreTest, InsertRetrieveListMemory) { TEST_F(PropertyValueStoreTest, InsertRetrieveListDisk) { Set(0, Location::Disk, std::vector{1, true, 2.5, "something", - PropertyValue::Null}); + PropertyValue()}); auto p = At(0, Location::Disk); EXPECT_EQ(p.type(), PropertyValue::Type::List); diff --git a/tests/unit/query_expression_evaluator.cpp b/tests/unit/query_expression_evaluator.cpp index 1c5915476..567210db2 100644 --- a/tests/unit/query_expression_evaluator.cpp +++ b/tests/unit/query_expression_evaluator.cpp @@ -121,20 +121,20 @@ TEST_F(ExpressionEvaluatorTest, AndOperatorNull) { { // Null doesn't short circuit auto *op = storage.Create( - storage.Create(PropertyValue::Null), + storage.Create(PropertyValue()), storage.Create(5)); EXPECT_THROW(Eval(op), QueryRuntimeException); } { auto *op = storage.Create( - storage.Create(PropertyValue::Null), + storage.Create(PropertyValue()), storage.Create(true)); auto value = Eval(op); EXPECT_TRUE(value.IsNull()); } { auto *op = storage.Create( - storage.Create(PropertyValue::Null), + storage.Create(PropertyValue()), storage.Create(false)); auto value = Eval(op); ASSERT_TRUE(value.IsBool()); @@ -295,7 +295,7 @@ TEST_F(ExpressionEvaluatorTest, InListOperator) { } { auto *list_literal = storage.Create(std::vector{ - storage.Create(PropertyValue::Null), + storage.Create(PropertyValue()), storage.Create(2), storage.Create("a")}); // Element doesn't exist in list with null element. @@ -308,21 +308,21 @@ TEST_F(ExpressionEvaluatorTest, InListOperator) { // Null list. auto *op = storage.Create( storage.Create("x"), - storage.Create(PropertyValue::Null)); + storage.Create(PropertyValue())); auto value = Eval(op); EXPECT_TRUE(value.IsNull()); } { // Null literal. auto *op = storage.Create( - storage.Create(PropertyValue::Null), list_literal); + storage.Create(PropertyValue()), list_literal); auto value = Eval(op); EXPECT_TRUE(value.IsNull()); } { // Null literal, empty list. auto *op = storage.Create( - storage.Create(PropertyValue::Null), + storage.Create(PropertyValue()), storage.Create(std::vector())); auto value = Eval(op); EXPECT_FALSE(value.ValueBool()); @@ -365,7 +365,7 @@ TEST_F(ExpressionEvaluatorTest, ListIndexing) { { // Indexing with one operator being null. auto *op = storage.Create( - storage.Create(PropertyValue::Null), + storage.Create(PropertyValue()), storage.Create(-2)); auto value = Eval(op); EXPECT_TRUE(value.IsNull()); @@ -407,7 +407,7 @@ TEST_F(ExpressionEvaluatorTest, MapIndexing) { { // Indexing with Null. auto *op = storage.Create( - map_literal, storage.Create(PropertyValue::Null)); + map_literal, storage.Create(PropertyValue())); auto value = Eval(op); EXPECT_TRUE(value.IsNull()); } @@ -460,12 +460,12 @@ TEST_F(ExpressionEvaluatorTest, VertexAndEdgeIndexing) { { // Indexing with Null. auto *op1 = storage.Create( - vertex_id, storage.Create(PropertyValue::Null)); + vertex_id, storage.Create(PropertyValue())); auto value1 = Eval(op1); EXPECT_TRUE(value1.IsNull()); auto *op2 = storage.Create( - edge_id, storage.Create(PropertyValue::Null)); + edge_id, storage.Create(PropertyValue())); auto value2 = Eval(op2); EXPECT_TRUE(value2.IsNull()); } @@ -533,7 +533,7 @@ TEST_F(ExpressionEvaluatorTest, ListSlicingOperator) { { // Bound of illegal type and null value bound. auto *op = storage.Create( - list_literal, storage.Create(PropertyValue::Null), + list_literal, storage.Create(PropertyValue()), storage.Create("mirko")); EXPECT_THROW(Eval(op), QueryRuntimeException); } @@ -547,7 +547,7 @@ TEST_F(ExpressionEvaluatorTest, ListSlicingOperator) { { // Null value list with undefined upper bound. auto *op = storage.Create( - storage.Create(PropertyValue::Null), + storage.Create(PropertyValue()), storage.Create(-2), nullptr); auto value = Eval(op); EXPECT_TRUE(value.IsNull()); @@ -557,7 +557,7 @@ TEST_F(ExpressionEvaluatorTest, ListSlicingOperator) { // Null value index. auto *op = storage.Create( list_literal, storage.Create(-2), - storage.Create(PropertyValue::Null)); + storage.Create(PropertyValue())); auto value = Eval(op); EXPECT_TRUE(value.IsNull()); ; @@ -622,7 +622,7 @@ TEST_F(ExpressionEvaluatorTest, IsNullOperator) { auto val1 = Eval(op); ASSERT_EQ(val1.ValueBool(), false); op = storage.Create( - storage.Create(PropertyValue::Null)); + storage.Create(PropertyValue())); auto val2 = Eval(op); ASSERT_EQ(val2.ValueBool(), true); } @@ -712,7 +712,7 @@ TEST_F(ExpressionEvaluatorTest, All) { TEST_F(ExpressionEvaluatorTest, FunctionAllNullList) { AstStorage storage; - auto *all = ALL("x", LITERAL(PropertyValue::Null), WHERE(LITERAL(true))); + auto *all = ALL("x", LITERAL(PropertyValue()), WHERE(LITERAL(true))); const auto x_sym = symbol_table.CreateSymbol("x", true); all->identifier_->MapTo(x_sym); auto value = Eval(all); @@ -756,7 +756,7 @@ TEST_F(ExpressionEvaluatorTest, FunctionSingle2) { TEST_F(ExpressionEvaluatorTest, FunctionSingleNullList) { AstStorage storage; auto *single = - SINGLE("x", LITERAL(PropertyValue::Null), WHERE(LITERAL(true))); + SINGLE("x", LITERAL(PropertyValue()), WHERE(LITERAL(true))); const auto x_sym = symbol_table.CreateSymbol("x", true); single->identifier_->MapTo(x_sym); auto value = Eval(single); @@ -784,7 +784,7 @@ TEST_F(ExpressionEvaluatorTest, FunctionExtract) { AstStorage storage; auto *ident_x = IDENT("x"); auto *extract = - EXTRACT("x", LIST(LITERAL(1), LITERAL(2), LITERAL(PropertyValue::Null)), + EXTRACT("x", LIST(LITERAL(1), LITERAL(2), LITERAL(PropertyValue())), ADD(ident_x, LITERAL(1))); const auto x_sym = symbol_table.CreateSymbol("x", true); extract->identifier_->MapTo(x_sym); @@ -802,7 +802,7 @@ TEST_F(ExpressionEvaluatorTest, FunctionExtractNull) { AstStorage storage; auto *ident_x = IDENT("x"); auto *extract = - EXTRACT("x", LITERAL(PropertyValue::Null), ADD(ident_x, LITERAL(1))); + EXTRACT("x", LITERAL(PropertyValue()), ADD(ident_x, LITERAL(1))); const auto x_sym = symbol_table.CreateSymbol("x", true); extract->identifier_->MapTo(x_sym); ident_x->MapTo(x_sym); diff --git a/tests/unit/query_plan_accumulate_aggregate.cpp b/tests/unit/query_plan_accumulate_aggregate.cpp index 293881bb6..17e040a5a 100644 --- a/tests/unit/query_plan_accumulate_aggregate.cpp +++ b/tests/unit/query_plan_accumulate_aggregate.cpp @@ -301,7 +301,7 @@ TEST(QueryPlan, AggregateGroupByValues) { group_by_vals.emplace_back(std::vector{1}); group_by_vals.emplace_back(std::vector{1, 2}); group_by_vals.emplace_back(std::vector{2, 1}); - group_by_vals.emplace_back(PropertyValue::Null); + group_by_vals.emplace_back(PropertyValue()); // should NOT result in another group because 7.0 == 7 group_by_vals.emplace_back(7.0); // should NOT result in another group diff --git a/tests/unit/query_plan_bag_semantics.cpp b/tests/unit/query_plan_bag_semantics.cpp index ef7d01030..9a3b85ef1 100644 --- a/tests/unit/query_plan_bag_semantics.cpp +++ b/tests/unit/query_plan_bag_semantics.cpp @@ -115,7 +115,7 @@ TEST(QueryPlan, OrderBy) { // contains a series of tests // each test defines the ordering a vector of values in the desired order - auto Null = PropertyValue::Null; + auto Null = PropertyValue(); std::vector>> orderable{ {Ordering::ASC, {0, 0, 0.5, 1, 2, 12.6, 42, Null, Null}}, {Ordering::ASC, {false, false, true, true, Null, Null}}, diff --git a/tests/unit/query_pretty_print.cpp b/tests/unit/query_pretty_print.cpp index 8a8a3b7c9..74aba00ca 100644 --- a/tests/unit/query_pretty_print.cpp +++ b/tests/unit/query_pretty_print.cpp @@ -42,7 +42,7 @@ TEST_F(ExpressionPrettyPrinterTest, Literals) { // [1 null "hello"] EXPECT_EQ(ToString(LITERAL( - (std::vector{1, PropertyValue::Null, "hello"}))), + (std::vector{1, PropertyValue(), "hello"}))), "[1, null, \"hello\"]"); // {hello: 1, there: 2}