Add decay function support for MultiFunctionScoreQuery #641

swethakann · 2024-03-28T00:18:53Z

No description provided.

swethakann · 2024-03-28T00:21:43Z

...lp/nrtsearch/server/luceneserver/search/query/multifunction/GeoPointDecayFilterFunction.java

+          GeoUtils.arcDistance(
+              origin.getLatitude(), origin.getLongitude(), geoPoint.getLat(), geoPoint.getLon());
+      double score = decayFunc.computeScore(distance, scale);
+      return Math.max(0.0, score - offset);


I am not quite sure whether innerQueryScore and getWeight need to be combined with this score here.

I don't think decay functions need the query score, but the result would need to be multiplied by the function weight.

aprudhomme · 2024-03-29T17:58:18Z

clientlib/src/main/proto/yelp/nrtsearch/search.proto

@@ -316,8 +316,22 @@ message MultiFunctionScoreQuery {
            // Produce score with score script definition
            Script script = 3;
        }
+        DecayFunction decayFunction = 4;


Is there a reason this is not part of the Function block?

aprudhomme · 2024-03-29T17:59:21Z

clientlib/src/main/proto/yelp/nrtsearch/search.proto

    }

+    message DecayFunction {
+        string fieldName = 1;
+        string decayType = 2;


If there is only a known set of decay types, it might be better as an enum.

aprudhomme · 2024-03-29T18:25:46Z

...lp/nrtsearch/server/luceneserver/search/query/multifunction/GeoPointDecayFilterFunction.java

+          GeoUtils.arcDistance(
+              origin.getLatitude(), origin.getLongitude(), geoPoint.getLat(), geoPoint.getLon());
+      double score = decayFunc.computeScore(distance, scale);
+      return Math.max(0.0, score - offset);


I don't think decay functions need the query score, but the result would need to be multiplied by the function weight.

aprudhomme · 2024-03-29T18:35:39Z

...lp/nrtsearch/server/luceneserver/search/query/multifunction/GeoPointDecayFilterFunction.java

+      double score = decayFunc.computeScore(distance, scale);
+      return Math.max(0.0, score - offset);


I'm not sure this math is right. These are the function definitions https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-function-score-query.html#_supported_decay_functions

aprudhomme · 2024-03-29T18:39:18Z

...lp/nrtsearch/server/luceneserver/search/query/multifunction/GeoPointDecayFilterFunction.java

+      Query filterQuery, float weight, MultiFunctionScoreQuery.DecayFunction decayFunction) {
+    super(filterQuery, weight, decayFunction);
+    this.decayFunction = decayFunction;
+    this.fieldName = decayFunction.getFieldName();


If you also pass in the IndexState you get get the FieldDef for validation. The FieldDef can also be used to get the doc values accessor.

aprudhomme · 2024-04-02T03:03:18Z

clientlib/src/main/proto/yelp/nrtsearch/search.proto

+        // Distance from origin + offset at which computed score will be equal to decay
+        string scale = 5;
+        // Compute decay function for docs with a distance greater than offset, will be 0.0 if none is set
+        string offset = 6;


Can the field comments also define the valid values for scale and offset, including what distance specifiers are allowed

aprudhomme · 2024-04-02T03:04:24Z

clientlib/src/main/proto/yelp/nrtsearch/search.proto

+        // Linear decay function
+        DECAY_TYPE_LINEAR = 1;
+        // Gaussian decay function
+        DECAY_TYPE_GUASS = 2;


nit: if exponential is spelled out, gaussian probably can be too

aprudhomme · 2024-04-02T03:07:25Z

...a/com/yelp/nrtsearch/server/luceneserver/search/query/multifunction/DecayFilterFunction.java

+      case DECAY_TYPE_LINEAR:
+        return new LinearDecayFunction();
+      default:
+        return null;


The default can probably throw an illegal argument exception

aprudhomme · 2024-04-02T03:12:11Z

...n/java/com/yelp/nrtsearch/server/luceneserver/search/query/multifunction/FilterFunction.java

@@ -77,6 +77,12 @@ public static FilterFunction build(
            ? QueryNodeMapper.getInstance().getQuery(filterFunctionGrpc.getFilter(), indexState)
            : null;
    float weight = filterFunctionGrpc.getWeight() != 0.0f ? filterFunctionGrpc.getWeight() : 1.0f;
+    if (filterFunctionGrpc.hasDecayFunction()) {


This can be integrated with the switch statement below

aprudhomme · 2024-04-02T03:13:36Z

...n/java/com/yelp/nrtsearch/server/luceneserver/search/query/multifunction/FilterFunction.java

+      if (decayFunction.hasGeoPoint()) {
+        return new GeoPointDecayFilterFunction(filterQuery, weight, decayFunction, indexState);
+      }


If geo point is the only mode supported, an exception should probably be throw if it is not set

aprudhomme · 2024-04-02T03:30:49Z

...lp/nrtsearch/server/luceneserver/search/query/multifunction/GeoPointDecayFilterFunction.java

+      segmentDocLookup.setDocId(docId);
+      LoadedDocValues<GeoPoint> latLonValues =
+          (LoadedDocValues<GeoPoint>) segmentDocLookup.get(fieldName);
+      this.latLng = latLonValues.toFieldValue(0).getLatLngValue();


You should be able to get the lat/lon values directly from the GeoPoint without using toFieldValue

aprudhomme · 2024-04-02T03:32:14Z

...lp/nrtsearch/server/luceneserver/search/query/multifunction/GeoPointDecayFilterFunction.java

+              latLng.getLatitude(),
+              latLng.getLongitude());


You will need to recompute the position for the given doc id

aprudhomme · 2024-04-02T03:36:03Z

clientlib/src/main/proto/yelp/nrtsearch/search.proto

+        DecayType decayType = 2;
+        // Origin point to calculate the distance
+        oneof Origin {
+            int32 numeric = 3;


If numeric is not supported yet, does it need to be here?

I will remove it. Thought we can add support for it too but noticed that the client does not need this immediately

aprudhomme · 2024-04-02T03:37:57Z

...in/java/com/yelp/nrtsearch/server/luceneserver/search/query/multifunction/DecayFunction.java

+public interface DecayFunction {
+  double computeScore(double distance, double offset, double scale);
+
+  double computeScale(double scale, double decay);
+
+  Explanation explainComputeScore(double distance, double offset, double scale);
+}


It would be good to add doc strings to this interface

aprudhomme · 2024-04-02T03:39:18Z

...lp/nrtsearch/server/luceneserver/search/query/multifunction/GeoPointDecayFilterFunction.java

+      segmentDocLookup.setDocId(docId);
+      LoadedDocValues<GeoPoint> latLonValues =
+          (LoadedDocValues<GeoPoint>) segmentDocLookup.get(fieldName);
+      this.latLng = latLonValues.toFieldValue(0).getLatLngValue();


What is the behavior if the field does not have any values present?

aprudhomme · 2024-04-03T18:57:13Z

...com/yelp/nrtsearch/server/luceneserver/search/query/multifunction/GuassianDecayFunction.java

+public class GuassianDecayFunction implements DecayFunction {
+  @Override
+  public double computeScore(double distance, double offset, double scale) {
+    return Math.exp((-1.0 * Math.pow(Math.max(0.0, distance - offset), 2.0)) / 2.0 * scale);


I think the denominator needs to be (2.0 * scale)

aprudhomme · 2024-04-03T18:58:57Z

...lp/nrtsearch/server/luceneserver/search/query/multifunction/GeoPointDecayFilterFunction.java

+  }
+
+  public void validateLatLonField(FieldDef fieldDef) {
+    if (!LAT_LON.equals(fieldDef.getType())) {


A better check would be using the instanceof operator, since the object is getting cast

aprudhomme · 2024-04-03T19:04:48Z

...lp/nrtsearch/server/luceneserver/search/query/multifunction/GeoPointDecayFilterFunction.java

+    public boolean validateDocValuesPresent() {
+      return segmentDocLookup.containsKey(fieldName) && !segmentDocLookup.get(fieldName).isEmpty();
+    }


You shouldn't need to do containsKey, since you already checked that doc values are enabled. You will also want to avoid multiple calls to segmentDocLookup.get(), since it will reload data from the index. Something like this should work

LoadedDocValues<GeoPoint> geoPointLoadedDocValues = (LoadedDocValues<GeoPoint>) segmentDocLookup.get(fieldName); if (geoPointLoadedDocValues.isEmpty()) { return 0.0; } else { ... }

Add decay function support for MultiFunctionScoreQuery

74f238c

swethakann commented Mar 28, 2024

View reviewed changes

swethakann requested a review from aprudhomme March 28, 2024 00:22

aprudhomme reviewed Mar 29, 2024

View reviewed changes

swethakann force-pushed the RP-10216_support_decay_function branch 2 times, most recently from 25516c6 to 719731c Compare April 1, 2024 20:13

PR changes and more tests

d59c35b

swethakann force-pushed the RP-10216_support_decay_function branch from 719731c to d59c35b Compare April 1, 2024 20:20

swethakann marked this pull request as ready for review April 1, 2024 20:33

swethakann requested a review from aprudhomme April 1, 2024 20:34

swethakann assigned aprudhomme Apr 1, 2024

aprudhomme reviewed Apr 2, 2024

View reviewed changes

swethakann force-pushed the RP-10216_support_decay_function branch from 96ce3d4 to 33fd26b Compare April 2, 2024 23:31

PR changes and some refactoring

92b8e79

swethakann force-pushed the RP-10216_support_decay_function branch from 33fd26b to 92b8e79 Compare April 2, 2024 23:32

swethakann requested a review from aprudhomme April 2, 2024 23:34

aprudhomme reviewed Apr 3, 2024

View reviewed changes

PR changes

0cd8290

swethakann requested a review from aprudhomme April 3, 2024 20:41

aprudhomme approved these changes Apr 3, 2024

View reviewed changes

swethakann merged commit 12d836e into master Apr 3, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add decay function support for MultiFunctionScoreQuery #641

Add decay function support for MultiFunctionScoreQuery #641

swethakann commented Mar 28, 2024

swethakann Mar 28, 2024

aprudhomme Mar 29, 2024

aprudhomme Mar 29, 2024

aprudhomme Mar 29, 2024

aprudhomme Mar 29, 2024

aprudhomme Mar 29, 2024

aprudhomme Mar 29, 2024

aprudhomme Apr 2, 2024

aprudhomme Apr 2, 2024

aprudhomme Apr 2, 2024

aprudhomme Apr 2, 2024

aprudhomme Apr 2, 2024

aprudhomme Apr 2, 2024

aprudhomme Apr 2, 2024

aprudhomme Apr 2, 2024

swethakann Apr 2, 2024

aprudhomme Apr 2, 2024

aprudhomme Apr 2, 2024

aprudhomme Apr 3, 2024

aprudhomme Apr 3, 2024

aprudhomme Apr 3, 2024

		double score = decayFunc.computeScore(distance, scale);
		return Math.max(0.0, score - offset);

Add decay function support for MultiFunctionScoreQuery #641

Add decay function support for MultiFunctionScoreQuery #641

Conversation

swethakann commented Mar 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment