From 397518cf378963b8de06608800a79efa2c8c204d Mon Sep 17 00:00:00 2001 From: Harpreet Sangar Date: Thu, 25 Apr 2024 14:24:05 +0530 Subject: [PATCH] Fix crash in sort by eval destructor. (#1685) * Fix crash in sort by eval destructor. * Add test for reference sort_by. --- include/field.h | 2 +- include/filter.h | 4 +-- src/collection.cpp | 50 ++++++++++--------------------- src/index.cpp | 2 +- test/collection_join_test.cpp | 55 +++++++++++++++++++++++++++++++++++ 5 files changed, 75 insertions(+), 38 deletions(-) diff --git a/include/field.h b/include/field.h index 7d85311d..03e52acf 100644 --- a/include/field.h +++ b/include/field.h @@ -578,7 +578,7 @@ struct sort_by { }; struct eval_t { - filter_node_t* filter_trees = nullptr; + filter_node_t** filter_trees = nullptr; // Array of filter_node_t pointers. std::vector eval_ids_vec; std::vector eval_ids_count_vec; std::vector scores; diff --git a/include/filter.h b/include/filter.h index e6071eba..59d6fce4 100644 --- a/include/filter.h +++ b/include/filter.h @@ -2,8 +2,8 @@ #include #include -#include -#include +#include "tsl/htrie_map.h" +#include "json.hpp" #include "store.h" enum NUM_COMPARATOR { diff --git a/src/collection.cpp b/src/collection.cpp index 6ef051ba..ebf3f0d0 100644 --- a/src/collection.cpp +++ b/src/collection.cpp @@ -36,6 +36,11 @@ struct sort_fields_guard_t { for (auto& eval_ids: sort_by_clause.eval.eval_ids_vec) { delete [] eval_ids; } + + for (uint32_t i = 0; i < sort_by_clause.eval_expressions.size(); i++) { + delete sort_by_clause.eval.filter_trees[i]; + } + delete [] sort_by_clause.eval.filter_trees; } } @@ -1183,23 +1188,25 @@ Option Collection::validate_and_standardize_sort_fields(const std::vector< is_group_by_query, remote_embedding_timeout_ms, remote_embedding_num_tries); - if (!sort_validation_op.ok()) { - return Option(sort_validation_op.code(), "Referenced collection `" + ref_collection_name + "`: " + - sort_validation_op.error()); - } - for (auto& ref_sort_field_std: ref_sort_fields_std) { ref_sort_field_std.reference_collection_name = ref_collection_name; sort_fields_std.emplace_back(ref_sort_field_std); } + if (!sort_validation_op.ok()) { + return Option(sort_validation_op.code(), "Referenced collection `" + ref_collection_name + "`: " + + sort_validation_op.error()); + } + continue; } else if (_sort_field.name == sort_field_const::eval) { - sort_by sort_field_std(sort_field_const::eval, _sort_field.order); + sort_fields_std.emplace_back(sort_field_const::eval, _sort_field.order); + auto& sort_field_std = sort_fields_std.back(); auto const& count = _sort_field.eval_expressions.size(); - sort_field_std.eval.filter_trees = new filter_node_t[count]; - std::unique_ptr filter_trees_guard(sort_field_std.eval.filter_trees); + sort_field_std.eval.filter_trees = new filter_node_t*[count]{nullptr}; + sort_field_std.eval_expressions = _sort_field.eval_expressions; + sort_field_std.eval.scores = _sort_field.eval.scores; for (uint32_t j = 0; j < count; j++) { auto const& filter_exp = _sort_field.eval_expressions[j]; @@ -1207,23 +1214,14 @@ Option Collection::validate_and_standardize_sort_fields(const std::vector< return Option(400, "The eval expression in sort_by is empty."); } - filter_node_t* filter_tree_root = nullptr; Option parse_filter_op = filter::parse_filter_query(filter_exp, search_schema, - store, "", filter_tree_root); - std::unique_ptr filter_tree_root_guard(filter_tree_root); - + store, "", sort_field_std.eval.filter_trees[j]); if (!parse_filter_op.ok()) { return Option(parse_filter_op.code(), "Error parsing eval expression in sort_by clause."); } - - sort_field_std.eval.filter_trees[j] = std::move(*filter_tree_root); } eval_sort_count++; - sort_field_std.eval_expressions = _sort_field.eval_expressions; - sort_field_std.eval.scores = _sort_field.eval.scores; - sort_fields_std.emplace_back(sort_field_std); - filter_trees_guard.release(); continue; } @@ -1254,22 +1252,6 @@ Option Collection::validate_and_standardize_sort_fields(const std::vector< sort_field_std.name = actual_field_name; sort_field_std.text_match_buckets = std::stoll(match_parts[1]); - } else if(actual_field_name == sort_field_const::eval) { - const std::string& filter_exp = sort_field_std.name.substr(paran_start + 1, - sort_field_std.name.size() - paran_start - - 2); - if(filter_exp.empty()) { - return Option(400, "The eval expression in sort_by is empty."); - } - - Option parse_filter_op = filter::parse_filter_query(filter_exp, search_schema, - store, "", sort_field_std.eval.filter_trees); - if(!parse_filter_op.ok()) { - return Option(parse_filter_op.code(), "Error parsing eval expression in sort_by clause."); - } - - sort_field_std.name = actual_field_name; - num_sort_expressions++; } else if(actual_field_name == sort_field_const::vector_query) { const std::string& vector_query_str = sort_field_std.name.substr(paran_start + 1, sort_field_std.name.size() - paran_start - diff --git a/src/index.cpp b/src/index.cpp index 22f273dc..8eb8ee69 100644 --- a/src/index.cpp +++ b/src/index.cpp @@ -6204,7 +6204,7 @@ void Index::populate_sort_mapping(int* sort_order, std::vector& geopoint auto& eval_exp = sort_fields_std[i].eval; auto count = sort_fields_std[i].eval_expressions.size(); for (uint32_t j = 0; j < count; j++) { - auto filter_result_iterator = filter_result_iterator_t("", this, &eval_exp.filter_trees[j], + auto filter_result_iterator = filter_result_iterator_t("", this, eval_exp.filter_trees[j], search_begin_us, search_stop_us); auto filter_init_op = filter_result_iterator.init_status(); if (!filter_init_op.ok()) { diff --git a/test/collection_join_test.cpp b/test/collection_join_test.cpp index 888960c1..9f07c4ad 100644 --- a/test/collection_join_test.cpp +++ b/test/collection_join_test.cpp @@ -4373,6 +4373,42 @@ TEST_F(CollectionJoinTest, SortByReference) { ASSERT_EQ(73.5, res_obj["hits"][1]["document"].at("product_price")); // Sort by reference optional filtering. + req_params = { + {"collection", "Products"}, + {"q", "*"}, + {"query_by", "product_name"}, + {"filter_by", "$Customers(id: *)"}, + {"sort_by", "$Customers(_eval(product_available):asc)"}, + {"include_fields", "product_id, $Customers(product_price, strategy:merge)"}, + }; + search_op = collectionManager.do_search(req_params, embedded_params, json_res, now_ts); + ASSERT_FALSE(search_op.ok()); + ASSERT_EQ("Referenced collection `Customers`: Error parsing eval expression in sort_by clause.", search_op.error()); + + req_params = { + {"collection", "Products"}, + {"q", "*"}, + {"query_by", "product_name"}, + {"filter_by", "$Customers(id: *)"}, + {"sort_by", "$Customers(_eval([(): 3]):asc)"}, + {"include_fields", "product_id, $Customers(product_price, strategy:merge)"}, + }; + search_op = collectionManager.do_search(req_params, embedded_params, json_res, now_ts); + ASSERT_FALSE(search_op.ok()); + ASSERT_EQ("Referenced collection `Customers`: The eval expression in sort_by is empty.", search_op.error()); + + req_params = { + {"collection", "Products"}, + {"q", "*"}, + {"query_by", "product_name"}, + {"filter_by", "$Customers(id: *)"}, + {"sort_by", "$Customers(_eval([(customer_name: Dan && product_price: > 100): 3, (customer_name): 2]):asc)"}, + {"include_fields", "product_id, $Customers(product_price, strategy:merge)"}, + }; + search_op = collectionManager.do_search(req_params, embedded_params, json_res, now_ts); + ASSERT_FALSE(search_op.ok()); + ASSERT_EQ("Referenced collection `Customers`: Error parsing eval expression in sort_by clause.", search_op.error()); + req_params = { {"collection", "Products"}, {"q", "*"}, @@ -4411,6 +4447,25 @@ TEST_F(CollectionJoinTest, SortByReference) { ASSERT_EQ("product_b", res_obj["hits"][1]["document"].at("product_id")); ASSERT_EQ(73.5, res_obj["hits"][1]["document"].at("product_price")); + req_params = { + {"collection", "Products"}, + {"q", "*"}, + {"query_by", "product_name"}, + {"filter_by", "$Customers(customer_id: customer_a)"}, + {"sort_by", "$Customers(_eval(product_location:(48.87709, 2.33495, 1km)):desc)"}, // Closer to product_a + {"include_fields", "product_id, $Customers(product_price, strategy:merge)"}, + }; + search_op = collectionManager.do_search(req_params, embedded_params, json_res, now_ts); + ASSERT_TRUE(search_op.ok()); + + res_obj = nlohmann::json::parse(json_res); + ASSERT_EQ(2, res_obj["found"].get()); + ASSERT_EQ(2, res_obj["hits"].size()); + ASSERT_EQ("product_a", res_obj["hits"][0]["document"].at("product_id")); + ASSERT_EQ(143, res_obj["hits"][0]["document"].at("product_price")); + ASSERT_EQ("product_b", res_obj["hits"][1]["document"].at("product_id")); + ASSERT_EQ(73.5, res_obj["hits"][1]["document"].at("product_price")); + // Text search req_params = { {"collection", "Products"},