From ed7c39ea2171772f6913334d7985d32cd3ce6e5a Mon Sep 17 00:00:00 2001 From: Yuri Gorshenin Date: Tue, 1 Dec 2015 16:52:27 +0300 Subject: [PATCH] Review fixes. --- search/retrieval.cpp | 4 +- search/retrieval.hpp | 6 +-- .../search_query_v2_test.cpp | 11 +++-- search/v2/features_layer_matcher.hpp | 2 + search/v2/geocoder.cpp | 28 ++++++++---- search/v2/geocoder.hpp | 44 ++++++++++++++++--- search/v2/search_model.hpp | 2 +- 7 files changed, 71 insertions(+), 26 deletions(-) diff --git a/search/retrieval.cpp b/search/retrieval.cpp index 2e7b51bf01..9f292ad1fd 100644 --- a/search/retrieval.cpp +++ b/search/retrieval.cpp @@ -74,7 +74,7 @@ unique_ptr RetrieveAddressFeaturesImpl( auto collector = [&](TValue const & value) { if (cancellable.IsCancelled()) - throw CancelException(); + MYTHROW(CancelException, ("Search cancelled")); features.push_back(value.m_featureId); }; @@ -97,7 +97,7 @@ unique_ptr RetrieveGeometryFeatures( auto collector = [&](uint64_t featureId) { if (cancellable.IsCancelled()) - throw CancelException(); + MYTHROW(CancelException, ("Search cancelled")); features.push_back(featureId); }; diff --git a/search/retrieval.hpp b/search/retrieval.hpp index 0333ed9e26..36ea1810ee 100644 --- a/search/retrieval.hpp +++ b/search/retrieval.hpp @@ -7,9 +7,9 @@ #include "geometry/rect2d.hpp" #include "base/cancellable.hpp" +#include "base/exception.hpp" #include "base/macros.hpp" -#include "std/exception.hpp" #include "std/function.hpp" #include "std/unique_ptr.hpp" #include "std/vector.hpp" @@ -29,9 +29,7 @@ namespace search // // TODO (@gorshenin): after merge to master, move this class to // base/cancellable.hpp. -struct CancelException : public exception -{ -}; +DECLARE_EXCEPTION(CancelException, RootException); class Retrieval : public my::Cancellable { diff --git a/search/search_integration_tests/search_query_v2_test.cpp b/search/search_integration_tests/search_query_v2_test.cpp index 8f43018932..b1f02d09ce 100644 --- a/search/search_integration_tests/search_query_v2_test.cpp +++ b/search/search_integration_tests/search_query_v2_test.cpp @@ -52,10 +52,7 @@ UNIT_TEST(SearchQueryV2_Smoke) m2::RectD viewport(m2::PointD(-1.0, -1.0), m2::PointD(1.0, 1.0)); Cleanup(map); - MY_SCOPE_GUARD(cleanup, [&]() - { - Cleanup(map); - }); + MY_SCOPE_GUARD(cleanup, [&]() { Cleanup(map); }); vector countries; countries.emplace_back(map.GetCountryName(), viewport); @@ -109,4 +106,10 @@ UNIT_TEST(SearchQueryV2_Smoke) make_shared(regResult.first, quantumTeleport1)}; TEST(MatchResults(engine, rules, request.Results()), ()); } + + { + TestSearchRequest request(engine, " ", "en", search::SearchParams::ALL, viewport); + request.Wait(); + TEST_EQUAL(0, request.Results().size(), ()); + } } diff --git a/search/v2/features_layer_matcher.hpp b/search/v2/features_layer_matcher.hpp index 6fded3a286..733768121c 100644 --- a/search/v2/features_layer_matcher.hpp +++ b/search/v2/features_layer_matcher.hpp @@ -37,6 +37,8 @@ public: return; } + // TODO (y@): match POI with streets separately. + vector childCenters; for (uint32_t featureId : child.m_features) { diff --git a/search/v2/geocoder.cpp b/search/v2/geocoder.cpp index 507f4bd0d9..611251a4f4 100644 --- a/search/v2/geocoder.cpp +++ b/search/v2/geocoder.cpp @@ -48,6 +48,9 @@ void Geocoder::SetSearchQueryParams(SearchQueryParams const & params) void Geocoder::Go(vector & results) { + if (m_numTokens == 0) + return; + m_results = &results; try @@ -69,7 +72,7 @@ void Geocoder::Go(vector & results) m_mwmId = handle.GetId(); MY_SCOPE_GUARD(cleanup, [&]() - { + { m_finder.reset(); m_loader.reset(); m_cache.clear(); @@ -87,15 +90,18 @@ void Geocoder::Go(vector & results) } } -void Geocoder::PrepareParams(size_t from, size_t to) +void Geocoder::PrepareParams(size_t curToken, size_t endToken) { - ASSERT_LESS(from, to, ()); - ASSERT_LESS_OR_EQUAL(to, m_numTokens, ()); + ASSERT_LESS(curToken, endToken, ()); + ASSERT_LESS_OR_EQUAL(endToken, m_numTokens, ()); m_retrievalParams.m_tokens.clear(); m_retrievalParams.m_prefixTokens.clear(); - for (size_t i = from; i < to; ++i) + // TODO (@y): possibly it's not cheap to copy vectors of strings. + // Profile it, and in case of serious performance loss, refactor + // SearchQueryParams to support subsets of tokens. + for (size_t i = curToken; i < endToken; ++i) { if (i < m_params.m_tokens.size()) m_retrievalParams.m_tokens.push_back(m_params.m_tokens[i]); @@ -108,8 +114,9 @@ void Geocoder::DoGeocoding(size_t curToken) { if (curToken == m_numTokens) { - // All tokens were consumed, intersect layers, emit features. - IntersectLayers(); + // All tokens were consumed, find paths through layers, emit + // features. + FindPaths(); return; } @@ -127,6 +134,9 @@ void Geocoder::DoGeocoding(size_t curToken) layer.m_endToken = curToken + n; } + // TODO (@y, @m): as |n| increases, good optimization is to update + // |features| incrementally, from [curToken, curToken + n) to + // [curToken, curToken + n + 1). auto features = RetrieveAddressFeatures(curToken, curToken + n); if (!features || features->PopCount() == 0) continue; @@ -208,7 +218,7 @@ bool Geocoder::LooksLikeHouseNumber(size_t curToken, size_t endToken) const return false; } -void Geocoder::IntersectLayers() +void Geocoder::FindPaths() { ASSERT(!m_layers.empty(), ()); @@ -225,7 +235,7 @@ void Geocoder::IntersectLayers() sort(sortedLayers.begin(), sortedLayers.end(), compareByType); m_finder->ForEachReachableVertex(sortedLayers, [this](uint32_t featureId) - { + { m_results->emplace_back(m_mwmId, featureId); }); } diff --git a/search/v2/geocoder.hpp b/search/v2/geocoder.hpp index 3d2f0202c8..ac1bc6bcaa 100644 --- a/search/v2/geocoder.hpp +++ b/search/v2/geocoder.hpp @@ -37,6 +37,21 @@ namespace v2 class FeaturesLayerPathFinder; class SearchModel; +// This class is used to retrieve all features corresponding to a +// search query. Search query is represented as a sequence of tokens +// (including synonyms for these tokens), and Geocoder tries to build +// all possible partitions (or layers) of the search query, where each +// layer is a set of features corresponding to some search class +// (e.g. POI, BUILDING, STREET, etc., see search/v2/search_model.hpp). +// Then, Geocoder builds a layered graph, with edges between features +// on adjacent layers (e.g. between BUILDING ans STREET, STREET and +// CITY, etc.). Usually an edge between two features means that a +// feature from the lowest layer geometrically belongs to a feature +// from the highest layer (BUILDING is located on STREET, STREET is +// located inside CITY, CITY is located inside STATE, etc.). Final +// part is to find all paths through this layered graph and report all +// features from the lowest layer, that are reachable from the highest +// layer. class Geocoder : public my::Cancellable { public: @@ -44,30 +59,46 @@ public: ~Geocoder() override; + // Sets search query params. void SetSearchQueryParams(SearchQueryParams const & params); + // Starts geocoding, retrieved features will be appended to + // |results|. void Go(vector & results); private: - void PrepareParams(size_t from, size_t to); + // Fills |m_retrievalParams| with [curToken, endToken) subsequence + // of search query tokens. + void PrepareParams(size_t curToken, size_t endToken); + // Tries to find all paths in a search tree, where each edge is + // marked with some substring of the query tokens. These paths are + // called "layer sequence" and current path is stored in |m_layers|. void DoGeocoding(size_t curToken); + // Returns CBV of features corresponding to [curToken, endToken) + // subsequence of search query tokens. This method caches results of + // previous requests. coding::CompressedBitVector * RetrieveAddressFeatures(size_t curToken, size_t endToken); + // Returns true if current path in the search tree (see comment for + // DoGeocoding()) looks sane. This method is used as a fast + // pre-check to cut off unnecessary work. bool IsLayerSequenceSane() const; + // Returns true if [curToken, endToken) subsequence looks like a house number. bool LooksLikeHouseNumber(size_t curToken, size_t endToken) const; - void IntersectLayers(); + // Finds all paths through layers and emits reachable features from + // the lowest layer. + void FindPaths(); Index & m_index; // Initial search query params. SearchQueryParams m_params; - // Total number of tokens (including last prefix token, if - // non-empty). + // Total number of search query tokens. size_t m_numTokens; // This field is used to map features to a limited number of search @@ -75,7 +106,7 @@ private: SearchModel const & m_model; // Following fields are set up by Search() method and can be - // modified and used only from Search() or it's callees. + // modified and used only from Search() or its callees. // Value of a current mwm. MwmValue * m_value; @@ -83,7 +114,7 @@ private: // Id of a current mwm. MwmSet::MwmId m_mwmId; - // Cache of bit-vectors. + // Cache of posting list of features. unordered_map> m_cache; // Features loader. @@ -98,6 +129,7 @@ private: // Stack of layers filled during geocoding. vector m_layers; + // Non-owning pointer to a vector of results. vector * m_results; }; } // namespace v2 diff --git a/search/v2/search_model.hpp b/search/v2/search_model.hpp index 03cdd4ceee..7028df1c5e 100644 --- a/search/v2/search_model.hpp +++ b/search/v2/search_model.hpp @@ -14,7 +14,7 @@ namespace search { namespace v2 { -// This class is used to map our types to a restricted set of +// This class is used to map feature types to a restricted set of // different search classes (do not confuse these classes with search // categories - they are completely different things). class SearchModel