split out geo location parser into its own distinct class

7 files changed, 106 insertions, 85 deletions

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;