diff options
11 files changed, 142 insertions, 139 deletions
diff --git a/searchcore/src/vespa/searchcore/proton/docsummary/docsumcontext.cpp b/searchcore/src/vespa/searchcore/proton/docsummary/docsumcontext.cpp index 0869fc175a7..38e7e37a82d 100644 --- a/searchcore/src/vespa/searchcore/proton/docsummary/docsumcontext.cpp +++ b/searchcore/src/vespa/searchcore/proton/docsummary/docsumcontext.cpp @@ -181,36 +181,29 @@ DocsumContext::FillRankFeatures(search::docsummary::GetDocsumsState * state, sea state->_rankFeatures = _matcher->getRankFeatures(_request, _searchCtx, _attrCtx, _sessionMgr); } -namespace { -Location *getLocation(const string &loc_str, search::IAttributeManager &attrMgr) +void +DocsumContext::ParseLocation(search::docsummary::GetDocsumsState *state) { - LOG(debug, "Filling document locations from location string: %s", loc_str.c_str()); - - Location *loc = new Location; - string location; - string::size_type pos = loc_str.find(':'); - if (pos != string::npos) { - string view = loc_str.substr(0, pos); - AttributeGuard::UP vec = attrMgr.getAttribute(view); + search::common::GeoLocationParser locationParser; + if (locationParser.parseOldFormatWithField(_request.location)) { + auto spec = locationParser.spec(); + LOG(debug, "Filling document locations from location string: %s", + _request.location.c_str()); + string view = spec.getFieldName(); + AttributeGuard::UP vec = _attrMgr.getAttribute(view); if (!vec->valid()) { view = PositionDataType::getZCurveFieldName(view); - vec = attrMgr.getAttribute(view); + vec = _attrMgr.getAttribute(view); } - loc->setVecGuard(std::move(vec)); - location = loc_str.substr(pos + 1); + state->_parsedLocation = std::make_unique<Location>(spec); + state->_parsedLocation->setVecGuard(std::move(vec)); } else { - LOG(warning, "Location string lacks attribute vector specification. loc='%s'", loc_str.c_str()); - location = loc_str; + state->_parsedLocation = std::make_unique<Location>(); + if (! _request.location.empty()) { + LOG(warning, "Error parsing location string '%s': %s", + _request.location.c_str(), locationParser.getParseError()); + } } - loc->parse(location); - return loc; -} -} // namespace - -void -DocsumContext::ParseLocation(search::docsummary::GetDocsumsState *state) -{ - state->_parsedLocation.reset(getLocation(_request.location, _attrMgr)); } std::unique_ptr<MatchingElements> diff --git a/searchcore/src/vespa/searchcore/proton/matching/query.cpp b/searchcore/src/vespa/searchcore/proton/matching/query.cpp index f278b25fd15..68db7f42681 100644 --- a/searchcore/src/vespa/searchcore/proton/matching/query.cpp +++ b/searchcore/src/vespa/searchcore/proton/matching/query.cpp @@ -79,8 +79,8 @@ struct ParsedLocationString { bool valid; string view; search::common::GeoLocationSpec locationSpec; - string loc_string; - ParsedLocationString() : valid(false), view(), locationSpec(), loc_string() {} + string loc_term; + ParsedLocationString() : valid(false), view(), locationSpec(), loc_term() {} ~ParsedLocationString() {} }; @@ -89,18 +89,15 @@ ParsedLocationString parseQueryLocationString(string str) { if (str.empty()) { return retval; } - string::size_type sep = str.find(':'); - if (sep == string::npos) { - LOG(warning, "Location string '%s' lacks attribute vector specification.", str.c_str()); - return retval; - } - retval.view = PositionDataType::getZCurveFieldName(str.substr(0, sep)); - const string loc = str.substr(sep + 1); - if (retval.locationSpec.parseOldFormat(loc)) { - retval.loc_string = loc; + search::common::GeoLocationParser locationParser; + if (locationParser.parseOldFormatWithField(str)) { + auto spec = locationParser.spec(); + retval.locationSpec = spec; + retval.view = PositionDataType::getZCurveFieldName(spec.getFieldName()); + retval.loc_term = str.substr(str.find(':') + 1); retval.valid = true; } else { - LOG(warning, "Location parse error (location: '%s'): %s", loc.c_str(), retval.locationSpec.getParseError()); + LOG(warning, "Location parse error (location: '%s'): %s", str.c_str(), locationParser.getParseError()); } return retval; } @@ -119,9 +116,9 @@ void exchangeLocationNodes(const string &location_str, for (const ProtonLocationTerm * pterm : fix_location_terms(query_tree.get())) { const string view = pterm->getView(); const string loc = pterm->getTerm().getLocationString(); - search::common::GeoLocationSpec loc_spec; - if (loc_spec.parseOldFormat(loc)) { - locationSpecs.emplace_back(view, loc_spec); + search::common::GeoLocationParser loc_parser; + if (loc_parser.parseOldFormat(loc)) { + locationSpecs.emplace_back(view, loc_parser.spec()); } else { LOG(warning, "GeoLocationItem in query had invalid location string: %s", loc.c_str()); } @@ -140,7 +137,7 @@ void exchangeLocationNodes(const string &location_str, if (parsed.valid) { int32_t id = -1; Weight weight(100); - query_tree = inject(std::move(query_tree), std::make_unique<ProtonLocationTerm>(parsed.loc_string, parsed.view, id, weight)); + query_tree = inject(std::move(query_tree), std::make_unique<ProtonLocationTerm>(parsed.loc_term, parsed.view, id, weight)); } } diff --git a/searchlib/src/tests/common/location/geo_location_test.cpp b/searchlib/src/tests/common/location/geo_location_test.cpp index 452adea2579..b29133172df 100644 --- a/searchlib/src/tests/common/location/geo_location_test.cpp +++ b/searchlib/src/tests/common/location/geo_location_test.cpp @@ -4,17 +4,18 @@ #include <vespa/searchlib/common/geo_location_spec.h> #include <vespa/vespalib/gtest/gtest.h> +using search::common::GeoLocationParser; using search::common::GeoLocationSpec; bool is_parseable(const char *str) { - GeoLocationSpec loc; - return loc.parseOldFormat(str); + GeoLocationParser parser; + return parser.parseOldFormat(str); } GeoLocationSpec parse(const char *str) { - GeoLocationSpec loc; - EXPECT_TRUE(loc.parseOldFormat(str)); - return loc; + GeoLocationParser parser; + EXPECT_TRUE(parser.parseOldFormat(str)); + return parser.spec(); } TEST(GeoLocationSpec, malformed_bounding_boxes_are_not_parseable) { diff --git a/searchlib/src/tests/common/location/location_test.cpp b/searchlib/src/tests/common/location/location_test.cpp index d781e5b7275..79eec441d62 100644 --- a/searchlib/src/tests/common/location/location_test.cpp +++ b/searchlib/src/tests/common/location/location_test.cpp @@ -3,43 +3,16 @@ #include <vespa/searchlib/common/location.h> #include <vespa/searchlib/attribute/attributeguard.h> - using search::common::Location; - -bool is_parseable(const char *str) { - Location loc; - return loc.parse(str); -} +using search::common::GeoLocationParser; +using search::common::GeoLocationSpec; Location parse(const char *str) { - Location loc; - if (!EXPECT_TRUE(loc.parse(str))) { - fprintf(stderr, " parse error: %s\n", loc.getParseError()); + GeoLocationParser parser; + if (!EXPECT_TRUE(parser.parseOldFormat(str))) { + fprintf(stderr, " parse error: %s\n", parser.getParseError()); } - return loc; -} - -TEST("require that malformed bounding boxes are not parseable") { - EXPECT_TRUE(is_parseable("[2,10,20,30,40]")); - EXPECT_FALSE(is_parseable("[2,10,20,30,40][2,10,20,30,40]")); - EXPECT_FALSE(is_parseable("[1,10,20,30,40]")); - EXPECT_FALSE(is_parseable("[3,10,20,30,40]")); - EXPECT_FALSE(is_parseable("[2, 10, 20, 30, 40]")); - EXPECT_FALSE(is_parseable("[2,10,20,30,40")); - EXPECT_FALSE(is_parseable("[2,10,20,30]")); - EXPECT_FALSE(is_parseable("[10,20,30,40]")); -} - -TEST("require that malformed circles are not parseable") { - EXPECT_TRUE(is_parseable("(2,10,20,5,0,0,0)")); - EXPECT_FALSE(is_parseable("(2,10,20,5,0,0,0)(2,10,20,5,0,0,0)")); - EXPECT_FALSE(is_parseable("(1,10,20,5,0,0,0)")); - EXPECT_FALSE(is_parseable("(3,10,20,5,0,0,0)")); - EXPECT_FALSE(is_parseable("(2, 10, 20, 5, 0, 0, 0)")); - EXPECT_FALSE(is_parseable("(2,10,20,5)")); - EXPECT_FALSE(is_parseable("(2,10,20,5,0,0,0")); - EXPECT_FALSE(is_parseable("(2,10,20,5,0,0,0,1000")); - EXPECT_FALSE(is_parseable("(10,20,5)")); + return Location(parser.spec()); } TEST("require that bounding boxes can be parsed") { diff --git a/searchlib/src/vespa/searchlib/attribute/attribute_blueprint_factory.cpp b/searchlib/src/vespa/searchlib/attribute/attribute_blueprint_factory.cpp index 62f4661b335..5148e6e02d2 100644 --- a/searchlib/src/vespa/searchlib/attribute/attribute_blueprint_factory.cpp +++ b/searchlib/src/vespa/searchlib/attribute/attribute_blueprint_factory.cpp @@ -250,8 +250,11 @@ public: _attribute(attribute), _location() { - _location.setVec(attribute); - _location.parse(loc.getLocationString()); + search::common::GeoLocationParser parser; + if (parser.parseOldFormat(loc.getLocationString())) { + _location.setVec(attribute); + _location.setSpec(parser.spec()); + } uint32_t estHits = _attribute.getNumDocs(); LOG(debug, "location %s in attribute with numdocs %u", loc.getLocationString().c_str(), estHits); HitEstimate estimate(estHits, estHits == 0); diff --git a/searchlib/src/vespa/searchlib/common/geo_location_spec.cpp b/searchlib/src/vespa/searchlib/common/geo_location_spec.cpp index 0535f70aabb..fc1cfc4e99c 100644 --- a/searchlib/src/vespa/searchlib/common/geo_location_spec.cpp +++ b/searchlib/src/vespa/searchlib/common/geo_location_spec.cpp @@ -28,9 +28,9 @@ namespace search::common { GeoLocationSpec::GeoLocationSpec() : _valid(false), - _hasPoint(false), - _hasRadius(false), - _hasBoundingBox(false), + _has_point(false), + _has_radius(false), + _has_bounding_box(false), _field_name(), _x(0), _y(0), @@ -39,26 +39,9 @@ GeoLocationSpec::GeoLocationSpec() : _min_x(std::numeric_limits<int32_t>::min()), _max_x(std::numeric_limits<int32_t>::max()), _min_y(std::numeric_limits<int32_t>::min()), - _max_y(std::numeric_limits<int32_t>::max()), - _parseError(NULL) -{ -} - + _max_y(std::numeric_limits<int32_t>::max()) +{} -bool -GeoLocationSpec::getDimensionality(const char * &p) { - if (*p == '2') { - p++; - if (*p != ',') { - _parseError = "Missing comma after 2D dimensionality"; - return false; - } - p++; - return true; - } - _parseError = "Bad dimensionality spec, not 2D"; - return false; -} std::string GeoLocationSpec::getLocationString() const @@ -87,8 +70,38 @@ GeoLocationSpec::getLocationString() const return loc.str(); } +std::string +GeoLocationSpec::getLocationStringWithField() const +{ + if (hasFieldName()) { + return getFieldName() + ":" + getLocationString(); + } else { + return getLocationString(); + } +} + + +GeoLocationParser::GeoLocationParser() + : GeoLocationSpec(), _parseError(NULL) +{} + bool -GeoLocationSpec::parseOldFormatWithField(const std::string &str) +GeoLocationParser::getDimensionality(const char * &p) { + if (*p == '2') { + p++; + if (*p != ',') { + _parseError = "Missing comma after 2D dimensionality"; + return false; + } + p++; + return true; + } + _parseError = "Bad dimensionality spec, not 2D"; + return false; +} + +bool +GeoLocationParser::parseOldFormatWithField(const std::string &str) { auto sep = str.find(':'); if (sep == std::string::npos) { @@ -101,7 +114,7 @@ GeoLocationSpec::parseOldFormatWithField(const std::string &str) } bool -GeoLocationSpec::parseOldFormat(const std::string &locStr) +GeoLocationParser::parseOldFormat(const std::string &locStr) { bool foundBoundingBox = false; bool foundLoc = false; @@ -201,9 +214,8 @@ GeoLocationSpec::parseOldFormat(const std::string &locStr) return false; } } - if (foundLoc) { - _hasRadius = (_radius != std::numeric_limits<uint32_t>::max()); + _has_radius = (_radius != std::numeric_limits<uint32_t>::max()); uint32_t maxdx = _radius; if (_x_aspect != 0) { uint64_t maxdx2 = ((static_cast<uint64_t>(_radius) << 32) + 0xffffffffu) / @@ -225,10 +237,13 @@ GeoLocationSpec::parseOldFormat(const std::string &locStr) if (implied_max_y < _max_y) _max_y = implied_max_y; if (implied_min_y > _min_y) _min_y = implied_min_y; } - _hasPoint = foundLoc; - _hasBoundingBox = foundBoundingBox || _hasRadius; - _valid = (_hasPoint || _hasBoundingBox); + _has_point = foundLoc; + _has_bounding_box = ((_min_x != std::numeric_limits<int32_t>::min()) || + (_max_x != std::numeric_limits<int32_t>::max()) || + (_min_y != std::numeric_limits<int32_t>::min()) || + (_max_y != std::numeric_limits<int32_t>::max())); + _valid = (_has_point || _has_bounding_box); return _valid; } -} +} // namespace diff --git a/searchlib/src/vespa/searchlib/common/geo_location_spec.h b/searchlib/src/vespa/searchlib/common/geo_location_spec.h index 1a848e9d7ea..853ad43ff46 100644 --- a/searchlib/src/vespa/searchlib/common/geo_location_spec.h +++ b/searchlib/src/vespa/searchlib/common/geo_location_spec.h @@ -13,31 +13,29 @@ public: GeoLocationSpec(); bool isValid() const { return _valid; } - bool hasPoint() const { return _hasPoint; } - bool hasBoundingBox() const { return _hasBoundingBox; } + bool hasPoint() const { return _has_point; } + bool hasBoundingBox() const { return _has_bounding_box; } bool hasFieldName() const { return ! _field_name.empty(); } uint32_t getXAspect() const { return _x_aspect; } int32_t getX() const { return _x; } int32_t getY() const { return _y; } uint32_t getRadius() const { return _radius; } - const char * getParseError() const { return _parseError; } int32_t getMinX() const { return _min_x; } int32_t getMinY() const { return _min_y; } int32_t getMaxX() const { return _max_x; } int32_t getMaxY() const { return _max_y; } - bool parseOldFormat(const std::string &locStr); - bool parseOldFormatWithField(const std::string &str); - std::string getLocationString() const; + std::string getLocationStringWithField() const; + std::string getFieldName() const { return _field_name; } -private: +protected: bool _valid; - bool _hasPoint; - bool _hasRadius; - bool _hasBoundingBox; + bool _has_point; + bool _has_radius; + bool _has_bounding_box; std::string _field_name; @@ -49,7 +47,17 @@ private: int32_t _max_x; /* Max X coordinate */ int32_t _min_y; /* Min Y coordinate */ int32_t _max_y; /* Max Y coordinate */ +}; +class GeoLocationParser : private GeoLocationSpec +{ +public: + GeoLocationParser(); + bool parseOldFormat(const std::string &locStr); + bool parseOldFormatWithField(const std::string &str); + GeoLocationSpec spec() const { return *this; } + const char * getParseError() const { return _parseError; } +private: const char *_parseError; bool getDimensionality(const char * &p); }; diff --git a/searchlib/src/vespa/searchlib/common/location.cpp b/searchlib/src/vespa/searchlib/common/location.cpp index bb8df6dcf26..ff87d5a260c 100644 --- a/searchlib/src/vespa/searchlib/common/location.cpp +++ b/searchlib/src/vespa/searchlib/common/location.cpp @@ -7,15 +7,34 @@ namespace search::common { Location::Location() : _zBoundingBox(0,0,0,0) {} +Location::Location(const GeoLocationSpec &other) + : _zBoundingBox(0,0,0,0) +{ + setSpec(other); +} + +void +Location::setSpec(const GeoLocationSpec &other) +{ + using vespalib::geo::ZCurve; + + GeoLocationSpec::operator=(other); + if (isValid()) { + _zBoundingBox = ZCurve::BoundingBox(getMinX(), getMaxX(), + getMinY(), getMaxY()); + } +} + + +#if 0 bool Location::parse(const std::string &locStr) { bool valid = GeoLocationSpec::parseOldFormat(locStr); if (valid) { - _zBoundingBox = vespalib::geo::ZCurve::BoundingBox(getMinX(), - getMaxX(), - getMinY(), - getMaxY()); + _zBoundingBox = vespalib::geo::ZCurve::BoundingBox(getMinX(), getMaxX(), + getMinY(), getMaxY()); } return valid; } +#endif } // namespace diff --git a/searchlib/src/vespa/searchlib/common/location.h b/searchlib/src/vespa/searchlib/common/location.h index 72b7bbc7ca4..c73fe7b817c 100644 --- a/searchlib/src/vespa/searchlib/common/location.h +++ b/searchlib/src/vespa/searchlib/common/location.h @@ -16,6 +16,7 @@ class Location : public DocumentLocations, { public: Location(); + Location(const GeoLocationSpec& other); ~Location() {} Location(Location &&) = default; bool getRankOnDistance() const { return hasPoint(); } @@ -23,7 +24,8 @@ public: bool getzFailBoundingBoxTest(int64_t docxy) const { return _zBoundingBox.getzFailBoundingBoxTest(docxy); } - bool parse(const std::string &locStr); + // bool parse(const std::string &locStr); + void setSpec(const GeoLocationSpec& other); private: GeoLocationSpec _spec; vespalib::geo::ZCurve::BoundingBox _zBoundingBox; diff --git a/searchsummary/src/vespa/searchsummary/docsummary/positionsdfw.cpp b/searchsummary/src/vespa/searchsummary/docsummary/positionsdfw.cpp index ecdde13b919..f49a382e9b5 100644 --- a/searchsummary/src/vespa/searchsummary/docsummary/positionsdfw.cpp +++ b/searchsummary/src/vespa/searchsummary/docsummary/positionsdfw.cpp @@ -76,7 +76,7 @@ AbsDistanceDFW::insertField(uint32_t docid, GetDocsumsState *state, ResType type state->_callback.ParseLocation(state); } assert(state->_parsedLocation); - if (state->_parsedLocation->getParseError() == nullptr) { + if (state->_parsedLocation->isValid()) { forceEmpty = false; } } diff --git a/streamingvisitors/src/vespa/searchvisitor/queryenvironment.cpp b/streamingvisitors/src/vespa/searchvisitor/queryenvironment.cpp index 70d304b4a87..5112ce59dfb 100644 --- a/streamingvisitors/src/vespa/searchvisitor/queryenvironment.cpp +++ b/streamingvisitors/src/vespa/searchvisitor/queryenvironment.cpp @@ -21,22 +21,14 @@ parseLocation(const string & location_str) if (location_str.empty()) { return fefLocation; } - string::size_type pos = location_str.find(':'); - if (pos == string::npos) { - LOG(warning, "Location string lacks attribute vector specification. loc='%s'. Location ignored.", - location_str.c_str()); - return fefLocation; - } - string attr = location_str.substr(0, pos); - const string location = location_str.substr(pos + 1); - - search::common::GeoLocationSpec locationSpec; - if (!locationSpec.parseOldFormat(location)) { + search::common::GeoLocationParser locationParser; + if (!locationParser.parseOldFormatWithField(location_str)) { LOG(warning, "Location parse error (location: '%s'): %s. Location ignored.", - location.c_str(), locationSpec.getParseError()); + location_str.c_str(), locationParser.getParseError()); return fefLocation; } - fefLocation.setAttribute(attr); + auto locationSpec = locationParser.spec(); + fefLocation.setAttribute(locationSpec.getFieldName()); fefLocation.setXPosition(locationSpec.getX()); fefLocation.setYPosition(locationSpec.getY()); fefLocation.setXAspect(locationSpec.getXAspect()); |