From cbfd5bb7c5df4517af56c23f7777d8a5a914b8f0 Mon Sep 17 00:00:00 2001 From: Roman Gershman Date: Tue, 20 Feb 2024 19:58:22 +0200 Subject: [PATCH] chore: doc_accessors now parse with json::Path as well (#2615) chore: replace search path parsing with json::Path Signed-off-by: Roman Gershman --- CMakeLists.txt | 2 - src/CMakeLists.txt | 2 - src/server/json_family.cc | 28 +---------- src/server/search/doc_accessors.cc | 75 ++++++++++++++++++++++-------- 4 files changed, 58 insertions(+), 49 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index dd89dfe29..4c5fcd426 100755 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -46,8 +46,6 @@ endif() include(third_party) include(internal) -set(USE_FB2 ON CACHE BOOL "") - include_directories(src) include_directories(helio) diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 5bb885503..83885d1bf 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -188,8 +188,6 @@ endfunction() # the output file resides in the build directory. configure_file(server/version.cc.in "${CMAKE_CURRENT_SOURCE_DIR}/server/version.cc" @ONLY) -add_definitions(-DUSE_FB2) - add_subdirectory(redis) add_subdirectory(core) add_subdirectory(facade) diff --git a/src/server/json_family.cc b/src/server/json_family.cc index 8911de9b2..e44e0e958 100644 --- a/src/server/json_family.cc +++ b/src/server/json_family.cc @@ -15,9 +15,8 @@ #include "base/flags.h" #include "base/logging.h" -#include "core/json/driver.h" #include "core/json/json_object.h" -#include "core/json/jsonpath_grammar.hh" +#include "core/json/path.h" #include "facade/cmd_arg_parser.h" #include "facade/op_status.h" #include "server/acl/acl_commands_def.h" @@ -62,29 +61,6 @@ inline void Evaluate(const json::Path& expr, const JsonType& obj, ExprCallback c }); } -class JsonPathDriver : public json::Driver { - public: - string msg; - void Error(const json::location& l, const std::string& msg) final { - this->msg = absl::StrCat("Error: ", msg); - } -}; - -io::Result JsonPathV2Parse(string_view path) { - if (path.size() > 8_KB) - return nonstd::make_unexpected("Path too long"); - JsonPathDriver driver; - json::Parser parser(&driver); - - driver.SetInput(string(path)); - int res = parser(); - if (res != 0) { - return nonstd::make_unexpected(driver.msg); - } - - return driver.TakePath(); -} - inline OpStatus JsonReplaceVerifyNoOp(JsonType&) { return OpStatus::OK; } @@ -1206,7 +1182,7 @@ OpResult OpSet(const OpArgs& op_args, string_view key, string_view path, io::Result ParsePathV2(string_view path) { if (absl::GetFlag(FLAGS_jsonpathv2)) { - return JsonPathV2Parse(path); + return json::ParsePath(path); } io::Result expr_result = ParseJsonPath(path); if (!expr_result) { diff --git a/src/server/search/doc_accessors.cc b/src/server/search/doc_accessors.cc index c1e6c6490..d232e83c3 100644 --- a/src/server/search/doc_accessors.cc +++ b/src/server/search/doc_accessors.cc @@ -2,14 +2,21 @@ // See LICENSE for licensing terms. // +// GCC yields a spurious warning about uninitialized data in DocumentAccessor::StringList. +#ifdef __GNUC__ +#pragma GCC diagnostic ignored "-Wmaybe-uninitialized" +#endif + #include "server/search/doc_accessors.h" #include #include #include -#include +#include "base/flags.h" +#include "core/json/path.h" +#include "core/overloaded.h" #include "core/search/search.h" #include "core/search/vector_utils.h" #include "core/string_map.h" @@ -19,6 +26,8 @@ extern "C" { #include "redis/listpack.h" }; +ABSL_DECLARE_FLAG(bool, jsonpathv2); + namespace dfly { using namespace std; @@ -104,7 +113,25 @@ SearchDocData StringMapAccessor::Serialize(const search::Schema& schema) const { return out; } -struct JsonAccessor::JsonPathContainer : public jsoncons::jsonpath::jsonpath_expression { +struct JsonAccessor::JsonPathContainer { + vector Evaluate(const JsonType& json) const { + vector res; + + visit(Overloaded{[&](const json::Path& path) { + json::EvaluatePath(path, json, + [&](auto, const JsonType& v) { res.push_back(v); }); + }, + [&](const jsoncons::jsonpath::jsonpath_expression& path) { + auto json_arr = path.evaluate(json); + for (const auto& v : json_arr.array_range()) + res.push_back(v); + }}, + val); + + return res; + } + + variant> val; }; BaseAccessor::StringList JsonAccessor::GetStrings(string_view active_field) const { @@ -112,9 +139,7 @@ BaseAccessor::StringList JsonAccessor::GetStrings(string_view active_field) cons if (!path) return {}; - auto path_res = path->evaluate(json_); - DCHECK(path_res.is_array()); // json path always returns arrays - + auto path_res = path->Evaluate(json_); if (path_res.empty()) return {}; @@ -122,12 +147,11 @@ BaseAccessor::StringList JsonAccessor::GetStrings(string_view active_field) cons buf_ = path_res[0].as_string(); return {buf_}; } - buf_.clear(); // First, grow buffer and compute string sizes vector sizes; - for (auto element : path_res.array_range()) { + for (const auto& element : path_res) { size_t start = buf_.size(); buf_ += element.as_string(); sizes.push_back(buf_.size() - start); @@ -135,6 +159,7 @@ BaseAccessor::StringList JsonAccessor::GetStrings(string_view active_field) cons // Reposition start pointers to the most recent allocation of buf StringList out(sizes.size()); + size_t start = 0; for (size_t i = 0; i < out.size(); i++) { out[i] = string_view{buf_}.substr(start, sizes[i]); @@ -149,8 +174,7 @@ BaseAccessor::VectorInfo JsonAccessor::GetVector(string_view active_field) const if (!path) return {}; - auto res = path->evaluate(json_); - DCHECK(res.is_array()); + auto res = path->Evaluate(json_); if (res.empty()) return {nullptr, 0}; @@ -158,7 +182,7 @@ BaseAccessor::VectorInfo JsonAccessor::GetVector(string_view active_field) const auto ptr = make_unique(size); size_t i = 0; - for (auto v : res[0].array_range()) + for (const auto& v : res[0].array_range()) ptr[i++] = v.as(); return {std::move(ptr), size}; @@ -169,16 +193,29 @@ JsonAccessor::JsonPathContainer* JsonAccessor::GetPath(std::string_view field) c return it->second.get(); } - error_code ec; - auto path_expr = MakeJsonPathExpr(field, ec); - - if (ec) { - LOG(WARNING) << "Invalid Json path: " << field << ' ' << ec.message(); - return nullptr; + string ec_msg; + unique_ptr ptr; + if (absl::GetFlag(FLAGS_jsonpathv2)) { + auto path_expr = json::ParsePath(field); + if (path_expr) { + ptr.reset(new JsonPathContainer{std::move(path_expr.value())}); + } else { + ec_msg = path_expr.error(); + } + } else { + error_code ec; + auto path_expr = MakeJsonPathExpr(field, ec); + if (ec) { + ec_msg = ec.message(); + } else { + ptr.reset(new JsonPathContainer{std::move(path_expr)}); + } } - JsonPathContainer path_container{std::move(path_expr)}; - auto ptr = make_unique(std::move(path_container)); + if (!ptr) { + LOG(WARNING) << "Invalid Json path: " << field << ' ' << ec_msg; + return nullptr; + } JsonPathContainer* path = ptr.get(); path_cache_[field] = std::move(ptr); @@ -194,7 +231,7 @@ SearchDocData JsonAccessor::Serialize(const search::Schema& schema, SearchDocData out{}; for (const auto& [ident, name] : fields) { if (auto* path = GetPath(ident); path) { - if (auto res = path->evaluate(json_); !res.empty()) + if (auto res = path->Evaluate(json_); !res.empty()) out[name] = res[0].to_string(); } }