diff options
17 files changed, 154 insertions, 159 deletions
diff --git a/jrt_test/src/tests/rpc-error/CMakeLists.txt b/jrt_test/src/tests/rpc-error/CMakeLists.txt index a86289c73bb..b0aab8a0eb9 100644 --- a/jrt_test/src/tests/rpc-error/CMakeLists.txt +++ b/jrt_test/src/tests/rpc-error/CMakeLists.txt @@ -3,6 +3,7 @@ vespa_add_executable(jrt_test_test-errors_app TEST SOURCES test-errors.cpp DEPENDS + GTest::gtest ) vespa_add_test(NAME jrt_test_test-errors_app NO_VALGRIND COMMAND ${CMAKE_CURRENT_SOURCE_DIR}/rpc-error_test.sh DEPENDS jrt_test_test-errors_app) diff --git a/jrt_test/src/tests/rpc-error/test-errors.cpp b/jrt_test/src/tests/rpc-error/test-errors.cpp index f7b8ff10185..cecdd09d718 100644 --- a/jrt_test/src/tests/rpc-error/test-errors.cpp +++ b/jrt_test/src/tests/rpc-error/test-errors.cpp @@ -1,171 +1,156 @@ // Copyright Vespa.ai. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root. -#include <vespa/vespalib/testkit/testapp.h> + #include <vespa/fnet/frt/supervisor.h> #include <vespa/fnet/frt/target.h> #include <vespa/fnet/frt/rpcrequest.h> +#include <vespa/vespalib/gtest/gtest.h> +#include <vespa/vespalib/util/ref_counted.h> + +vespalib::string spec; -class TestErrors : public vespalib::TestApp +class TestErrors : public ::testing::Test { -private: - fnet::frt::StandaloneFRT server; - FRT_Supervisor *client; - FRT_Target *target; +protected: + static std::unique_ptr<fnet::frt::StandaloneFRT> server; + static vespalib::ref_counted<FRT_Target> target; + vespalib::ref_counted<FRT_RPCRequest> alloc_rpc_request() { + return vespalib::ref_counted<FRT_RPCRequest>::internal_attach(server->supervisor().AllocRPCRequest()); + } public: TestErrors(); ~TestErrors() override; - void init(const char *spec) { - client = & server.supervisor(); - target = client->GetTarget(spec); - } - void fini() { - target->internal_subref(); - target = nullptr; - client = nullptr; - } - - void testNoError(); - void testNoSuchMethod(); - void testWrongParameters(); - void testWrongReturnValues(); - void testMethodFailed(); - int Main() override; + static void SetUpTestSuite(); + static void TearDownTestSuite(); }; +std::unique_ptr<fnet::frt::StandaloneFRT> TestErrors::server; +vespalib::ref_counted<FRT_Target> TestErrors::target; + TestErrors::TestErrors() = default; TestErrors::~TestErrors() = default; void -TestErrors::testNoError() +TestErrors::SetUpTestSuite() { - FRT_RPCRequest *req1 = client->AllocRPCRequest(); + server = std::make_unique<fnet::frt::StandaloneFRT>(); + target = vespalib::ref_counted<FRT_Target>::internal_attach(server->supervisor().GetTarget(spec.c_str())); +} + +void +TestErrors::TearDownTestSuite() +{ + target.reset(); + server.reset(); +} + +TEST_F(TestErrors, no_error) +{ + auto req1 = alloc_rpc_request(); req1->SetMethodName("test"); req1->GetParams()->AddInt32(42); req1->GetParams()->AddInt32(0); req1->GetParams()->AddInt8(0); - target->InvokeSync(req1, 60.0); + target->InvokeSync(req1.get(), 60.0); EXPECT_TRUE(!req1->IsError()); - if (EXPECT_TRUE(1 == req1->GetReturn()->GetNumValues())) { - EXPECT_TRUE(42 == req1->GetReturn()->GetValue(0)._intval32); - } else { - EXPECT_TRUE(false); - } - req1->internal_subref(); + ASSERT_EQ(1, req1->GetReturn()->GetNumValues()); + ASSERT_EQ(42, req1->GetReturn()->GetValue(0)._intval32); } -void -TestErrors::testNoSuchMethod() +TEST_F(TestErrors, no_such_method) { - FRT_RPCRequest *req1 = client->AllocRPCRequest(); + auto req1 = alloc_rpc_request(); req1->SetMethodName("bogus"); - target->InvokeSync(req1, 60.0); + target->InvokeSync(req1.get(), 60.0); EXPECT_TRUE(req1->IsError()); EXPECT_TRUE(0 == req1->GetReturn()->GetNumValues()); EXPECT_TRUE(FRTE_RPC_NO_SUCH_METHOD == req1->GetErrorCode()); - req1->internal_subref(); } -void -TestErrors::testWrongParameters() +TEST_F(TestErrors, wrong_parameters) { - FRT_RPCRequest *req1 = client->AllocRPCRequest(); + auto req1 = alloc_rpc_request(); req1->SetMethodName("test"); req1->GetParams()->AddInt32(42); req1->GetParams()->AddInt32(0); req1->GetParams()->AddInt32(0); - target->InvokeSync(req1, 60.0); + target->InvokeSync(req1.get(), 60.0); EXPECT_TRUE(req1->IsError()); - EXPECT_TRUE(0 == req1->GetReturn()->GetNumValues()); + EXPECT_EQ(0, req1->GetReturn()->GetNumValues()); EXPECT_TRUE(FRTE_RPC_WRONG_PARAMS == req1->GetErrorCode()); - req1->internal_subref(); + req1.reset(); - FRT_RPCRequest *req2 = client->AllocRPCRequest(); + auto req2 = alloc_rpc_request(); req2->SetMethodName("test"); req2->GetParams()->AddInt32(42); req2->GetParams()->AddInt32(0); - target->InvokeSync(req2, 60.0); + target->InvokeSync(req2.get(), 60.0); EXPECT_TRUE(req2->IsError()); - EXPECT_TRUE(0 == req2->GetReturn()->GetNumValues()); + EXPECT_EQ(0, req2->GetReturn()->GetNumValues()); EXPECT_TRUE(FRTE_RPC_WRONG_PARAMS == req2->GetErrorCode()); - req2->internal_subref(); + req2.reset(); - FRT_RPCRequest *req3 = client->AllocRPCRequest(); + auto req3 = alloc_rpc_request(); req3->SetMethodName("test"); req3->GetParams()->AddInt32(42); req3->GetParams()->AddInt32(0); req3->GetParams()->AddInt8(0); req3->GetParams()->AddInt8(0); - target->InvokeSync(req3, 60.0); + target->InvokeSync(req3.get(), 60.0); EXPECT_TRUE(req3->IsError()); - EXPECT_TRUE(0 == req3->GetReturn()->GetNumValues()); + EXPECT_EQ(0, req3->GetReturn()->GetNumValues()); EXPECT_TRUE(FRTE_RPC_WRONG_PARAMS == req3->GetErrorCode()); - req3->internal_subref(); } -void -TestErrors::testWrongReturnValues() +TEST_F(TestErrors, wrong_return_values) { - FRT_RPCRequest *req1 = client->AllocRPCRequest(); + auto req1 = alloc_rpc_request(); req1->SetMethodName("test"); req1->GetParams()->AddInt32(42); req1->GetParams()->AddInt32(0); req1->GetParams()->AddInt8(1); - target->InvokeSync(req1, 60.0); + target->InvokeSync(req1.get(), 60.0); EXPECT_TRUE(req1->IsError()); - EXPECT_TRUE(0 == req1->GetReturn()->GetNumValues()); - EXPECT_TRUE(FRTE_RPC_WRONG_RETURN == req1->GetErrorCode()); - req1->internal_subref(); + EXPECT_EQ(0, req1->GetReturn()->GetNumValues()); + EXPECT_TRUE(FRTE_RPC_WRONG_RETURN == req1->GetErrorCode()); } -void -TestErrors::testMethodFailed() +TEST_F(TestErrors, method_failed) { - FRT_RPCRequest *req1 = client->AllocRPCRequest(); + auto req1 = alloc_rpc_request(); req1->SetMethodName("test"); req1->GetParams()->AddInt32(42); req1->GetParams()->AddInt32(75000); req1->GetParams()->AddInt8(0); - target->InvokeSync(req1, 60.0); + target->InvokeSync(req1.get(), 60.0); EXPECT_TRUE(req1->IsError()); - EXPECT_TRUE(0 == req1->GetReturn()->GetNumValues()); - EXPECT_TRUE(75000 == req1->GetErrorCode()); - req1->internal_subref(); + EXPECT_EQ(0, req1->GetReturn()->GetNumValues()); + EXPECT_EQ(75000, req1->GetErrorCode()); - FRT_RPCRequest *req2 = client->AllocRPCRequest(); + auto req2 = alloc_rpc_request(); req2->SetMethodName("test"); req2->GetParams()->AddInt32(42); req2->GetParams()->AddInt32(75000); req2->GetParams()->AddInt8(1); - target->InvokeSync(req2, 60.0); + target->InvokeSync(req2.get(), 60.0); EXPECT_TRUE(req2->IsError()); - EXPECT_TRUE(0 == req2->GetReturn()->GetNumValues()); - EXPECT_TRUE(75000 == req2->GetErrorCode()); - req2->internal_subref(); + EXPECT_EQ(0, req2->GetReturn()->GetNumValues()); + EXPECT_EQ(75000, req2->GetErrorCode()); } - int -TestErrors::Main() +main(int argc, char* argv[]) { - if (_argc != 2) { - fprintf(stderr, "usage: %s spec", _argv[0]); + if (argc != 2) { + fprintf(stderr, "usage: %s spec\n", argv[0]); return 1; } - TEST_INIT("test_errors"); - init(_argv[1]); - testNoError(); - testNoSuchMethod(); - testWrongParameters(); - testWrongReturnValues(); - testMethodFailed(); - fini(); - TEST_DONE(); + spec = argv[1]; + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); } - - -TEST_APPHOOK(TestErrors); diff --git a/searchcore/src/tests/proton/matching/matching_test.cpp b/searchcore/src/tests/proton/matching/matching_test.cpp index 4d74a42a3bc..e3bb5d519c6 100644 --- a/searchcore/src/tests/proton/matching/matching_test.cpp +++ b/searchcore/src/tests/proton/matching/matching_test.cpp @@ -962,22 +962,20 @@ TEST(MatchingTest, require_that_match_params_are_set_up_straight_with_ranking_on ASSERT_EQ(10u, p.numDocs); ASSERT_EQ(2u, p.heapSize); ASSERT_EQ(4u, p.arraySize); - ASSERT_EQ(0.7, p.rankDropLimit); + ASSERT_EQ(0.7, p.first_phase_rank_score_drop_limit.value()); ASSERT_EQ(0u, p.offset); ASSERT_EQ(1u, p.hits); - ASSERT_TRUE(p.has_rank_drop_limit()); } TEST(MatchingTest, require_that_match_params_can_turn_off_rank_drop_limit) { - MatchParams p(10, 2, 4, -std::numeric_limits<feature_t>::quiet_NaN(), 0, 1, true, true); + MatchParams p(10, 2, 4, std::nullopt, 0, 1, true, true); ASSERT_EQ(10u, p.numDocs); ASSERT_EQ(2u, p.heapSize); ASSERT_EQ(4u, p.arraySize); - ASSERT_TRUE(std::isnan(p.rankDropLimit)); + ASSERT_FALSE(p.first_phase_rank_score_drop_limit.has_value()); ASSERT_EQ(0u, p.offset); ASSERT_EQ(1u, p.hits); - ASSERT_FALSE(p.has_rank_drop_limit()); } @@ -987,7 +985,7 @@ TEST(MatchingTest, require_that_match_params_are_set_up_straight_with_ranking_on ASSERT_EQ(10u, p.numDocs); ASSERT_EQ(6u, p.heapSize); ASSERT_EQ(6u, p.arraySize); - ASSERT_EQ(0.7, p.rankDropLimit); + ASSERT_EQ(0.7, p.first_phase_rank_score_drop_limit.value()); ASSERT_EQ(1u, p.offset); ASSERT_EQ(1u, p.hits); } @@ -998,7 +996,7 @@ TEST(MatchingTest, require_that_match_params_are_set_up_straight_with_ranking_on ASSERT_EQ(10u, p.numDocs); ASSERT_EQ(6u, p.heapSize); ASSERT_EQ(8u, p.arraySize); - ASSERT_EQ(0.7, p.rankDropLimit); + ASSERT_EQ(0.7, p.first_phase_rank_score_drop_limit.value()); ASSERT_EQ(4u, p.offset); ASSERT_EQ(4u, p.hits); } @@ -1009,7 +1007,7 @@ TEST(MatchingTest, require_that_match_params_are_capped_by_numDocs) ASSERT_EQ(1u, p.numDocs); ASSERT_EQ(1u, p.heapSize); ASSERT_EQ(1u, p.arraySize); - ASSERT_EQ(0.7, p.rankDropLimit); + ASSERT_EQ(0.7, p.first_phase_rank_score_drop_limit.value()); ASSERT_EQ(1u, p.offset); ASSERT_EQ(0u, p.hits); } @@ -1020,7 +1018,7 @@ TEST(MatchingTest, require_that_match_params_are_capped_by_numDocs_and_hits_adju ASSERT_EQ(5u, p.numDocs); ASSERT_EQ(5u, p.heapSize); ASSERT_EQ(5u, p.arraySize); - ASSERT_EQ(0.7, p.rankDropLimit); + ASSERT_EQ(0.7, p.first_phase_rank_score_drop_limit.value()); ASSERT_EQ(4u, p.offset); ASSERT_EQ(1u, p.hits); } @@ -1031,7 +1029,7 @@ TEST(MatchingTest, require_that_match_params_are_set_up_straight_with_ranking_of ASSERT_EQ(10u, p.numDocs); ASSERT_EQ(0u, p.heapSize); ASSERT_EQ(0u, p.arraySize); - ASSERT_EQ(0.7, p.rankDropLimit); + ASSERT_EQ(0.7, p.first_phase_rank_score_drop_limit.value()); ASSERT_EQ(4u, p.offset); ASSERT_EQ(4u, p.hits); } diff --git a/searchcore/src/vespa/searchcore/proton/matching/match_params.cpp b/searchcore/src/vespa/searchcore/proton/matching/match_params.cpp index 5cd97d314b5..2e3d7c77926 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/match_params.cpp +++ b/searchcore/src/vespa/searchcore/proton/matching/match_params.cpp @@ -19,7 +19,7 @@ computeArraySize(uint32_t hitsPlussOffset, uint32_t heapSize, uint32_t arraySize MatchParams::MatchParams(uint32_t numDocs_in, uint32_t heapSize_in, uint32_t arraySize_in, - search::feature_t rankDropLimit_in, + std::optional<search::feature_t> first_phase_rank_score_drop_limit_in, uint32_t offset_in, uint32_t hits_in, bool hasFinalRank, @@ -31,12 +31,7 @@ MatchParams::MatchParams(uint32_t numDocs_in, : 0), offset(std::min(numDocs_in, offset_in)), hits(std::min(numDocs_in - offset, hits_in)), - rankDropLimit(rankDropLimit_in) + first_phase_rank_score_drop_limit(first_phase_rank_score_drop_limit_in) { } -bool -MatchParams::has_rank_drop_limit() const { - return ! std::isnan(rankDropLimit); -} - } diff --git a/searchcore/src/vespa/searchcore/proton/matching/match_params.h b/searchcore/src/vespa/searchcore/proton/matching/match_params.h index 96ff892525f..a487b9f13f0 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/match_params.h +++ b/searchcore/src/vespa/searchcore/proton/matching/match_params.h @@ -4,6 +4,7 @@ #include <vespa/searchlib/common/feature.h> #include <cstdint> +#include <optional> namespace proton::matching { @@ -17,18 +18,17 @@ struct MatchParams { const uint32_t arraySize; const uint32_t offset; const uint32_t hits; - const search::feature_t rankDropLimit; + const std::optional<search::feature_t> first_phase_rank_score_drop_limit; MatchParams(uint32_t numDocs_in, uint32_t heapSize_in, uint32_t arraySize_in, - search::feature_t rankDropLimit_in, + std::optional<search::feature_t> first_phase_rank_drop_limit_in, uint32_t offset_in, uint32_t hits_in, bool hasFinalRank, bool needRanking); bool save_rank_scores() const noexcept { return (arraySize != 0); } - bool has_rank_drop_limit() const; }; } diff --git a/searchcore/src/vespa/searchcore/proton/matching/match_thread.cpp b/searchcore/src/vespa/searchcore/proton/matching/match_thread.cpp index 6b443231c0a..81a4b443208 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/match_thread.cpp +++ b/searchcore/src/vespa/searchcore/proton/matching/match_thread.cpp @@ -15,6 +15,7 @@ #include <vespa/searchlib/queryeval/profiled_iterator.h> #include <vespa/vespalib/data/slime/cursor.h> #include <vespa/vespalib/data/slime/inserter.h> +#include <limits> #include <vespa/log/log.h> LOG_SETUP(".proton.matching.match_thread"); @@ -99,11 +100,11 @@ fillPartialResult(ResultProcessor::Context & context, size_t totalHits, size_t n //----------------------------------------------------------------------------- -MatchThread::Context::Context(double rankDropLimit, MatchTools &tools, HitCollector &hits, uint32_t num_threads) +MatchThread::Context::Context(std::optional<double> first_phase_rank_score_drop_limit, MatchTools &tools, HitCollector &hits, uint32_t num_threads) : matches(0), _matches_limit(tools.match_limiter().sample_hits_per_thread(num_threads)), _score_feature(get_score_feature(tools.rank_program())), - _rankDropLimit(rankDropLimit), + _first_phase_rank_score_drop_limit(first_phase_rank_score_drop_limit.value_or(0.0 /* ignored */)), _hits(hits), _doom(tools.getDoom()), dropped() @@ -119,7 +120,7 @@ MatchThread::Context::rankHit(uint32_t docId) { score = -HUGE_VAL; } if (use_rank_drop_limit != RankDropLimitE::no) { - if (__builtin_expect(score > _rankDropLimit, true)) { + if (__builtin_expect(score > _first_phase_rank_score_drop_limit, true)) { _hits.addHit(docId, score); } else if (use_rank_drop_limit == RankDropLimitE::track) { dropped.template emplace_back(docId); @@ -217,7 +218,7 @@ MatchThread::match_loop(MatchTools &tools, HitCollector &hits) bool softDoomed = false; uint32_t docsCovered = 0; vespalib::duration overtime(vespalib::duration::zero()); - Context context(matchParams.rankDropLimit, tools, hits, num_threads); + Context context(matchParams.first_phase_rank_score_drop_limit, tools, hits, num_threads); for (DocidRange docid_range = scheduler.first_range(thread_id); !docid_range.empty(); docid_range = scheduler.next_range(thread_id)) @@ -270,7 +271,7 @@ template <bool do_rank, bool do_limit, bool do_share> void MatchThread::match_loop_helper_rank_limit_share(MatchTools &tools, HitCollector &hits) { - if (matchParams.has_rank_drop_limit()) { + if (matchParams.first_phase_rank_score_drop_limit.has_value()) { if (matchToolsFactory.hasOnMatchTask()) { match_loop_helper_rank_limit_share_drop<do_rank, do_limit, do_share, RankDropLimitE::track>(tools, hits); } else { diff --git a/searchcore/src/vespa/searchcore/proton/matching/match_thread.h b/searchcore/src/vespa/searchcore/proton/matching/match_thread.h index c6b233f2fcd..b267e7b799c 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/match_thread.h +++ b/searchcore/src/vespa/searchcore/proton/matching/match_thread.h @@ -73,7 +73,7 @@ private: class Context { public: - Context(double rankDropLimit, MatchTools &tools, HitCollector &hits, + Context(std::optional<double> first_phase_rank_score_drop_limit, MatchTools &tools, HitCollector &hits, uint32_t num_threads) __attribute__((noinline)); template <RankDropLimitE use_rank_drop_limit> void rankHit(uint32_t docId); @@ -86,7 +86,7 @@ private: private: uint32_t _matches_limit; LazyValue _score_feature; - double _rankDropLimit; + double _first_phase_rank_score_drop_limit; HitCollector &_hits; const Doom _doom; public: diff --git a/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp b/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp index 4a9156770f0..52ec4bbedd7 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp +++ b/searchcore/src/vespa/searchcore/proton/matching/matcher.cpp @@ -38,7 +38,7 @@ using search::fef::MatchData; using search::fef::RankSetup; using search::fef::indexproperties::hitcollector::HeapSize; using search::fef::indexproperties::hitcollector::ArraySize; -using search::fef::indexproperties::hitcollector::RankScoreDropLimit; +using search::fef::indexproperties::hitcollector::FirstPhaseRankScoreDropLimit; using search::queryeval::Blueprint; using search::queryeval::SearchIterator; using vespalib::Doom; @@ -94,12 +94,12 @@ private: bool willNeedRanking(const SearchRequest & request, const GroupingContext & groupingContext, - search::feature_t rank_score_drop_limit) + std::optional<search::feature_t> first_phase_rank_score_drop_limit) { return (groupingContext.needRanking() || (request.maxhits != 0)) && (request.sortSpec.empty() || (request.sortSpec.find("[rank]") != vespalib::string::npos) || - !std::isnan(rank_score_drop_limit)); + first_phase_rank_score_drop_limit.has_value()); } SearchReply::UP @@ -289,11 +289,11 @@ Matcher::match(const SearchRequest &request, vespalib::ThreadBundle &threadBundl const Properties & rankProperties = request.propertiesMap.rankProperties(); uint32_t heapSize = HeapSize::lookup(rankProperties, _rankSetup->getHeapSize()); uint32_t arraySize = ArraySize::lookup(rankProperties, _rankSetup->getArraySize()); - search::feature_t rank_score_drop_limit = RankScoreDropLimit::lookup(rankProperties, _rankSetup->getRankScoreDropLimit()); + auto first_phase_rank_score_drop_limit = FirstPhaseRankScoreDropLimit::lookup(rankProperties, _rankSetup->get_first_phase_rank_score_drop_limit()); - MatchParams params(searchContext.getDocIdLimit(), heapSize, arraySize, rank_score_drop_limit, + MatchParams params(searchContext.getDocIdLimit(), heapSize, arraySize, first_phase_rank_score_drop_limit, request.offset, request.maxhits, !_rankSetup->getSecondPhaseRank().empty(), - willNeedRanking(request, groupingContext, rank_score_drop_limit)); + willNeedRanking(request, groupingContext, first_phase_rank_score_drop_limit)); ResultProcessor rp(attrContext, metaStore, sessionMgr, groupingContext, sessionId, request.sortSpec, params.offset, params.hits); diff --git a/searchlib/src/tests/fef/properties/properties_test.cpp b/searchlib/src/tests/fef/properties/properties_test.cpp index d3890c8f9fc..5e9c3a25ef8 100644 --- a/searchlib/src/tests/fef/properties/properties_test.cpp +++ b/searchlib/src/tests/fef/properties/properties_test.cpp @@ -409,16 +409,18 @@ TEST(PropertiesTest, test_stuff) EXPECT_EQ(hitcollector::EstimateLimit::lookup(p), 50u); } { // vespa.hitcollector.rankscoredroplimit - EXPECT_EQ(hitcollector::RankScoreDropLimit::NAME, vespalib::string("vespa.hitcollector.rankscoredroplimit")); - search::feature_t got1 = hitcollector::RankScoreDropLimit::DEFAULT_VALUE; - EXPECT_TRUE(got1 != got1); - Properties p; - search::feature_t got2= hitcollector::RankScoreDropLimit::lookup(p); - EXPECT_TRUE(got2 != got2); + EXPECT_EQ(vespalib::string("vespa.hitcollector.rankscoredroplimit"), hitcollector::FirstPhaseRankScoreDropLimit::NAME); + Properties p; + auto got2 = hitcollector::FirstPhaseRankScoreDropLimit::lookup(p); + EXPECT_EQ(std::optional<search::feature_t>(), got2); + got2 = hitcollector::FirstPhaseRankScoreDropLimit::lookup(p, std::nullopt); + EXPECT_EQ(std::optional<search::feature_t>(), got2); + got2 = hitcollector::FirstPhaseRankScoreDropLimit::lookup(p, 4.5); + EXPECT_EQ(std::optional<search::feature_t>(4.5), got2); p.add("vespa.hitcollector.rankscoredroplimit", "-123456789.12345"); - EXPECT_EQ(hitcollector::RankScoreDropLimit::lookup(p), -123456789.12345); + EXPECT_EQ(std::optional<search::feature_t>(-123456789.12345), hitcollector::FirstPhaseRankScoreDropLimit::lookup(p)); p.clear().add("vespa.hitcollector.rankscoredroplimit", "123456789.12345"); - EXPECT_EQ(hitcollector::RankScoreDropLimit::lookup(p), 123456789.12345); + EXPECT_EQ(std::optional<search::feature_t>(123456789.12345), hitcollector::FirstPhaseRankScoreDropLimit::lookup(p)); } { // vespa.fieldweight. EXPECT_EQ(FieldWeight::BASE_NAME, vespalib::string("vespa.fieldweight.")); diff --git a/searchlib/src/tests/ranksetup/ranksetup_test.cpp b/searchlib/src/tests/ranksetup/ranksetup_test.cpp index 348326c3936..595b2065feb 100644 --- a/searchlib/src/tests/ranksetup/ranksetup_test.cpp +++ b/searchlib/src/tests/ranksetup/ranksetup_test.cpp @@ -525,7 +525,7 @@ void RankSetupTest::testRankSetup() env.getProperties().add(hitcollector::ArraySize::NAME, "60"); env.getProperties().add(hitcollector::EstimatePoint::NAME, "70"); env.getProperties().add(hitcollector::EstimateLimit::NAME, "80"); - env.getProperties().add(hitcollector::RankScoreDropLimit::NAME, "90.5"); + env.getProperties().add(hitcollector::FirstPhaseRankScoreDropLimit::NAME, "90.5"); env.getProperties().add(mutate::on_match::Attribute::NAME, "a"); env.getProperties().add(mutate::on_match::Operation::NAME, "+=3"); env.getProperties().add(mutate::on_first_phase::Attribute::NAME, "b"); @@ -567,7 +567,7 @@ void RankSetupTest::testRankSetup() EXPECT_EQUAL(rs.getArraySize(), 60u); EXPECT_EQUAL(rs.getEstimatePoint(), 70u); EXPECT_EQUAL(rs.getEstimateLimit(), 80u); - EXPECT_EQUAL(rs.getRankScoreDropLimit(), 90.5); + EXPECT_EQUAL(rs.get_first_phase_rank_score_drop_limit().value(), 90.5); EXPECT_EQUAL(rs.getMutateOnMatch()._attribute, "a"); EXPECT_EQUAL(rs.getMutateOnMatch()._operation, "+=3"); EXPECT_EQUAL(rs.getMutateOnFirstPhase()._attribute, "b"); diff --git a/searchlib/src/vespa/searchlib/fef/indexproperties.cpp b/searchlib/src/vespa/searchlib/fef/indexproperties.cpp index 1f88c34bef3..516a09e31b4 100644 --- a/searchlib/src/vespa/searchlib/fef/indexproperties.cpp +++ b/searchlib/src/vespa/searchlib/fef/indexproperties.cpp @@ -36,14 +36,20 @@ lookupStringVector(const Properties &props, const vespalib::string &name, return defaultValue; } -double -lookupDouble(const Properties &props, const vespalib::string &name, double defaultValue) +std::optional<double> +lookup_opt_double(const Properties &props, vespalib::stringref name, std::optional<double> default_value) { Property p = props.lookup(name); if (p.found()) { return vespalib::locale::c::strtod(p.get().c_str(), nullptr); } - return defaultValue; + return default_value; +} + +double +lookupDouble(const Properties &props, const vespalib::string &name, double defaultValue) +{ + return lookup_opt_double(props, name, defaultValue).value(); } uint32_t @@ -683,19 +689,19 @@ EstimateLimit::lookup(const Properties &props) return lookupUint32(props, NAME, DEFAULT_VALUE); } -const vespalib::string RankScoreDropLimit::NAME("vespa.hitcollector.rankscoredroplimit"); -const feature_t RankScoreDropLimit::DEFAULT_VALUE(-std::numeric_limits<feature_t>::quiet_NaN()); +const vespalib::string FirstPhaseRankScoreDropLimit::NAME("vespa.hitcollector.rankscoredroplimit"); +const std::optional<feature_t> FirstPhaseRankScoreDropLimit::DEFAULT_VALUE(std::nullopt); -feature_t -RankScoreDropLimit::lookup(const Properties &props) +std::optional<feature_t> +FirstPhaseRankScoreDropLimit::lookup(const Properties &props) { return lookup(props, DEFAULT_VALUE); } -feature_t -RankScoreDropLimit::lookup(const Properties &props, feature_t defaultValue) +std::optional<feature_t> +FirstPhaseRankScoreDropLimit::lookup(const Properties &props, std::optional<feature_t> default_value) { - return lookupDouble(props, NAME, defaultValue); + return lookup_opt_double(props, NAME, default_value); } } // namspace hitcollector diff --git a/searchlib/src/vespa/searchlib/fef/indexproperties.h b/searchlib/src/vespa/searchlib/fef/indexproperties.h index d047eb13347..bf965e6805a 100644 --- a/searchlib/src/vespa/searchlib/fef/indexproperties.h +++ b/searchlib/src/vespa/searchlib/fef/indexproperties.h @@ -5,6 +5,7 @@ #include <vespa/searchlib/common/feature.h> #include <vespa/vespalib/fuzzy/fuzzy_matching_algorithm.h> #include <vespa/vespalib/stllike/string.h> +#include <optional> #include <vector> namespace search::fef { class Properties; } @@ -562,17 +563,17 @@ namespace hitcollector { }; /** - * Property for the rank score drop limit used in parallel query evaluation. - * Drop a hit if the rank score <= drop limit. + * Property for the first phase rank score drop limit used in parallel + * query evaluation. + * Drop a hit if the first phase rank score <= drop limit. **/ - struct RankScoreDropLimit { + struct FirstPhaseRankScoreDropLimit { static const vespalib::string NAME; - static const feature_t DEFAULT_VALUE; - static feature_t lookup(const Properties &props); - static feature_t lookup(const Properties &props, feature_t defaultValue); + static const std::optional<feature_t> DEFAULT_VALUE; + static std::optional<feature_t> lookup(const Properties &props); + static std::optional<feature_t> lookup(const Properties &props, std::optional<feature_t> default_value); }; - } // namespace hitcollector /** diff --git a/searchlib/src/vespa/searchlib/fef/ranksetup.cpp b/searchlib/src/vespa/searchlib/fef/ranksetup.cpp index ba5abb35141..85c183f140c 100644 --- a/searchlib/src/vespa/searchlib/fef/ranksetup.cpp +++ b/searchlib/src/vespa/searchlib/fef/ranksetup.cpp @@ -50,7 +50,7 @@ RankSetup::RankSetup(const BlueprintFactory &factory, const IIndexEnvironment &i _degradationMaxFilterCoverage(1.0), _degradationSamplePercentage(0.2), _degradationPostFilterMultiplier(1.0), - _rankScoreDropLimit(0), + _first_phase_rank_score_drop_limit(), _match_features(), _summaryFeatures(), _dumpFeatures(), @@ -120,7 +120,7 @@ RankSetup::configure() setDiversityCutoffStrategy(matchphase::DiversityCutoffStrategy::lookup(_indexEnv.getProperties())); setEstimatePoint(hitcollector::EstimatePoint::lookup(_indexEnv.getProperties())); setEstimateLimit(hitcollector::EstimateLimit::lookup(_indexEnv.getProperties())); - setRankScoreDropLimit(hitcollector::RankScoreDropLimit::lookup(_indexEnv.getProperties())); + set_first_phase_rank_score_drop_limit(hitcollector::FirstPhaseRankScoreDropLimit::lookup(_indexEnv.getProperties())); setSoftTimeoutEnabled(softtimeout::Enabled::lookup(_indexEnv.getProperties())); setSoftTimeoutTailCost(softtimeout::TailCost::lookup(_indexEnv.getProperties())); set_global_filter_lower_limit(matching::GlobalFilterLowerLimit::lookup(_indexEnv.getProperties())); diff --git a/searchlib/src/vespa/searchlib/fef/ranksetup.h b/searchlib/src/vespa/searchlib/fef/ranksetup.h index f20ecd4b42b..4fe35250253 100644 --- a/searchlib/src/vespa/searchlib/fef/ranksetup.h +++ b/searchlib/src/vespa/searchlib/fef/ranksetup.h @@ -9,6 +9,7 @@ #include "rank_program.h" #include <vespa/searchlib/common/stringmap.h> #include <vespa/vespalib/fuzzy/fuzzy_matching_algorithm.h> +#include <optional> namespace search::fef { @@ -59,7 +60,7 @@ private: double _degradationMaxFilterCoverage; double _degradationSamplePercentage; double _degradationPostFilterMultiplier; - feature_t _rankScoreDropLimit; + std::optional<feature_t> _first_phase_rank_score_drop_limit; std::vector<vespalib::string> _match_features; std::vector<vespalib::string> _summaryFeatures; std::vector<vespalib::string> _dumpFeatures; @@ -332,18 +333,18 @@ public: uint32_t getEstimateLimit() const { return _estimateLimit; } /** - * Sets the rank score drop limit to be used in parallel query evaluation. + * Sets the first phase rank score drop limit to be used in parallel query evaluation. * - * @param rankScoreDropLimit the rank score drop limit + * @param value the first phase rank score drop limit **/ - void setRankScoreDropLimit(feature_t rankScoreDropLimit) { _rankScoreDropLimit = rankScoreDropLimit; } + void set_first_phase_rank_score_drop_limit(std::optional<feature_t> value) { _first_phase_rank_score_drop_limit = value; } /** * Returns the rank score drop limit to be used in parallel query evaluation. * * @return the rank score drop limit **/ - feature_t getRankScoreDropLimit() const { return _rankScoreDropLimit; } + std::optional<feature_t> get_first_phase_rank_score_drop_limit() const noexcept { return _first_phase_rank_score_drop_limit; } /** * This method may be used to indicate that certain features diff --git a/streamingvisitors/src/vespa/searchvisitor/searchvisitor.cpp b/streamingvisitors/src/vespa/searchvisitor/searchvisitor.cpp index 7ecda0e82f1..926da9438a1 100644 --- a/streamingvisitors/src/vespa/searchvisitor/searchvisitor.cpp +++ b/streamingvisitors/src/vespa/searchvisitor/searchvisitor.cpp @@ -684,7 +684,7 @@ SearchVisitor::RankController::processAccessedAttributes(const QueryEnvironment SearchVisitor::RankController::RankController() : _rankProfile("default"), _rankManagerSnapshot(nullptr), - _rank_score_drop_limit(std::numeric_limits<search::feature_t>::min()), + _rank_score_drop_limit(), _hasRanking(false), _hasSummaryFeatures(false), _dumpFeatures(false), @@ -703,9 +703,9 @@ SearchVisitor::RankController::setupRankProcessors(Query & query, const search::IAttributeManager & attrMan, std::vector<AttrInfo> & attributeFields) { - using RankScoreDropLimit = search::fef::indexproperties::hitcollector::RankScoreDropLimit; + using FirstPhaseRankScoreDropLimit = search::fef::indexproperties::hitcollector::FirstPhaseRankScoreDropLimit; const search::fef::RankSetup & rankSetup = _rankManagerSnapshot->getRankSetup(_rankProfile); - _rank_score_drop_limit = RankScoreDropLimit::lookup(_queryProperties, rankSetup.getRankScoreDropLimit()); + _rank_score_drop_limit = FirstPhaseRankScoreDropLimit::lookup(_queryProperties, rankSetup.get_first_phase_rank_score_drop_limit()); _rankProcessor = std::make_unique<RankProcessor>(_rankManagerSnapshot, _rankProfile, query, location, _queryProperties, _featureOverrides, &attrMan); _rankProcessor->initForRanking(wantedHitCount, use_sort_blob); // register attribute vectors needed for ranking @@ -751,8 +751,11 @@ SearchVisitor::RankController::rankMatchedDocument(uint32_t docId) bool SearchVisitor::RankController::keepMatchedDocument() { + if (!_rank_score_drop_limit.has_value()) { + return true; + } // also make sure that NaN scores are added - return (!(_rankProcessor->getRankScore() <= _rank_score_drop_limit)); + return (!(_rankProcessor->getRankScore() <= _rank_score_drop_limit.value())); } void @@ -1139,7 +1142,7 @@ SearchVisitor::handleDocument(StorageDocument::SP documentSP) } else { _hitsRejectedCount++; LOG(debug, "Do not keep document with id '%s' because rank score (%f) <= rank score drop limit (%f)", - documentId.c_str(), rp.getRankScore(), _rankController.rank_score_drop_limit()); + documentId.c_str(), rp.getRankScore(), _rankController.rank_score_drop_limit().value()); } } else { LOG(debug, "Did not match document with id '%s'", document.docDoc().getId().getScheme().toString().c_str()); diff --git a/streamingvisitors/src/vespa/searchvisitor/searchvisitor.h b/streamingvisitors/src/vespa/searchvisitor/searchvisitor.h index 7d73a159f6f..2f6673abd2d 100644 --- a/streamingvisitors/src/vespa/searchvisitor/searchvisitor.h +++ b/streamingvisitors/src/vespa/searchvisitor/searchvisitor.h @@ -30,6 +30,7 @@ #include <vespa/document/fieldvalue/fieldvalues.h> #include <vespa/documentapi/messagebus/messages/queryresultmessage.h> #include <vespa/document/fieldvalue/iteratorhandler.h> +#include <optional> using namespace search::aggregation; @@ -127,7 +128,7 @@ private: private: vespalib::string _rankProfile; std::shared_ptr<const RankManager::Snapshot> _rankManagerSnapshot; - search::feature_t _rank_score_drop_limit; + std::optional<search::feature_t> _rank_score_drop_limit; bool _hasRanking; bool _hasSummaryFeatures; bool _dumpFeatures; @@ -157,7 +158,7 @@ private: RankProcessor * getRankProcessor() { return _rankProcessor.get(); } void setDumpFeatures(bool dumpFeatures) { _dumpFeatures = dumpFeatures; } bool getDumpFeatures() const { return _dumpFeatures; } - search::feature_t rank_score_drop_limit() const noexcept { return _rank_score_drop_limit; } + std::optional<search::feature_t> rank_score_drop_limit() const noexcept { return _rank_score_drop_limit; } /** * Setup rank processors used for ranking and dumping. diff --git a/vespalib/src/vespa/vespalib/util/ref_counted.h b/vespalib/src/vespa/vespalib/util/ref_counted.h index 1992bdaa5a8..3daa8bbff99 100644 --- a/vespalib/src/vespa/vespalib/util/ref_counted.h +++ b/vespalib/src/vespa/vespalib/util/ref_counted.h @@ -96,6 +96,7 @@ public: } T *operator->() const noexcept { return _ptr; } T &operator*() const noexcept { return *_ptr; } + T* get() const noexcept { return _ptr; } operator bool() const noexcept { return (_ptr != nullptr); } void reset() noexcept { replace_with(nullptr); } ~ref_counted() noexcept { maybe_subref(); } |