apache / lucene

Apache Lucene open-source search software
https://lucene.apache.org/
Apache License 2.0
2.45k stars 975 forks source link

Make Weight#scorerSupplier abstract, Weight#scorer final #13319

Closed iamsanjay closed 1 month ago

iamsanjay commented 2 months ago

Description

13180

This PR would declare the scoreSupplier method abstract and marked scorer final. The nature of the change itself is not complex, however the large number of files has been modified with this change.

Few Observations:

  1. As this change would enforce subclasses of Weight to implement scorerSupplier, the default cost calculation method is used where it was missing
@Override
public long cost() {
    return scorer.iterator().cost();
}
  1. scorer method is removed from SpanQuery.java and therefore all the classes has to implement the scorerSupplier, We can also move the socrerSupplier method to SpanQuery that way the subclasses does not has to provide the implementation, which is same for all subclasses.
  2. scorer method removed from AssertingWeight.
  3. What would be the implementation for cost where calling scorer throws UnsupportedOpeartionException. Does cost method also throws the same exception, currently it's returning 0. Below are the classes where you will find such code.
lucene/core/src/test/org/apache/lucene/search/TestBooleanScorer.java is also such example.
lucene/core/src/test/org/apache/lucene/search/TestConjunctionDISI.java
lucene/core/src/test/org/apache/lucene/search/TestDisiPriorityQueue.java
lucene/core/src/test/org/apache/lucene/search/TestDisjunctionScoreBlockBoundaryPropagator.java
lucene/core/src/test/org/apache/lucene/search/TestMaxScoreBulkScorer.java
lucene/facet/src/java/org/apache/lucene/facet/DrillSidewaysQuery.java
lucene/suggest/src/java/org/apache/lucene/search/suggest/document/CompletionWeight.java
iamsanjay commented 2 months ago

scorerSupplier has to be null if scorer is null. Therefore scorer was initialized inside the scorerSupplier, instead of scorerSupplier.get method. And if scorer evaluates to null then scorerSupplier would return null.

iamsanjay commented 1 month ago

Instead of both get() and cost() throwing the exception, Can make scorerSupplier throw UnsupportedOperationException()?

@Override
public ScorerSupplier scorerSupplier(LeafReaderContext context) throws IOException {
     throw new UnsupportedOperationException();
}
jpountz commented 1 month ago

Instead of both get() and cost() throwing the exception, Can make scorerSupplier throw UnsupportedOperationException()?

In general yes, there may be a few exceptions though like JustCompileSearch where we want to keep these methods since it's about detecting API changes.

iamsanjay commented 1 month ago

In general yes, there may be a few exceptions though like JustCompileSearch where we want to keep these methods since it's about detecting API changes.

Okay in that case, let's make cost method throw an exception.

iamsanjay commented 1 month ago

I also changed scorerSupplier in few classes to delegate it to scorerSupplier instead of scorer.

On a side note, I faced an issue where it got stuck in an infinite loop. The reason because scorerSupplier was calling super.scorer() and then scorer was delegating it to scorerSupplier. I wonder If it's a design issue? And, How can we change the existing design So that It won't happen?