From 6bb72d14a17741ee9eebd7909783713a996af73b Mon Sep 17 00:00:00 2001 From: Teon Banek Date: Thu, 30 May 2019 12:30:36 +0200 Subject: [PATCH] Use Allocator for string in TypedValue Reviewers: mtomic, llugovic, mferencevic Reviewed By: mtomic, llugovic Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D2104 --- src/glue/communication.cpp | 2 +- src/query/distributed/serialization.cpp | 2 +- .../interpret/awesome_memgraph_functions.cpp | 123 +++++++++++++----- src/query/interpret/eval.hpp | 6 +- src/query/interpreter.cpp | 14 +- src/query/typed_value.cpp | 32 ++--- src/query/typed_value.hpp | 64 +++++++-- tests/unit/distributed_interpretation.cpp | 2 +- tests/unit/query_expression_evaluator.cpp | 5 +- 9 files changed, 177 insertions(+), 73 deletions(-) diff --git a/src/glue/communication.cpp b/src/glue/communication.cpp index a0e088a05..80dd6ca72 100644 --- a/src/glue/communication.cpp +++ b/src/glue/communication.cpp @@ -54,7 +54,7 @@ Value ToBoltValue(const query::TypedValue &value) { case query::TypedValue::Type::Double: return Value(value.ValueDouble()); case query::TypedValue::Type::String: - return Value(value.ValueString()); + return Value(std::string(value.ValueString())); case query::TypedValue::Type::List: { std::vector values; values.reserve(value.ValueList().size()); diff --git a/src/query/distributed/serialization.cpp b/src/query/distributed/serialization.cpp index 0e6e1d108..6a36698fa 100644 --- a/src/query/distributed/serialization.cpp +++ b/src/query/distributed/serialization.cpp @@ -25,7 +25,7 @@ void Save(const query::TypedValue &value, slk::Builder *builder, return; case query::TypedValue::Type::String: slk::Save(static_cast(4), builder); - slk::Save(value.ValueString(), builder); + slk::Save(std::string(value.ValueString()), builder); return; case query::TypedValue::Type::List: { slk::Save(static_cast(5), builder); diff --git a/src/query/interpret/awesome_memgraph_functions.cpp b/src/query/interpret/awesome_memgraph_functions.cpp index 01fd3e6a8..02a164b8d 100644 --- a/src/query/interpret/awesome_memgraph_functions.cpp +++ b/src/query/interpret/awesome_memgraph_functions.cpp @@ -1,5 +1,6 @@ #include "query/interpret/awesome_memgraph_functions.hpp" +#include #include #include #include @@ -583,7 +584,8 @@ TypedValue Rand(TypedValue *, int64_t nargs, const EvaluationContext &, return rand_dist_(pseudo_rand_gen_); } -template +template TypedValue StringMatchOperator(TypedValue *args, int64_t nargs, const EvaluationContext &, database::GraphDbAccessor *) { @@ -609,21 +611,24 @@ TypedValue StringMatchOperator(TypedValue *args, int64_t nargs, } // Check if s1 starts with s2. -bool StartsWithPredicate(const std::string &s1, const std::string &s2) { +bool StartsWithPredicate(const TypedValue::TString &s1, + const TypedValue::TString &s2) { if (s1.size() < s2.size()) return false; return std::equal(s2.begin(), s2.end(), s1.begin()); } auto StartsWith = StringMatchOperator; // Check if s1 ends with s2. -bool EndsWithPredicate(const std::string &s1, const std::string &s2) { +bool EndsWithPredicate(const TypedValue::TString &s1, + const TypedValue::TString &s2) { if (s1.size() < s2.size()) return false; return std::equal(s2.rbegin(), s2.rend(), s1.rbegin()); } auto EndsWith = StringMatchOperator; // Check if s1 contains s2. -bool ContainsPredicate(const std::string &s1, const std::string &s2) { +bool ContainsPredicate(const TypedValue::TString &s1, + const TypedValue::TString &s2) { if (s1.size() < s2.size()) return false; return s1.find(s2) != std::string::npos; } @@ -642,7 +647,10 @@ TypedValue Assert(TypedValue *args, int64_t nargs, const EvaluationContext &, "Second argument of 'assert' must be a string."); if (!args[0].ValueBool()) { std::string message("Assertion failed"); - if (nargs == 2) message += ": " + args[1].ValueString(); + if (nargs == 2) { + message += ": "; + message += args[1].ValueString(); + } message += "."; throw QueryRuntimeException(message); } @@ -760,7 +768,10 @@ TypedValue Left(TypedValue *args, int64_t nargs, const EvaluationContext &, "Second argument of 'left' must be a non-negative integer."); case TypedValue::Type::String: if (args[1].IsInt() && args[1].ValueInt() >= 0) { - return args[0].ValueString().substr(0, args[1].ValueInt()); + auto *memory = args[0].GetMemoryResource(); + return TypedValue( + utils::Substr(args[0].ValueString(), 0, args[1].ValueInt()), + memory); } throw QueryRuntimeException( "Second argument of 'left' must be a non-negative integer."); @@ -784,8 +795,12 @@ TypedValue Right(TypedValue *args, int64_t nargs, const EvaluationContext &, case TypedValue::Type::String: { const auto &str = args[0].ValueString(); if (args[1].IsInt() && args[1].ValueInt() >= 0) { - int len = args[1].ValueInt(); - return len <= str.size() ? str.substr(str.size() - len, len) : str; + auto len = args[1].ValueInt(); + auto *memory = args[0].GetMemoryResource(); + return len <= str.size() + ? TypedValue(utils::Substr(str, str.size() - len, len), + memory) + : TypedValue(str, memory); } throw QueryRuntimeException( "Second argument of 'right' must be a non-negative integer."); @@ -796,30 +811,69 @@ TypedValue Right(TypedValue *args, int64_t nargs, const EvaluationContext &, } } -#define WRAP_STRING_FUNCTION(name, lowercased_name, function) \ - TypedValue name(TypedValue *args, int64_t nargs, const EvaluationContext &, \ - database::GraphDbAccessor *) { \ - if (nargs != 1) { \ - throw QueryRuntimeException("'" #lowercased_name \ - "' requires exactly one argument."); \ - } \ - switch (args[0].type()) { \ - case TypedValue::Type::Null: \ - return TypedValue::Null; \ - case TypedValue::Type::String: \ - return std::string(function(args[0].ValueString())); \ - default: \ - throw QueryRuntimeException("'" #lowercased_name \ - "' argument should be a string."); \ - } \ +TypedValue CallStringFunction( + TypedValue *args, int64_t nargs, const std::string &name, + std::function fun) { + if (nargs != 1) { + throw QueryRuntimeException("'" + name + + "' requires exactly one argument."); } + switch (args[0].type()) { + case TypedValue::Type::Null: + return TypedValue(args[0].GetMemoryResource()); + case TypedValue::Type::String: + return fun(args[0].ValueString()); + default: + throw QueryRuntimeException("'" + name + + "' argument should be a string."); + } +} -WRAP_STRING_FUNCTION(LTrim, lTrim, utils::LTrim); -WRAP_STRING_FUNCTION(RTrim, rTrim, utils::RTrim); -WRAP_STRING_FUNCTION(Trim, trim, utils::Trim); -WRAP_STRING_FUNCTION(Reverse, reverse, utils::Reversed); -WRAP_STRING_FUNCTION(ToLower, toLower, utils::ToLowerCase); -WRAP_STRING_FUNCTION(ToUpper, toUpper, utils::ToUpperCase); +TypedValue LTrim(TypedValue *args, int64_t nargs, const EvaluationContext &, + database::GraphDbAccessor *) { + return CallStringFunction(args, nargs, "lTrim", [](const auto &str) { + return TypedValue::TString(utils::LTrim(str), str.get_allocator()); + }); +} + +TypedValue RTrim(TypedValue *args, int64_t nargs, const EvaluationContext &, + database::GraphDbAccessor *) { + return CallStringFunction(args, nargs, "rTrim", [](const auto &str) { + return TypedValue::TString(utils::RTrim(str), str.get_allocator()); + }); +} + +TypedValue Trim(TypedValue *args, int64_t nargs, const EvaluationContext &, + database::GraphDbAccessor *) { + return CallStringFunction(args, nargs, "trim", [](const auto &str) { + return TypedValue::TString(utils::Trim(str), str.get_allocator()); + }); +} + +TypedValue Reverse(TypedValue *args, int64_t nargs, const EvaluationContext &, + database::GraphDbAccessor *) { + return CallStringFunction(args, nargs, "reverse", [](const auto &str) { + return utils::Reversed(str, str.get_allocator()); + }); +} + +TypedValue ToLower(TypedValue *args, int64_t nargs, const EvaluationContext &, + database::GraphDbAccessor *) { + return CallStringFunction(args, nargs, "toLower", [](const auto &str) { + TypedValue::TString res(str.get_allocator()); + utils::ToLowerCase(&res, str); + return res; + }); +} + +TypedValue ToUpper(TypedValue *args, int64_t nargs, const EvaluationContext &, + database::GraphDbAccessor *) { + return CallStringFunction(args, nargs, "toUpper", [](const auto &str) { + TypedValue::TString res(str.get_allocator()); + utils::ToUpperCase(&res, str); + return res; + }); +} TypedValue Replace(TypedValue *args, int64_t nargs, const EvaluationContext &, database::GraphDbAccessor *dba) { @@ -890,12 +944,13 @@ TypedValue Substring(TypedValue *args, int64_t nargs, const EvaluationContext &, return TypedValue::Null; } const auto &str = args[0].ValueString(); - int start = args[1].ValueInt(); + auto start = args[1].ValueInt(); + auto *memory = args[0].GetMemoryResource(); if (nargs == 2) { - return start < str.size() ? str.substr(start) : ""; + return TypedValue(utils::Substr(str, start), memory); } - int len = args[2].ValueInt(); - return start < str.size() ? str.substr(start, len) : ""; + auto len = args[2].ValueInt(); + return TypedValue(utils::Substr(str, start, len), memory); } #if MG_SINGLE_NODE diff --git a/src/query/interpret/eval.hpp b/src/query/interpret/eval.hpp index dfa617002..abca88edc 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.Value>(); - auto found = map.find(index.ValueString()); + auto found = map.find(std::string(index.ValueString())); if (found == map.end()) return TypedValue::Null; return found->second; } @@ -194,7 +194,7 @@ class ExpressionEvaluator : public ExpressionVisitor { throw QueryRuntimeException( "Expected a string as a property name, got {}.", index.type()); return lhs.Value().PropsAt( - dba_->Property(index.ValueString())); + dba_->Property(std::string(index.ValueString()))); } if (lhs.IsEdge()) { @@ -202,7 +202,7 @@ class ExpressionEvaluator : public ExpressionVisitor { throw QueryRuntimeException( "Expected a string as a property name, got {}.", index.type()); return lhs.Value().PropsAt( - dba_->Property(index.ValueString())); + dba_->Property(std::string(index.ValueString()))); } // lhs is Null diff --git a/src/query/interpreter.cpp b/src/query/interpreter.cpp index 8186fa43c..74457d5ab 100644 --- a/src/query/interpreter.cpp +++ b/src/query/interpreter.cpp @@ -114,9 +114,10 @@ Callback HandleAuthQuery(AuthQuery *auth_query, auth::Auth *auth, std::lock_guard lock(auth->WithLock()); auto user = auth->AddUser( - username, password.IsString() - ? std::make_optional(password.ValueString()) - : std::nullopt); + username, + password.IsString() + ? std::make_optional(std::string(password.ValueString())) + : std::nullopt); if (!user) { throw QueryRuntimeException("User or role '{}' already exists.", username); @@ -146,9 +147,10 @@ Callback HandleAuthQuery(AuthQuery *auth_query, auth::Auth *auth, if (!user) { throw QueryRuntimeException("User '{}' doesn't exist.", username); } - user->UpdatePassword(password.IsString() - ? std::make_optional(password.ValueString()) - : std::nullopt); + user->UpdatePassword( + password.IsString() + ? std::make_optional(std::string(password.ValueString())) + : std::nullopt); auth->SaveUser(*user); return std::vector>(); }; diff --git a/src/query/typed_value.cpp b/src/query/typed_value.cpp index 8d679100c..b6bc89a48 100644 --- a/src/query/typed_value.cpp +++ b/src/query/typed_value.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include "glog/logging.h" @@ -39,7 +40,7 @@ TypedValue::TypedValue(const PropertyValue &value, return; case PropertyValue::Type::String: type_ = Type::String; - new (&string_v) std::string(value.Value()); + new (&string_v) TString(value.Value(), memory_); return; case PropertyValue::Type::List: { type_ = Type::List; @@ -83,7 +84,7 @@ TypedValue::TypedValue(PropertyValue &&other, utils::MemoryResource *memory) break; case PropertyValue::Type::String: type_ = Type::String; - new (&string_v) std::string(std::move(other.Value())); + new (&string_v) TString(other.Value(), memory_); break; case PropertyValue::Type::List: { type_ = Type::List; @@ -127,7 +128,7 @@ TypedValue::TypedValue(const TypedValue &other, utils::MemoryResource *memory) this->double_v = other.double_v; return; case TypedValue::Type::String: - new (&string_v) std::string(other.string_v); + new (&string_v) TString(other.string_v, memory_); return; case Type::List: new (&list_v) TVector(other.list_v, memory_); @@ -166,7 +167,7 @@ TypedValue::TypedValue(TypedValue &&other, utils::MemoryResource *memory) this->double_v = other.double_v; break; case TypedValue::Type::String: - new (&string_v) std::string(std::move(other.string_v)); + new (&string_v) TString(std::move(other.string_v), memory_); break; case Type::List: new (&list_v) TVector(std::move(other.list_v), memory_); @@ -198,7 +199,7 @@ TypedValue::operator PropertyValue() const { case TypedValue::Type::Double: return PropertyValue(double_v); case TypedValue::Type::String: - return PropertyValue(string_v); + return PropertyValue(std::string(string_v)); case TypedValue::Type::List: return PropertyValue( std::vector(list_v.begin(), list_v.end())); @@ -242,7 +243,7 @@ TypedValue::operator PropertyValue() const { DEFINE_VALUE_AND_TYPE_GETTERS(bool, Bool, bool_v) DEFINE_VALUE_AND_TYPE_GETTERS(int64_t, Int, int_v) DEFINE_VALUE_AND_TYPE_GETTERS(double, Double, double_v) -DEFINE_VALUE_AND_TYPE_GETTERS(std::string, String, string_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(VertexAccessor, Vertex, vertex_v) @@ -346,7 +347,10 @@ DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(int, Int, int_v) DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(bool, Bool, bool_v) DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(int64_t, Int, int_v) DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(double, Double, double_v) +DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const TypedValue::TString &, String, + string_v) DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const std::string &, String, string_v) +DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const std::string_view &, String, string_v) DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const TypedValue::TVector &, List, list_v) TypedValue &TypedValue::operator=(const std::vector &other) { @@ -378,7 +382,7 @@ DEFINE_TYPED_VALUE_COPY_ASSIGNMENT(const Path &, Path, path_v) return *this; \ } -DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(std::string, String, string_v) +DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(TypedValue::TString, String, string_v) DEFINE_TYPED_VALUE_MOVE_ASSIGNMENT(TypedValue::TVector, List, list_v) TypedValue &TypedValue::operator=(std::vector &&other) { @@ -423,7 +427,7 @@ TypedValue &TypedValue::operator=(const TypedValue &other) { this->double_v = other.double_v; return *this; case TypedValue::Type::String: - new (&string_v) std::string(other.string_v); + new (&string_v) TString(other.string_v, memory_); return *this; case TypedValue::Type::List: new (&list_v) TVector(other.list_v, memory_); @@ -471,7 +475,7 @@ TypedValue &TypedValue::operator=(TypedValue &&other) noexcept(false) { this->double_v = other.double_v; break; case TypedValue::Type::String: - new (&string_v) std::string(std::move(other.string_v)); + new (&string_v) TString(std::move(other.string_v), memory_); break; case TypedValue::Type::List: new (&list_v) TVector(std::move(other.list_v), memory_); @@ -508,10 +512,7 @@ void TypedValue::DestroyValue() { // we need to call destructors for non primitive types since we used // placement new case Type::String: - // Clang fails to compile ~std::string. It seems it is a bug in some - // versions of clang. using namespace std statement solves the issue. - using namespace std; - string_v.~string(); + string_v.~TString(); break; case Type::List: list_v.~TVector(); @@ -680,7 +681,8 @@ TypedValue operator!(const TypedValue &a) { * @return A string. */ std::string ValueToString(const TypedValue &value) { - if (value.IsString()) return value.ValueString(); + // TODO: Should this allocate a string through value.GetMemoryResource()? + if (value.IsString()) return std::string(value.ValueString()); if (value.IsInt()) return std::to_string(value.Value()); if (value.IsDouble()) return fmt::format("{}", value.Value()); // unsupported situations @@ -894,7 +896,7 @@ size_t TypedValue::Hash::operator()(const TypedValue &value) const { case TypedValue::Type::Double: return std::hash{}(value.Value()); case TypedValue::Type::String: - return std::hash{}(value.ValueString()); + return std::hash{}(value.ValueString()); case TypedValue::Type::List: { return utils::FnvCollection{}( value.ValueList()); diff --git a/src/query/typed_value.hpp b/src/query/typed_value.hpp index 3af89628a..dc1a6caeb 100644 --- a/src/query/typed_value.hpp +++ b/src/query/typed_value.hpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -67,6 +68,10 @@ class TypedValue { Path }; + /** Concrete value type of character string */ + 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 @@ -150,13 +155,40 @@ class TypedValue { TypedValue(const std::string &value, utils::MemoryResource *memory = utils::NewDeleteResource()) : memory_(memory), type_(Type::String) { - new (&string_v) std::string(value); + new (&string_v) TString(value, memory_); } TypedValue(const char *value, utils::MemoryResource *memory = utils::NewDeleteResource()) : memory_(memory), type_(Type::String) { - new (&string_v) std::string(value); + new (&string_v) TString(value, memory_); + } + + explicit TypedValue( + const std::string_view &value, + utils::MemoryResource *memory = utils::NewDeleteResource()) + : memory_(memory), type_(Type::String) { + new (&string_v) TString(value, memory_); + } + + /** + * 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 TString &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 TString &other, utils::MemoryResource *memory) + : memory_(memory), type_(Type::String) { + new (&string_v) TString(other, memory_); } /** Construct a copy using the given utils::MemoryResource */ @@ -219,13 +251,23 @@ class TypedValue { TypedValue(const PropertyValue &value, utils::MemoryResource *memory); // move constructors for non-primitive types - TypedValue(std::string &&value) noexcept : type_(Type::String) { - new (&string_v) std::string(std::move(value)); - } - TypedValue(std::string &&value, utils::MemoryResource *memory) noexcept + /** + * Construct with the value of other. + * utils::MemoryResource is obtained from other. After the move, other will be + * left in unspecified state. + */ + TypedValue(TString &&other) noexcept + : TypedValue(std::move(other), + other.get_allocator().GetMemoryResource()) {} + + /** + * Construct with the value of other and use the given MemoryResource + * After the move, other will be left in unspecified state. + */ + TypedValue(TString &&other, utils::MemoryResource *memory) : memory_(memory), type_(Type::String) { - new (&string_v) std::string(std::move(value)); + new (&string_v) TString(std::move(other), memory_); } /** @@ -330,7 +372,9 @@ class TypedValue { TypedValue &operator=(bool); TypedValue &operator=(int64_t); TypedValue &operator=(double); + TypedValue &operator=(const TString &); TypedValue &operator=(const std::string &); + TypedValue &operator=(const std::string_view &); TypedValue &operator=(const TVector &); TypedValue &operator=(const std::vector &); TypedValue &operator=(const TypedValue::value_map_t &); @@ -342,7 +386,7 @@ class TypedValue { TypedValue &operator=(const TypedValue &other); // move assignment operators - TypedValue &operator=(std::string &&); + TypedValue &operator=(TString &&); TypedValue &operator=(TVector &&); TypedValue &operator=(std::vector &&); TypedValue &operator=(TypedValue::value_map_t &&); @@ -383,7 +427,7 @@ class TypedValue { DECLARE_VALUE_AND_TYPE_GETTERS(bool, Bool) DECLARE_VALUE_AND_TYPE_GETTERS(int64_t, Int) DECLARE_VALUE_AND_TYPE_GETTERS(double, Double) - DECLARE_VALUE_AND_TYPE_GETTERS(std::string, String) + DECLARE_VALUE_AND_TYPE_GETTERS(TString, String) /** * Get the list value. @@ -432,7 +476,7 @@ class TypedValue { // but most of algorithms (concatenations, serialisation...) has linear time // complexity so it shouldn't be a problem. This is maybe even faster // because of data locality. - std::string string_v; + TString string_v; TVector list_v; // clang doesn't allow unordered_map to have incomplete type as value so we // we use map. diff --git a/tests/unit/distributed_interpretation.cpp b/tests/unit/distributed_interpretation.cpp index 721397a5a..ceda076dc 100644 --- a/tests/unit/distributed_interpretation.cpp +++ b/tests/unit/distributed_interpretation.cpp @@ -153,7 +153,7 @@ TEST_F(DistributedInterpretationTest, RemoteExpandTest2) { std::vector row; row.reserve(res.size()); for (const auto &col : res) { - row.push_back(col.ValueString()); + row.emplace_back(col.ValueString()); } got.push_back(row); } diff --git a/tests/unit/query_expression_evaluator.cpp b/tests/unit/query_expression_evaluator.cpp index 817cdfd80..5f7aff4d8 100644 --- a/tests/unit/query_expression_evaluator.cpp +++ b/tests/unit/query_expression_evaluator.cpp @@ -1171,8 +1171,9 @@ TEST_F(FunctionTest, Labels) { v.add_label(dba.Label("label2")); std::vector labels; auto _labels = EvaluateFunction("LABELS", {v}).ValueList(); + labels.reserve(_labels.size()); for (auto label : _labels) { - labels.push_back(label.ValueString()); + labels.emplace_back(label.ValueString()); } ASSERT_THAT(labels, UnorderedElementsAre("label1", "label2")); ASSERT_THROW(EvaluateFunction("LABELS", {2}), QueryRuntimeException); @@ -1251,7 +1252,7 @@ TEST_F(FunctionTest, Keys) { auto prop_keys_to_string = [](TypedValue t) { std::vector keys; for (auto property : t.ValueList()) { - keys.push_back(property.ValueString()); + keys.emplace_back(property.ValueString()); } return keys; };