Merge pull request #14080 from vespa-engine/arnej/use-added-json-geo-format

Arnej/use added json geo format

9 files changed, 50 insertions, 70 deletions

diff --git a/searchcore/src/tests/proton/matching/query_test.cpp b/searchcore/src/tests/proton/matching/query_test.cpp
index 01f7e25eb51..7cc11b0dc0c 100644
--- a/searchcore/src/tests/proton/matching/query_test.cpp
+++ b/searchcore/src/tests/proton/matching/query_test.cpp
@@ -698,7 +698,7 @@ void Test::requireThatQueryGluesEverythingTogether() {
     ASSERT_TRUE(search.get());
 }
 
-void checkQueryAddsLocation(Test &test, const string &loc_string) {
+void checkQueryAddsLocation(Test &test, const string &loc_in, const string &loc_out) {
     fef_test::IndexEnvironment index_environment;
     FieldInfo field_info(FieldType::INDEX, CollectionType::SINGLE, field, 0);
     index_environment.getFields().push_back(field_info);
@@ -712,7 +712,7 @@ void checkQueryAddsLocation(Test &test, const string &loc_string) {
 
     Query query;
     query.buildTree(stack_dump,
-                    loc_field + ":" + loc_string,
+                    loc_field + ":" + loc_in,
                     ViewResolver(), index_environment);
     vector<const ITermData *> term_data;
     query.extractTerms(term_data);
@@ -729,9 +729,9 @@ void checkQueryAddsLocation(Test &test, const string &loc_string) {
     query.fetchPostings();
     SearchIterator::UP search = query.createSearch(*md);
     test.ASSERT_TRUE(search.get());
-    if (!test.EXPECT_NOT_EQUAL(string::npos, search->asString().find(loc_string))) {
-        fprintf(stderr, "search (missing loc_string '%s'): %s",
-                loc_string.c_str(), search->asString().c_str());
+    if (!test.EXPECT_NOT_EQUAL(string::npos, search->asString().find(loc_out))) {
+        fprintf(stderr, "search (missing loc_out '%s'): %s",
+                loc_out.c_str(), search->asString().c_str());
     }
 }
 
@@ -790,11 +790,15 @@ void Test::requireThatLocationIsAddedTheCorrectPlace() {
 }
 
 void Test::requireThatQueryAddsLocation() {
-    checkQueryAddsLocation(*this, "(2,10,10,3,0,1,0,0)");
+    checkQueryAddsLocation(*this, "(2,10,10,3,0,1,0,0)", "{p:{x:10,y:10},r:3,b:{x:[7,13],y:[7,13]}}");
+    checkQueryAddsLocation(*this, "{p:{x:10,y:10},r:3}", "{p:{x:10,y:10},r:3,b:{x:[7,13],y:[7,13]}}");
+    checkQueryAddsLocation(*this, "{b:{x:[6,11],y:[8,15]},p:{x:10,y:10},r:3}", "{p:{x:10,y:10},r:3,b:{x:[7,11],y:[8,13]}}");
+    checkQueryAddsLocation(*this, "{a:12345,b:{x:[8,10],y:[8,10]},p:{x:10,y:10},r:3}", "{p:{x:10,y:10},r:3,a:12345,b:{x:[8,10],y:[8,10]}}");
 }
 
 void Test::requireThatQueryAddsLocationCutoff() {
-    checkQueryAddsLocation(*this, "[2,10,10,20,20]");
+    checkQueryAddsLocation(*this, "[2,10,11,23,24]", "{b:{x:[10,23],y:[11,24]}}");
+    checkQueryAddsLocation(*this, "{b:{y:[11,24],x:[10,23]}}", "{b:{x:[10,23],y:[11,24]}}");
 }
 
 void
diff --git a/searchcore/src/vespa/searchcore/proton/matching/query.cpp b/searchcore/src/vespa/searchcore/proton/matching/query.cpp
index 65959e6e6de..994e26081e7 100644
--- a/searchcore/src/vespa/searchcore/proton/matching/query.cpp
+++ b/searchcore/src/vespa/searchcore/proton/matching/query.cpp
@@ -83,7 +83,7 @@ GeoLocationSpec parse_location_string(string str) {
         return empty;
     }
     GeoLocationParser parser;
-    if (parser.parseOldFormatWithField(str)) {
+    if (parser.parseWithField(str)) {
         auto attr_name = PositionDataType::getZCurveFieldName(parser.getFieldName());
         return GeoLocationSpec{attr_name, parser.getGeoLocation()};
     } else {
diff --git a/searchlib/src/tests/common/location/geo_location_test.cpp b/searchlib/src/tests/common/location/geo_location_test.cpp
index eec2411f2af..d142733041e 100644
--- a/searchlib/src/tests/common/location/geo_location_test.cpp
+++ b/searchlib/src/tests/common/location/geo_location_test.cpp
@@ -18,12 +18,7 @@ using Aspect = search::common::GeoLocation::Aspect;
 constexpr int32_t plus_inf = std::numeric_limits<int32_t>::max();
 constexpr int32_t minus_inf = std::numeric_limits<int32_t>::min();
 
-bool is_parseable(const char *str) {
-    GeoLocationParser parser;
-    return parser.parseOldFormat(str);
-}
-
-bool is_parseable_new(const char *str, bool with_field = false) {
+bool is_parseable(const char *str, bool with_field = false) {
     GeoLocationParser parser;
     if (with_field) {
         return parser.parseWithField(str);
@@ -31,13 +26,7 @@ bool is_parseable_new(const char *str, bool with_field = false) {
     return parser.parseNoField(str);
 }
 
-GeoLocation parse(const char *str) {
-    GeoLocationParser parser;
-    EXPECT_TRUE(parser.parseOldFormat(str));
-    return parser.getGeoLocation();
-}
-
-GeoLocation parse_new(const std::string &str, bool with_field = false) {
+GeoLocation parse(const std::string &str, bool with_field = false) {
     GeoLocationParser parser;
     if (with_field) {
         EXPECT_TRUE(parser.parseWithField(str));
@@ -59,12 +48,12 @@ TEST(GeoLocationParserTest, malformed_bounding_boxes_are_not_parseable) {
 }
 
 TEST(GeoLocationParserTest, new_bounding_box_formats) {
-    EXPECT_TRUE(is_parseable_new("{b:{x:[10,30],y:[20,40]}}"));
-    EXPECT_TRUE(is_parseable_new("{b:{}}"));
-    EXPECT_TRUE(is_parseable_new("{b:[]}"));
-    EXPECT_TRUE(is_parseable_new("{b:10,b:20}"));
-    EXPECT_TRUE(is_parseable_new("{b:[10, 20, 30, 40]}"));
-    EXPECT_FALSE(is_parseable_new("{b:{x:[10,30],y:[20,40]}"));
+    EXPECT_TRUE(is_parseable("{b:{x:[10,30],y:[20,40]}}"));
+    EXPECT_TRUE(is_parseable("{b:{}}"));
+    EXPECT_TRUE(is_parseable("{b:[]}"));
+    EXPECT_TRUE(is_parseable("{b:10,b:20}"));
+    EXPECT_TRUE(is_parseable("{b:[10, 20, 30, 40]}"));
+    EXPECT_FALSE(is_parseable("{b:{x:[10,30],y:[20,40]}"));
 }
 
 TEST(GeoLocationParserTest, malformed_circles_are_not_parseable) {
@@ -80,23 +69,23 @@ TEST(GeoLocationParserTest, malformed_circles_are_not_parseable) {
 }
 
 TEST(GeoLocationParserTest, new_circle_formats) {
-    EXPECT_TRUE(is_parseable_new("{p:{x:10,y:20}}"));
-    EXPECT_TRUE(is_parseable_new("{p:{x:10,y:20},r:5}"));
-    EXPECT_TRUE(is_parseable_new("{p:{x:10, y:10}, r:5}"));
-    EXPECT_TRUE(is_parseable_new("{'p':{y:20,x:10},'r':5}"));
-    EXPECT_TRUE(is_parseable_new("{\n \"p\": { \"x\": 10, \"y\": 20},\n \"r\": 5\n}"));
+    EXPECT_TRUE(is_parseable("{p:{x:10,y:20}}"));
+    EXPECT_TRUE(is_parseable("{p:{x:10,y:20},r:5}"));
+    EXPECT_TRUE(is_parseable("{p:{x:10, y:10}, r:5}"));
+    EXPECT_TRUE(is_parseable("{'p':{y:20,x:10},'r':5}"));
+    EXPECT_TRUE(is_parseable("{\n \"p\": { \"x\": 10, \"y\": 20},\n \"r\": 5\n}"));
     // json demands colon:
-    EXPECT_FALSE(is_parseable_new("{p:{x:10,y:10},r=5}"));
+    EXPECT_FALSE(is_parseable("{p:{x:10,y:10},r=5}"));
     // missing y -> 0 default:
-    EXPECT_TRUE(is_parseable_new("{p:{x:10},r:5}"));
+    EXPECT_TRUE(is_parseable("{p:{x:10},r:5}"));
     // unused extra fields are ignored:
-    EXPECT_TRUE(is_parseable_new("{p:{x:10,y:10,z:10},r:5,c:1,d:17}"));
+    EXPECT_TRUE(is_parseable("{p:{x:10,y:10,z:10},r:5,c:1,d:17}"));
 }
 
 TEST(GeoLocationParserTest, bounding_boxes_can_be_parsed) {
     for (const auto & loc : {
         parse("[2,10,20,30,40]"),
-        parse_new("{b:{x:[10,30],y:[20,40]}}")
+        parse("{b:{x:[10,30],y:[20,40]}}")
     }) {
         EXPECT_EQ(false, loc.has_point);
         EXPECT_EQ(true, loc.bounding_box.active());
@@ -114,7 +103,7 @@ TEST(GeoLocationParserTest, bounding_boxes_can_be_parsed) {
 TEST(GeoLocationParserTest, circles_can_be_parsed) {
     for (const auto & loc : {
         parse("(2,10,20,5,0,0,0)"),
-        parse_new("{p:{x:10,y:20},r:5}")
+        parse("{p:{x:10,y:20},r:5}")
     }) {
         EXPECT_EQ(true, loc.has_point);
         EXPECT_EQ(true, loc.bounding_box.active());
@@ -132,7 +121,7 @@ TEST(GeoLocationParserTest, circles_can_be_parsed) {
 TEST(GeoLocationParserTest, circles_can_have_aspect_ratio) {
     for (const auto & loc : {
         parse("(2,10,20,5,0,0,0,2147483648)"),
-        parse_new("{p:{x:10,y:20},r:5,a:2147483648}")
+        parse("{p:{x:10,y:20},r:5,a:2147483648}")
     }) {
         EXPECT_EQ(true, loc.has_point);
         EXPECT_EQ(true, loc.bounding_box.active());
@@ -145,14 +134,14 @@ TEST(GeoLocationParserTest, circles_can_have_aspect_ratio) {
         EXPECT_EQ(21, loc.bounding_box.x.high);
         EXPECT_EQ(25, loc.bounding_box.y.high);
     }
-    auto loc2 = parse_new("{p:{x:10,y:10},a:3123456789}");
+    auto loc2 = parse("{p:{x:10,y:10},a:3123456789}");
     EXPECT_EQ(3123456789, loc2.x_aspect.multiplier);
 }
 
 TEST(GeoLocationParserTest, bounding_box_can_be_specified_after_circle) {
     for (const auto & loc : {
         parse("(2,10,20,5,0,0,0)[2,10,20,30,40]"),
-        parse_new("{p:{x:10,y:20},r:5,b:{x:[10,30],y:[20,40]}}")
+        parse("{p:{x:10,y:20},r:5,b:{x:[10,30],y:[20,40]}}")
     }) {
         EXPECT_EQ(true, loc.has_point);
         EXPECT_EQ(true, loc.bounding_box.active());
@@ -170,7 +159,7 @@ TEST(GeoLocationParserTest, bounding_box_can_be_specified_after_circle) {
 TEST(GeoLocationParserTest, circles_can_be_specified_after_bounding_box) {
     for (const auto & loc : {
         parse("[2,10,20,30,40](2,10,20,5,0,0,0)"),
-        parse_new("{b:{x:[10,30],y:[20,40]},p:{x:10,y:20},r:5}")
+        parse("{b:{x:[10,30],y:[20,40]},p:{x:10,y:20},r:5}")
     }) {
         EXPECT_EQ(true, loc.has_point);
         EXPECT_EQ(true, loc.bounding_box.active());
@@ -183,7 +172,7 @@ TEST(GeoLocationParserTest, circles_can_be_specified_after_bounding_box) {
         EXPECT_EQ(15, loc.bounding_box.x.high);
         EXPECT_EQ(25, loc.bounding_box.y.high);    
     }
-    const auto &loc = parse_new("{a:12345,b:{x:[8,10],y:[8,10]},p:{x:10,y:10},r:3}");
+    const auto &loc = parse("{a:12345,b:{x:[8,10],y:[8,10]},p:{x:10,y:10},r:3}");
     EXPECT_EQ(true, loc.has_point);
     EXPECT_EQ(10, loc.point.x);
     EXPECT_EQ(10, loc.point.y);
@@ -193,7 +182,7 @@ TEST(GeoLocationParserTest, circles_can_be_specified_after_bounding_box) {
 TEST(GeoLocationParserTest, santa_search_gives_non_wrapped_bounding_box) {
     for (const auto & loc : {
         parse("(2,122163600,89998536,290112,4,2000,0,109704)"),
-        parse_new("{p:{x:122163600,y:89998536},r:290112,a:109704}")
+        parse("{p:{x:122163600,y:89998536},r:290112,a:109704}")
     }) {
         EXPECT_GE(loc.bounding_box.x.high, loc.bounding_box.x.low);
         EXPECT_GE(loc.bounding_box.y.high, loc.bounding_box.y.low);
@@ -203,7 +192,7 @@ TEST(GeoLocationParserTest, santa_search_gives_non_wrapped_bounding_box) {
 TEST(GeoLocationParserTest, near_boundary_search_gives_non_wrapped_bounding_box) {
     for (const auto & loc1 : {
         parse("(2,2000000000,2000000000,3000000000,0,1,0)"),
-        parse_new("{p:{x:2000000000,y:2000000000},r:3000000000}")
+        parse("{p:{x:2000000000,y:2000000000},r:3000000000}")
     }) {
         EXPECT_GE(loc1.bounding_box.x.high, loc1.bounding_box.x.low);
         EXPECT_GE(loc1.bounding_box.y.high, loc1.bounding_box.y.low);
@@ -213,7 +202,7 @@ TEST(GeoLocationParserTest, near_boundary_search_gives_non_wrapped_bounding_box)
     
     for (const auto & loc2 : {
         parse("(2,-2000000000,-2000000000,3000000000,0,1,0)"),
-        parse_new("{p:{x:-2000000000,y:-2000000000},r:3000000000}")
+        parse("{p:{x:-2000000000,y:-2000000000},r:3000000000}")
     }) {
         EXPECT_GE(loc2.bounding_box.x.high, loc2.bounding_box.x.low);
         EXPECT_GE(loc2.bounding_box.y.high, loc2.bounding_box.y.low);
@@ -479,7 +468,7 @@ TEST(GeoLocationParserTest, can_parse_what_query_tree_produces) {
 
     search::query::Location loc_1(point_1);
     std::string str_1 = loc_1.getJsonFormatString();
-    auto result_1 = parse_new(str_1);
+    auto result_1 = parse(str_1);
 
     EXPECT_EQ(true, result_1.has_point);
     EXPECT_EQ(false, result_1.has_radius());
@@ -490,7 +479,7 @@ TEST(GeoLocationParserTest, can_parse_what_query_tree_produces) {
 
     search::query::Location loc_1b(point_1, distance, aspect_ratio);
     std::string str_1b = loc_1b.getJsonFormatString();
-    auto result_1b = parse_new(str_1b);
+    auto result_1b = parse(str_1b);
 
     EXPECT_EQ(true, result_1b.has_point);
     EXPECT_EQ(true, result_1b.has_radius());
@@ -505,7 +494,7 @@ TEST(GeoLocationParserTest, can_parse_what_query_tree_produces) {
 
     search::query::Location loc_2(rectangle_1);
     std::string str_2 = loc_2.getJsonFormatString();
-    auto result_2 = parse_new(str_2);
+    auto result_2 = parse(str_2);
 
     EXPECT_EQ(false, result_2.has_point);
     EXPECT_EQ(false, result_2.has_radius());
@@ -518,7 +507,7 @@ TEST(GeoLocationParserTest, can_parse_what_query_tree_produces) {
 
     search::query::Location loc_3(rectangle_1, point_1, distance, aspect_ratio);
     std::string str_3 = loc_3.getJsonFormatString();
-    auto result_3 = parse_new(str_3);
+    auto result_3 = parse(str_3);
 
     EXPECT_EQ(true, result_3.has_point);
     EXPECT_EQ(true, result_3.has_radius());
diff --git a/searchlib/src/vespa/searchlib/common/geo_location_parser.cpp b/searchlib/src/vespa/searchlib/common/geo_location_parser.cpp
index 53792e56562..7d793f9eb55 100644
--- a/searchlib/src/vespa/searchlib/common/geo_location_parser.cpp
+++ b/searchlib/src/vespa/searchlib/common/geo_location_parser.cpp
@@ -63,19 +63,6 @@ GeoLocationParser::correctDimensionalitySkip(const char * &p) {
 }
 
 bool
-GeoLocationParser::parseOldFormatWithField(const std::string &str)
-{
-     auto sep = str.find(':');
-     if (sep == std::string::npos) {
-         _parseError = "Location string lacks field specification";
-         return false;
-     }
-     _field_name = str.substr(0, sep);
-     std::string only_loc = str.substr(sep + 1);
-     return parseOldFormat(only_loc);
-}
-
-bool
 GeoLocationParser::parseOldFormat(const std::string &locStr)
 {
     bool foundBoundingBox = false;
diff --git a/searchlib/src/vespa/searchlib/common/geo_location_parser.h b/searchlib/src/vespa/searchlib/common/geo_location_parser.h
index 7d725f4a035..e1a68163fbb 100644
--- a/searchlib/src/vespa/searchlib/common/geo_location_parser.h
+++ b/searchlib/src/vespa/searchlib/common/geo_location_parser.h
@@ -20,9 +20,6 @@ public:
     bool parseNoField(const std::string &locStr);
     bool parseWithField(const std::string &locStr);
 
-    bool parseOldFormat(const std::string &locStr);
-    bool parseOldFormatWithField(const std::string &str);
-
     std::string getFieldName() const { return _field_name; }
     GeoLocation getGeoLocation() const;
 
@@ -46,6 +43,7 @@ private:
     const char *_parseError;
     bool correctDimensionalitySkip(const char * &p);
     bool parseJsonFormat(const std::string &locStr);
+    bool parseOldFormat(const std::string &locStr);
 };
 
 } // namespace
diff --git a/searchlib/src/vespa/searchlib/query/tree/location.cpp b/searchlib/src/vespa/searchlib/query/tree/location.cpp
index 44f0b82d304..9b45ba18b97 100644
--- a/searchlib/src/vespa/searchlib/query/tree/location.cpp
+++ b/searchlib/src/vespa/searchlib/query/tree/location.cpp
@@ -100,7 +100,7 @@ Location::getJsonFormatString() const
 }
 
 vespalib::asciistream &operator<<(vespalib::asciistream &out, const Location &loc) {
-    return out << loc.getOldFormatString();
+    return out << loc.getJsonFormatString();
 }
 
 } // namespace
diff --git a/searchlib/src/vespa/searchlib/query/tree/stackdumpquerycreator.h b/searchlib/src/vespa/searchlib/query/tree/stackdumpquerycreator.h
index a7e00d41555..66702fcd85c 100644
--- a/searchlib/src/vespa/searchlib/query/tree/stackdumpquerycreator.h
+++ b/searchlib/src/vespa/searchlib/query/tree/stackdumpquerycreator.h
@@ -144,7 +144,9 @@ private:
                 t = &builder.addSuffixTerm(term, view, id, weight);
             } else if (type == ParseItem::ITEM_GEO_LOCATION_TERM) {
                 search::common::GeoLocationParser parser;
-                parser.parseOldFormat(term);
+                if (! parser.parseNoField(term)) {
+                    LOG(warning, "invalid geo location term '%s'", term.data());
+                }
                 Location loc(parser.getGeoLocation());
                 t = &builder.addLocationTerm(loc, view, id, weight);
             } else if (type == ParseItem::ITEM_NUMTERM) {
diff --git a/searchsummary/src/vespa/searchsummary/docsummary/docsumstate.cpp b/searchsummary/src/vespa/searchsummary/docsummary/docsumstate.cpp
index 1f9b553b56a..822017e0bdf 100644
--- a/searchsummary/src/vespa/searchsummary/docsummary/docsumstate.cpp
+++ b/searchsummary/src/vespa/searchsummary/docsummary/docsumstate.cpp
@@ -76,7 +76,7 @@ GetDocsumsState::parse_locations()
     assert(_parsedLocations.empty()); // only allowed to call this once
     if (! _args.getLocation().empty()) {
         GeoLocationParser parser;
-        if (parser.parseOldFormatWithField(_args.getLocation())) {
+        if (parser.parseWithField(_args.getLocation())) {
             auto view = parser.getFieldName();
             auto attr_name = PositionDataType::getZCurveFieldName(view);
             GeoLocationSpec spec{attr_name, parser.getGeoLocation()};
@@ -94,7 +94,7 @@ GetDocsumsState::parse_locations()
                 vespalib::string view = iterator.getIndexName();
                 vespalib::string term = iterator.getTerm();
                 GeoLocationParser parser;                
-                if (parser.parseOldFormat(term)) {
+                if (parser.parseNoField(term)) {
                     auto attr_name = PositionDataType::getZCurveFieldName(view);
                     GeoLocationSpec spec{attr_name, parser.getGeoLocation()};
                     _parsedLocations.push_back(spec);
diff --git a/streamingvisitors/src/vespa/searchvisitor/queryenvironment.cpp b/streamingvisitors/src/vespa/searchvisitor/queryenvironment.cpp
index 06bdb9f69bb..383f6c3d44b 100644
--- a/streamingvisitors/src/vespa/searchvisitor/queryenvironment.cpp
+++ b/streamingvisitors/src/vespa/searchvisitor/queryenvironment.cpp
@@ -26,7 +26,7 @@ parseLocation(const string & location_str)
         return fefLocations;
     }
     GeoLocationParser locationParser;
-    if (!locationParser.parseOldFormatWithField(location_str)) {
+    if (!locationParser.parseWithField(location_str)) {
         LOG(warning, "Location parse error (location: '%s'): %s. Location ignored.",
                      location_str.c_str(), locationParser.getParseError());
         return fefLocations;