Merge pull request #13908 from vespa-engine/arnej/remove-fef-location

Arnej/remove fef location

11 files changed, 51 insertions, 173 deletions

diff --git a/searchlib/src/tests/features/prod_features.cpp b/searchlib/src/tests/features/prod_features.cpp
index f886ba59c1c..25b5ba20d26 100644
--- a/searchlib/src/tests/features/prod_features.cpp
+++ b/searchlib/src/tests/features/prod_features.cpp
@@ -62,6 +62,8 @@ using search::StringAttribute;
 using search::SingleBoolAttribute;
 using search::WeightedSetStringExtAttribute;
 using search::attribute::WeightedEnumContent;
+using search::common::GeoLocation;
+using search::common::GeoLocationSpec;
 
 using AttributePtr = AttributeVector::SP;
 using AVC = search::attribute::Config;
@@ -507,8 +509,8 @@ Test::assertCloseness(feature_t exp, const vespalib::string & attr, double dista
     int32_t x = 0;
     positions.emplace_back(x, x);
     setupForDistanceTest(ft, "pos", positions, false);
-    ft.getQueryEnv().getLocation().setXPosition((int)distance);
-    ft.getQueryEnv().getLocation().setValid(true);
+    GeoLocation::Point p{int32_t(distance), 0};
+    ft.getQueryEnv().addLocation(GeoLocationSpec{attr, p});
     if (maxDistance > 0) {
         ft.getIndexEnv().getProperties().add(feature + ".maxDistance",
                                              vespalib::make_string("%u", (unsigned int)maxDistance));
@@ -857,13 +859,16 @@ Test::testDistance()
             { // non-existing attribute
                 FtFeatureTest ft(_factory, "distance(pos)");
                 ft.getIndexEnv().getBuilder().addField(FieldType::ATTRIBUTE, CollectionType::SINGLE, DataType::INT64, "pos");
-                ft.getQueryEnv().getLocation().setValid(true);
+                GeoLocation::Point p{0, 0};
+                ft.getQueryEnv().addLocation(GeoLocationSpec{"pos", p});
+
                 ASSERT_TRUE(ft.setup());
                 ASSERT_TRUE(ft.execute(RankResult().addScore("distance(pos)", 6400000000.0)));
             }
             { // label
                 FtFeatureTest ft(_factory, "distance(label,foo)");
-                ft.getQueryEnv().getLocation().setValid(true);
+                GeoLocation::Point p{0, 0};
+                ft.getQueryEnv().addLocation(GeoLocationSpec{"pos", p});
                 ASSERT_TRUE(ft.setup());
                 ASSERT_TRUE(ft.execute(RankResult().addScore("distance(label,foo)", std::numeric_limits<feature_t>::max())));
             }
@@ -873,7 +878,8 @@ Test::testDistance()
                 pos->commit();
                 ft.getIndexEnv().getAttributeMap().add(pos);
                 ft.getIndexEnv().getBuilder().addField(FieldType::ATTRIBUTE, CollectionType::SINGLE, DataType::INT64, "pos");
-                ft.getQueryEnv().getLocation().setValid(true);
+                GeoLocation::Point p{0, 0};
+                ft.getQueryEnv().addLocation(GeoLocationSpec{"pos", p});
                 ASSERT_TRUE(ft.setup());
                 ASSERT_TRUE(ft.execute(RankResult().addScore("distance(pos)", 6400000000.0)));
             }
@@ -883,7 +889,8 @@ Test::testDistance()
                 pos->commit();
                 ft.getIndexEnv().getAttributeMap().add(pos);
                 ft.getIndexEnv().getBuilder().addField(FieldType::ATTRIBUTE, CollectionType::SINGLE, DataType::INT64, "pos");
-                ft.getQueryEnv().getLocation().setValid(true);
+                GeoLocation::Point p{0, 0};
+                ft.getQueryEnv().addLocation(GeoLocationSpec{"pos", p});
                 ASSERT_TRUE(ft.setup());
                 ASSERT_TRUE(ft.execute(RankResult().addScore("distance(pos)", 6400000000.0)));
             }
@@ -893,7 +900,8 @@ Test::testDistance()
                 pos->commit();
                 ft.getIndexEnv().getAttributeMap().add(pos);
                 ft.getIndexEnv().getBuilder().addField(FieldType::ATTRIBUTE, CollectionType::WEIGHTEDSET, DataType::INT64, "pos");
-                ft.getQueryEnv().getLocation().setValid(true);
+                GeoLocation::Point p{0, 0};
+                ft.getQueryEnv().addLocation(GeoLocationSpec{"pos", p});
                 ASSERT_TRUE(ft.setup());
                 ASSERT_TRUE(ft.execute(RankResult().addScore("distance(pos)", 6400000000.0)));
             }
@@ -939,10 +947,9 @@ Test::assert2DZDistance(feature_t exp, const vespalib::string & positions,
         pos.emplace_back(x, y);
     }
     setupForDistanceTest(ft, "pos", pos, true);
-    ft.getQueryEnv().getLocation().setXPosition(xquery);
-    ft.getQueryEnv().getLocation().setYPosition(yquery);
-    ft.getQueryEnv().getLocation().setXAspect(xAspect);
-    ft.getQueryEnv().getLocation().setValid(true);
+    GeoLocation::Point p{xquery, yquery};
+    GeoLocation::Aspect aspect{xAspect};
+    ft.getQueryEnv().addLocation(GeoLocationSpec{"pos", {p, aspect}});
     ASSERT_TRUE(ft.setup());
     ASSERT_TRUE(ft.execute(RankResult().setEpsilon(1e-4).
                            addScore("distance(pos)", exp)));
diff --git a/searchlib/src/vespa/searchlib/features/distancefeature.cpp b/searchlib/src/vespa/searchlib/features/distancefeature.cpp
index bd2c7becc81..6582bcae92a 100644
--- a/searchlib/src/vespa/searchlib/features/distancefeature.cpp
+++ b/searchlib/src/vespa/searchlib/features/distancefeature.cpp
@@ -2,7 +2,7 @@
 
 #include "distancefeature.h"
 #include <vespa/searchcommon/common/schema.h>
-#include <vespa/searchlib/fef/location.h>
+#include <vespa/searchlib/common/geo_location_spec.h>
 #include <vespa/searchlib/fef/matchdata.h>
 #include <vespa/document/datatype/positiondatatype.h>
 #include <vespa/vespalib/geo/zcurve.h>
@@ -100,19 +100,10 @@ DistanceExecutor::calculate2DZDistance(uint32_t docId)
     int32_t docy = 0;
     for (auto loc : _locations) {
         assert(loc);
-        assert(loc->isValid());
-        int32_t loc_x = loc->getXPosition();
-        int32_t loc_y = loc->getYPosition();
-        uint64_t loc_a = loc->getXAspect();
-        LOG(debug, "location: x=%u, y=%u, aspect=%zu", loc_x, loc_y, loc_a);
+        assert(loc->location.valid());
         for (uint32_t i = 0; i < numValues; ++i) {
             vespalib::geo::ZCurve::decode(_intBuf[i], &docx, &docy);
-            uint32_t dx = (loc_x > docx) ? (loc_x - docx) : (docx - loc_x);
-            if (loc_a != 0) {
-                dx = (uint64_t(dx) * loc_a) >> 32;
-            }
-            uint32_t dy = (loc_y > docy) ? (loc_y - docy) : (docy - loc_y);
-            uint64_t sqdist = (uint64_t) dx * dx + (uint64_t) dy * dy;
+            uint64_t sqdist = loc->location.sq_distance_to({docx, docy});
             if (sqdist < sqabsdist) {
                 sqabsdist = sqdist;
             }
@@ -121,7 +112,7 @@ DistanceExecutor::calculate2DZDistance(uint32_t docId)
     return static_cast<feature_t>(std::sqrt(static_cast<feature_t>(sqabsdist)));
 }
 
-DistanceExecutor::DistanceExecutor(std::vector<const Location *> locations,
+DistanceExecutor::DistanceExecutor(GeoLocationSpecPtrs locations,
                                    const search::attribute::IAttributeVector * pos) :
     FeatureExecutor(),
     _locations(locations),
@@ -253,17 +244,17 @@ DistanceBlueprint::createExecutor(const IQueryEnvironment &env, vespalib::Stash
     }
     // expect geo pos:
     const search::attribute::IAttributeVector * pos = nullptr;
-    std::vector<const search::fef::Location *> matching_locs;
-    std::vector<const search::fef::Location *> other_locs;
+    GeoLocationSpecPtrs matching_locs;
+    GeoLocationSpecPtrs other_locs;
 
     for (auto loc_ptr : env.getAllLocations()) {
-        if (_use_geo_pos && loc_ptr && loc_ptr->isValid()) {
-            if (loc_ptr->getAttribute() == _arg_string) {
+        if (_use_geo_pos && loc_ptr && loc_ptr->location.valid()) {
+            if (loc_ptr->field_name == _arg_string) {
                 LOG(debug, "found loc from query env matching '%s'", _arg_string.c_str());
                 matching_locs.push_back(loc_ptr);
             } else {
                 LOG(debug, "found loc(%s) from query env not matching arg(%s)",
-                    loc_ptr->getAttribute().c_str(), _arg_string.c_str());
+                    loc_ptr->field_name.c_str(), _arg_string.c_str());
                 other_locs.push_back(loc_ptr);
             }
         }
diff --git a/searchlib/src/vespa/searchlib/features/distancefeature.h b/searchlib/src/vespa/searchlib/features/distancefeature.h
index 024b9f37f31..ece139c6546 100644
--- a/searchlib/src/vespa/searchlib/features/distancefeature.h
+++ b/searchlib/src/vespa/searchlib/features/distancefeature.h
@@ -7,12 +7,15 @@
 
 namespace search::features {
 
+/** Convenience typedef. */
+using GeoLocationSpecPtrs = std::vector<const search::common::GeoLocationSpec *>;
+
 /**
  * Implements the executor for the distance feature.
  */
 class DistanceExecutor : public fef::FeatureExecutor {
 private:
-    std::vector<const fef::Location *>  _locations;
+    GeoLocationSpecPtrs                 _locations;
     const attribute::IAttributeVector * _pos;
     attribute::IntegerContent           _intBuf;
 
@@ -26,7 +29,7 @@ public:
      * @param locations location objects associated with the query environment.
      * @param pos the attribute to use for positions (expects zcurve encoding).
      */
-    DistanceExecutor(std::vector<const fef::Location *> locations,
+    DistanceExecutor(GeoLocationSpecPtrs locations,
                      const attribute::IAttributeVector * pos);
     void execute(uint32_t docId) override;
 
diff --git a/searchlib/src/vespa/searchlib/fef/CMakeLists.txt b/searchlib/src/vespa/searchlib/fef/CMakeLists.txt
index 396775b20c5..178de1b8b87 100644
--- a/searchlib/src/vespa/searchlib/fef/CMakeLists.txt
+++ b/searchlib/src/vespa/searchlib/fef/CMakeLists.txt
@@ -16,7 +16,6 @@ vespa_add_library(searchlib_fef OBJECT
     filetablefactory.cpp
     functiontablefactory.cpp
     indexproperties.cpp
-    location.cpp
     matchdata.cpp
     matchdatalayout.cpp
     objectstore.cpp
diff --git a/searchlib/src/vespa/searchlib/fef/fef.h b/searchlib/src/vespa/searchlib/fef/fef.h
index 7dd24bd17ae..b677ba128c9 100644
--- a/searchlib/src/vespa/searchlib/fef/fef.h
+++ b/searchlib/src/vespa/searchlib/fef/fef.h
@@ -35,7 +35,6 @@
 #include "itablemanager.h"
 #include "itermdata.h"
 #include "itermfielddata.h"
-#include "location.h"
 #include "matchdata.h"
 #include "matchdatalayout.h"
 #include "parameter.h"
diff --git a/searchlib/src/vespa/searchlib/fef/iqueryenvironment.h b/searchlib/src/vespa/searchlib/fef/iqueryenvironment.h
index 811e7fd4616..7c6a84916f4 100644
--- a/searchlib/src/vespa/searchlib/fef/iqueryenvironment.h
+++ b/searchlib/src/vespa/searchlib/fef/iqueryenvironment.h
@@ -6,9 +6,10 @@
 #include "objectstore.h"
 #include <vespa/searchcommon/attribute/iattributecontext.h>
 
+namespace search::common { class GeoLocationSpec; }
+
 namespace search::fef {
 
-class Location;
 class Properties;
 class ITermData;
 
@@ -24,6 +25,9 @@ public:
      **/
     typedef std::shared_ptr<IQueryEnvironment> SP;
 
+    /** Convenience typedef. */
+    using GeoLocationSpecPtrs = std::vector<const search::common::GeoLocationSpec *>;
+
     /**
      * Obtain the set of properties associated with this query
      * environment. This set of properties is known through the system
@@ -60,7 +64,7 @@ public:
      *
      * @return pointers to location objects.
      **/
-    virtual std::vector<const Location *> getAllLocations() const = 0;
+    virtual GeoLocationSpecPtrs getAllLocations() const = 0;
 
     /**
      * Returns the attribute context for this query.
diff --git a/searchlib/src/vespa/searchlib/fef/location.cpp b/searchlib/src/vespa/searchlib/fef/location.cpp
deleted file mode 100644
index 978daa0f930..00000000000
--- a/searchlib/src/vespa/searchlib/fef/location.cpp
+++ /dev/null
@@ -1,18 +0,0 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-
-#include "location.h"
-
-namespace search {
-namespace fef {
-
-Location::Location() :
-    _attr(),
-    _xPos(0),
-    _yPos(0),
-    _xAspect(0),
-    _valid(false)
-{
-}
-
-} // namespace fef
-} // namespace search
diff --git a/searchlib/src/vespa/searchlib/fef/location.h b/searchlib/src/vespa/searchlib/fef/location.h
deleted file mode 100644
index 3bc693e11b4..00000000000
--- a/searchlib/src/vespa/searchlib/fef/location.h
+++ /dev/null
@@ -1,111 +0,0 @@
-// Copyright 2017 Yahoo Holdings. Licensed under the terms of the Apache 2.0 license. See LICENSE in the project root.
-
-#pragma once
-
-#include <vespa/vespalib/stllike/string.h>
-
-namespace search {
-namespace fef {
-
-/**
- * This class contains location data that is associated with a query.
- **/
-class Location
-{
-private:
-    vespalib::string _attr;
-    int32_t     _xPos;
-    int32_t     _yPos;
-    uint32_t    _xAspect;
-    bool        _valid;
-
-public:
-    /**
-     * Creates an empty object.
-     **/
-    Location();
-
-    /**
-     * Sets the name of the attribute to use for x positions.
-     *
-     * @param  xAttr the attribute name.
-     * @return this to allow chaining.
-     **/
-    Location &
-    setAttribute(const vespalib::string & attr)
-    {
-        _attr = attr;
-        return *this;
-    }
-
-    /**
-     * Returns the name of the attribute to use for positions.
-     *
-     * @return the attribute name.
-     **/
-    const vespalib::string & getAttribute() const { return _attr; }
-
-    /**
-     * Sets the x position of this location.
-     *
-     * @param  xPos the x position.
-     * @return this to allow chaining.
-     **/
-    Location & setXPosition(int32_t xPos) { _xPos = xPos; return *this; }
-
-    /**
-     * Returns the x position of this location.
-     *
-     * @return the x position.
-     **/
-    int32_t getXPosition() const { return _xPos; }
-
-    /**
-     * Sets the y position of this location.
-     *
-     * @param yPos the y position.
-     * @return this to allow chaining.
-     **/
-    Location & setYPosition(int32_t yPos) { _yPos = yPos; return *this; }
-
-    /**
-     * Returns the y position of this location.
-     *
-     * @return the y position.
-     **/
-    int32_t getYPosition() const { return _yPos; }
-
-    /**
-     * Sets the x distance multiplier fraction.
-     *
-     * @param xAspect the x aspect.
-     * @return this to allow chaining.
-     **/
-    Location & setXAspect(uint32_t xAspect) { _xAspect = xAspect; return *this; }
-
-    /**
-     * Returns the x distance multiplier fraction.
-     *
-     * @return the x aspect.
-     **/
-    uint32_t getXAspect() const { return _xAspect; }
-
-    /**
-     * Sets whether this is a valid location object.
-     *
-     * @param  valid true if this is valid.
-     * @return this to allow chaining.
-     **/
-    Location & setValid(bool valid) { _valid = valid; return *this; }
-
-    /**
-     * Returns whether this is a valid location object.
-     *
-     * @param true if this is a valid.
-     **/
-    bool isValid() const { return _valid; }
-};
-
-} // namespace fef
-} // namespace search
-
diff --git a/searchlib/src/vespa/searchlib/fef/phrase_splitter_query_env.h b/searchlib/src/vespa/searchlib/fef/phrase_splitter_query_env.h
index 3a8cde99c06..90bf4e8955f 100644
--- a/searchlib/src/vespa/searchlib/fef/phrase_splitter_query_env.h
+++ b/searchlib/src/vespa/searchlib/fef/phrase_splitter_query_env.h
@@ -73,7 +73,7 @@ public:
     }
 
     const Properties & getProperties() const override { return _queryEnv.getProperties(); }
-    std::vector<const Location *> getAllLocations() const override {
+    GeoLocationSpecPtrs getAllLocations() const override {
         return _queryEnv.getAllLocations();
     }
     const attribute::IAttributeContext & getAttributeContext() const override { return _queryEnv.getAttributeContext(); }
diff --git a/searchlib/src/vespa/searchlib/fef/test/queryenvironment.cpp b/searchlib/src/vespa/searchlib/fef/test/queryenvironment.cpp
index 4697675c071..d602e74ddfb 100644
--- a/searchlib/src/vespa/searchlib/fef/test/queryenvironment.cpp
+++ b/searchlib/src/vespa/searchlib/fef/test/queryenvironment.cpp
@@ -8,7 +8,7 @@ QueryEnvironment::QueryEnvironment(IndexEnvironment *env)
     : _indexEnv(env),
       _terms(),
       _properties(),
-      _location(),
+      _locations(),
       _attrCtx((env == nullptr) ? attribute::IAttributeContext::UP() : env->getAttributeMap().createContext())
 {
 }
diff --git a/searchlib/src/vespa/searchlib/fef/test/queryenvironment.h b/searchlib/src/vespa/searchlib/fef/test/queryenvironment.h
index 4b9bec1ee68..d8d5c802360 100644
--- a/searchlib/src/vespa/searchlib/fef/test/queryenvironment.h
+++ b/searchlib/src/vespa/searchlib/fef/test/queryenvironment.h
@@ -4,12 +4,14 @@
 #include "indexenvironment.h"
 #include <vespa/searchcommon/attribute/iattributecontext.h>
 #include <vespa/searchlib/fef/iqueryenvironment.h>
-#include <vespa/searchlib/fef/location.h>
 #include <vespa/searchlib/fef/simpletermdata.h>
+#include <vespa/searchlib/common/geo_location_spec.h>
 #include <unordered_map>
 
 namespace search::fef::test {
 
+using search::common::GeoLocationSpec;
+
 /**
  * Implementation of the IQueryEnvironment interface used for testing.
  */
@@ -22,7 +24,7 @@ private:
     IndexEnvironment           *_indexEnv;
     std::vector<SimpleTermData> _terms;
     Properties                  _properties;
-    Location                    _location;
+    std::vector<GeoLocationSpec> _locations;
     search::attribute::IAttributeContext::UP _attrCtx;
     std::unordered_map<std::string, double> _avg_field_lengths;
 
@@ -38,10 +40,12 @@ public:
     const Properties &getProperties() const override { return _properties; }
     uint32_t getNumTerms() const override { return _terms.size(); }
     const ITermData *getTerm(uint32_t idx) const override { return idx < _terms.size() ? &_terms[idx] : NULL; }
-    std::vector<const Location *> getAllLocations() const override {
-        std::vector<const Location *> retval;
-        retval.push_back(&_location);
-        return retval;
+    GeoLocationSpecPtrs getAllLocations() const override {
+        GeoLocationSpecPtrs locations;
+        for (const auto & loc : _locations) {
+            locations.push_back(&loc);
+        }
+        return locations;
     }
     const search::attribute::IAttributeContext &getAttributeContext() const override { return *_attrCtx; }
     double get_average_field_length(const vespalib::string& field_name) const override {
@@ -86,7 +90,7 @@ public:
     Properties & getProperties() { return _properties; }
 
     /** Returns a reference to the location of this. */
-    Location & getLocation() { return _location; }
+    void addLocation(const GeoLocationSpec &spec) { _locations.push_back(spec); }
 
     std::unordered_map<std::string, double>& get_avg_field_lengths() { return _avg_field_lengths; }
 };