diff --git a/src/query/distributed/serialization.cpp b/src/query/distributed/serialization.cpp index 17e5ed024..ebc7318bb 100644 --- a/src/query/distributed/serialization.cpp +++ b/src/query/distributed/serialization.cpp @@ -43,7 +43,7 @@ void Save(const query::TypedValue &value, slk::Builder *builder, size_t size = map.size(); slk::Save(size, builder); for (const auto &kv : map) { - slk::Save(kv.first, builder); + slk::Save(std::string(kv.first), builder); slk::Save(kv.second, builder, versions, worker_id); } return; diff --git a/src/query/interpret/eval.hpp b/src/query/interpret/eval.hpp index 4a65d8e14..5d575c6de 100644 --- a/src/query/interpret/eval.hpp +++ b/src/query/interpret/eval.hpp @@ -184,7 +184,7 @@ class ExpressionEvaluator : public ExpressionVisitor { throw QueryRuntimeException("Expected a string as a map index, got {}.", index.type()); const auto &map = lhs.ValueMap(); - auto found = map.find(std::string(index.ValueString())); + auto found = map.find(index.ValueString()); if (found == map.end()) return TypedValue::Null; return found->second; } @@ -276,8 +276,8 @@ class ExpressionEvaluator : public ExpressionVisitor { return expression_result.Value().PropsAt( GetProperty(property_lookup.property_)); case TypedValue::Type::Map: { - auto &map = expression_result.ValueMap(); - auto found = map.find(property_lookup.property_.name); + const auto &map = expression_result.ValueMap(); + auto found = map.find(property_lookup.property_.name.c_str()); if (found == map.end()) return TypedValue::Null; return found->second; } diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index fab7bd85b..c2c2ce556 100644 --- a/src/query/plan/operator.cpp +++ b/src/query/plan/operator.cpp @@ -2035,7 +2035,8 @@ void SetProperties::SetPropertiesCursor::Set(TRecordAccessor &record, break; case TypedValue::Type::Map: { for (const auto &kv : rhs.ValueMap()) - PropsSetChecked(&record, db_.Property(kv.first), kv.second); + PropsSetChecked(&record, db_.Property(std::string(kv.first)), + kv.second); break; } default: diff --git a/src/query/typed_value.cpp b/src/query/typed_value.cpp index 7b1714904..242d14735 100644 --- a/src/query/typed_value.cpp +++ b/src/query/typed_value.cpp @@ -52,8 +52,9 @@ TypedValue::TypedValue(const PropertyValue &value, } case PropertyValue::Type::Map: { type_ = Type::Map; - auto map = value.Value>(); - new (&map_v) std::map(map.begin(), map.end()); + const auto &map = value.Value>(); + new (&map_v) TMap(memory_); + for (const auto &kv : map) map_v.emplace(kv.first, kv.second); return; } } @@ -98,9 +99,8 @@ TypedValue::TypedValue(PropertyValue &&other, utils::MemoryResource *memory) case PropertyValue::Type::Map: { type_ = Type::Map; auto &map = other.Value>(); - new (&map_v) std::map( - std::make_move_iterator(map.begin()), - std::make_move_iterator(map.end())); + new (&map_v) TMap(memory_); + for (auto &kv : map) map_v.emplace(kv.first, std::move(kv.second)); break; } } @@ -134,7 +134,7 @@ TypedValue::TypedValue(const TypedValue &other, utils::MemoryResource *memory) new (&list_v) TVector(other.list_v, memory_); return; case Type::Map: - new (&map_v) std::map(other.map_v); + new (&map_v) TMap(other.map_v, memory_); return; case Type::Vertex: new (&vertex_v) VertexAccessor(other.vertex_v); @@ -173,7 +173,7 @@ TypedValue::TypedValue(TypedValue &&other, utils::MemoryResource *memory) new (&list_v) TVector(std::move(other.list_v), memory_); break; case Type::Map: - new (&map_v) std::map(std::move(other.map_v)); + new (&map_v) TMap(std::move(other.map_v), memory_); break; case Type::Vertex: new (&vertex_v) VertexAccessor(std::move(other.vertex_v)); @@ -203,9 +203,11 @@ TypedValue::operator PropertyValue() const { case TypedValue::Type::List: return PropertyValue( std::vector(list_v.begin(), list_v.end())); - case TypedValue::Type::Map: - return PropertyValue( - std::map(map_v.begin(), map_v.end())); + case TypedValue::Type::Map: { + std::map map; + for (const auto &kv : map_v) map.emplace(kv.first, kv.second); + return PropertyValue(std::move(map)); + } default: break; } @@ -245,7 +247,7 @@ DEFINE_VALUE_AND_TYPE_GETTERS(int64_t, Int, int_v) DEFINE_VALUE_AND_TYPE_GETTERS(double, Double, double_v) DEFINE_VALUE_AND_TYPE_GETTERS(TypedValue::TString, String, string_v) DEFINE_VALUE_AND_TYPE_GETTERS(TypedValue::TVector, List, list_v) -DEFINE_VALUE_AND_TYPE_GETTERS(TypedValue::value_map_t, Map, map_v) +DEFINE_VALUE_AND_TYPE_GETTERS(TypedValue::TMap, Map, map_v) DEFINE_VALUE_AND_TYPE_GETTERS(VertexAccessor, Vertex, vertex_v) DEFINE_VALUE_AND_TYPE_GETTERS(EdgeAccessor, Edge, edge_v) DEFINE_VALUE_AND_TYPE_GETTERS(Path, Path, path_v) @@ -363,7 +365,14 @@ TypedValue &TypedValue::operator=(const std::vector &other) { return *this; } -DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const TypedValue::value_map_t &, Map, map_v) +DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const TypedValue::TMap &, Map, map_v) + +TypedValue &TypedValue::operator=( + const std::map &other) { + *this = TypedValue(other, memory_); + return *this; +} + DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const VertexAccessor &, Vertex, vertex_v) DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const EdgeAccessor &, Edge, edge_v) DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const Path &, Path, path_v) @@ -395,7 +404,13 @@ TypedValue &TypedValue::operator=(std::vector &&other) { return *this; } -DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(TypedValue::value_map_t, Map, map_v) +DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(TypedValue::TMap, Map, map_v) + +TypedValue &TypedValue::operator=(std::map &&other) { + *this = TypedValue(std::move(other), memory_); + return *this; +} + DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(VertexAccessor, Vertex, vertex_v) DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(EdgeAccessor, Edge, edge_v) DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(Path, Path, path_v) @@ -433,7 +448,7 @@ TypedValue &TypedValue::operator=(const TypedValue &other) { new (&list_v) TVector(other.list_v, memory_); return *this; case TypedValue::Type::Map: - new (&map_v) std::map(other.map_v); + new (&map_v) TMap(other.map_v, memory_); return *this; case TypedValue::Type::Vertex: new (&vertex_v) VertexAccessor(other.vertex_v); @@ -481,7 +496,7 @@ TypedValue &TypedValue::operator=(TypedValue &&other) noexcept(false) { new (&list_v) TVector(std::move(other.list_v), memory_); break; case TypedValue::Type::Map: - new (&map_v) std::map(std::move(other.map_v)); + new (&map_v) TMap(std::move(other.map_v), memory_); break; case TypedValue::Type::Vertex: new (&vertex_v) VertexAccessor(std::move(other.vertex_v)); @@ -518,8 +533,7 @@ void TypedValue::DestroyValue() { list_v.~TVector(); break; case Type::Map: - using namespace std; - map_v.~map(); + map_v.~TMap(); break; case Type::Vertex: vertex_v.~VertexAccessor(); @@ -904,7 +918,7 @@ size_t TypedValue::Hash::operator()(const TypedValue &value) const { case TypedValue::Type::Map: { size_t hash = 6543457; for (const auto &kv : value.ValueMap()) { - hash ^= std::hash{}(kv.first); + hash ^= std::hash{}(kv.first); hash ^= this->operator()(kv.second); } return hash; diff --git a/src/query/typed_value.hpp b/src/query/typed_value.hpp index dc1a6caeb..ca3858701 100644 --- a/src/query/typed_value.hpp +++ b/src/query/typed_value.hpp @@ -6,7 +6,6 @@ #include #include #include -#include #include #include @@ -72,13 +71,17 @@ class TypedValue { using TString = std::basic_string, utils::Allocator>; - /** - * Unordered set of TypedValue items. Can contain at most one Null element, - * and treats an integral and floating point value as same if they are equal - * in the floating point domain (TypedValue operator== behaves the same). - */ - using unordered_set = std::unordered_set; - using value_map_t = std::map; + // TypedValue at this exact moment of compilation is an incomplete type, and + // the standard says that instantiating a container with an incomplete type + // invokes undefined behaviour. The libstdc++-8.3.0 we are using supports + // std::map with incomplete type, but this is still murky territory. Note that + // since C++17, std::vector is explicitly said to support incomplete types. + + // Use transparent std::less which forwards to `operator<`, so that it's + // possible to use `find` with C-style (null terminated) strings without + // actually constructing (and allocating) a key. + using TMap = std::map, + utils::Allocator>>; using TVector = std::vector>; /** Allocator type so that STL containers are aware that we need one */ @@ -220,10 +223,32 @@ class TypedValue { new (&list_v) TVector(value, memory_); } + /** Construct a copy using the given utils::MemoryResource */ TypedValue(const std::map &value, utils::MemoryResource *memory = utils::NewDeleteResource()) : memory_(memory), type_(Type::Map) { - new (&map_v) std::map(value); + new (&map_v) TMap(memory_); + for (const auto &kv : value) map_v.emplace(kv.first, kv.second); + } + + /** + * Construct a copy of other. + * utils::MemoryResource is obtained by calling + * std::allocator_traits<>:: + * select_on_container_copy_construction(other.get_allocator()). + * Since we use utils::Allocator, which does not propagate, this means that + * memory_ will be the default utils::NewDeleteResource(). + */ + TypedValue(const TMap &other) + : TypedValue(other, std::allocator_traits>:: + select_on_container_copy_construction( + other.get_allocator()) + .GetMemoryResource()) {} + + /** Construct a copy using the given utils::MemoryResource */ + TypedValue(const TMap &value, utils::MemoryResource *memory) + : memory_(memory), type_(Type::Map) { + new (&map_v) TMap(value, memory_); } TypedValue(const VertexAccessor &vertex, @@ -279,7 +304,7 @@ class TypedValue { /** * Perform an element-wise move of the other and use the given MemoryResource. - * Other will be not be empty, though elements may be Null. + * Other will be not be left empty, though elements may be Null. */ TypedValue(std::vector &&other, utils::MemoryResource *memory) : memory_(memory), type_(Type::List) { @@ -314,15 +339,44 @@ class TypedValue { new (&list_v) TVector(std::move(other), memory_); } - TypedValue(std::map &&value) noexcept - : type_(Type::Map) { - new (&map_v) std::map(std::move(value)); + /** + * Perform an element-wise move using default utils::NewDeleteResource(). + * Other will not be left empty, i.e. keys will exist but their values may + * be Null. + */ + TypedValue(std::map &&other) + : TypedValue(std::move(other), utils::NewDeleteResource()) {} + + /** + * Perform an element-wise move using the given MemoryResource. + * Other will not be left empty, i.e. keys will exist but their values may + * be Null. + */ + TypedValue(std::map &&other, + utils::MemoryResource *memory) + : memory_(memory), type_(Type::Map) { + new (&map_v) TMap(memory_); + for (auto &kv : other) map_v.emplace(kv.first, std::move(kv.second)); } - TypedValue(std::map &&value, - utils::MemoryResource *memory) noexcept + /** + * Construct with the value of other. + * utils::MemoryResource is obtained from other. After the move, other will be + * left empty. + */ + TypedValue(TMap &&other) noexcept + : TypedValue(std::move(other), + other.get_allocator().GetMemoryResource()) {} + + /** + * Construct with the value of other and use the given MemoryResource. + * If `other.get_allocator() != *memory`, this call will perform an + * element-wise move and other is not guaranteed to be empty, i.e. keys may + * exist but their values may be Null. + */ + TypedValue(TMap &&other, utils::MemoryResource *memory) : memory_(memory), type_(Type::Map) { - new (&map_v) std::map(std::move(value)); + new (&map_v) TMap(std::move(other), memory_); } TypedValue(VertexAccessor &&vertex) noexcept : type_(Type::Vertex) { @@ -377,7 +431,8 @@ class TypedValue { TypedValue &operator=(const std::string_view &); TypedValue &operator=(const TVector &); TypedValue &operator=(const std::vector &); - TypedValue &operator=(const TypedValue::value_map_t &); + TypedValue &operator=(const TMap &); + TypedValue &operator=(const std::map &); TypedValue &operator=(const VertexAccessor &); TypedValue &operator=(const EdgeAccessor &); TypedValue &operator=(const Path &); @@ -389,7 +444,8 @@ class TypedValue { TypedValue &operator=(TString &&); TypedValue &operator=(TVector &&); TypedValue &operator=(std::vector &&); - TypedValue &operator=(TypedValue::value_map_t &&); + TypedValue &operator=(TMap &&); + TypedValue &operator=(std::map &&); TypedValue &operator=(VertexAccessor &&); TypedValue &operator=(EdgeAccessor &&); TypedValue &operator=(Path &&); @@ -440,7 +496,7 @@ class TypedValue { /** Check if the stored value is a list value */ bool IsList() const; - DECLARE_VALUE_AND_TYPE_GETTERS(value_map_t, Map) + DECLARE_VALUE_AND_TYPE_GETTERS(TMap, Map) DECLARE_VALUE_AND_TYPE_GETTERS(VertexAccessor, Vertex) DECLARE_VALUE_AND_TYPE_GETTERS(EdgeAccessor, Edge) DECLARE_VALUE_AND_TYPE_GETTERS(Path, Path) @@ -478,9 +534,7 @@ class TypedValue { // because of data locality. TString string_v; TVector list_v; - // clang doesn't allow unordered_map to have incomplete type as value so we - // we use map. - std::map map_v; + TMap map_v; VertexAccessor vertex_v; EdgeAccessor edge_v; Path path_v; diff --git a/tests/unit/query_expression_evaluator.cpp b/tests/unit/query_expression_evaluator.cpp index 98dcaa649..9101f3bb1 100644 --- a/tests/unit/query_expression_evaluator.cpp +++ b/tests/unit/query_expression_evaluator.cpp @@ -1009,7 +1009,7 @@ TEST_F(FunctionTest, Properties) { auto prop_values_to_int = [](TypedValue t) { std::unordered_map properties; for (auto property : t.ValueMap()) { - properties[property.first] = property.second.ValueInt(); + properties[std::string(property.first)] = property.second.ValueInt(); } return properties; }; diff --git a/tests/unit/query_plan_accumulate_aggregate.cpp b/tests/unit/query_plan_accumulate_aggregate.cpp index 5b2fc09bf..eee05c364 100644 --- a/tests/unit/query_plan_accumulate_aggregate.cpp +++ b/tests/unit/query_plan_accumulate_aggregate.cpp @@ -327,7 +327,8 @@ TEST(QueryPlan, AggregateGroupByValues) { auto context = MakeContext(storage, symbol_table, &dba); auto results = CollectProduce(*produce, &context); ASSERT_EQ(results.size(), group_by_vals.size() - 2); - TypedValue::unordered_set result_group_bys; + std::unordered_set + result_group_bys; for (const auto &row : results) { ASSERT_EQ(2, row.size()); result_group_bys.insert(row[1]); diff --git a/tests/unit/typed_value.cpp b/tests/unit/typed_value.cpp index f59db3b01..5b923727e 100644 --- a/tests/unit/typed_value.cpp +++ b/tests/unit/typed_value.cpp @@ -490,6 +490,27 @@ TEST_F(AllTypesFixture, PropagationOfMemoryOnConstruction) { EXPECT_TRUE(TypedValue::BoolEqual{}(original[i], moved[i])); EXPECT_TRUE(TypedValue::BoolEqual{}(original[i], copied[i])); } + } else if (value.type() == TypedValue::Type::Map) { + ASSERT_EQ(move_constructed_value.type(), value.type()); + const auto &original = value.ValueMap(); + const auto &moved = move_constructed_value.ValueMap(); + const auto &copied = copy_constructed_value.ValueMap(); + auto expect_allocator = [](const auto &kv, auto *memory_resource) { + EXPECT_EQ(*kv.first.get_allocator().GetMemoryResource(), + *memory_resource); + EXPECT_EQ(*kv.second.GetMemoryResource(), *memory_resource); + }; + for (const auto &kv : original) { + expect_allocator(kv, utils::NewDeleteResource()); + auto moved_it = moved.find(kv.first); + ASSERT_NE(moved_it, moved.end()); + expect_allocator(*moved_it, &monotonic_memory); + auto copied_it = copied.find(kv.first); + ASSERT_NE(copied_it, copied.end()); + expect_allocator(*copied_it, &monotonic_memory); + EXPECT_TRUE(TypedValue::BoolEqual{}(kv.second, moved_it->second)); + EXPECT_TRUE(TypedValue::BoolEqual{}(kv.second, copied_it->second)); + } } } }