Do not expose full-fledged scorers in LeafCollector.setScorer [LUCENE-6228]

[asfimport]

Currently LeafCollector.setScorer takes a Scorer, which I don't like because several methods should never be called in the context of a Collector (like nextDoc or advance).

I think it's even more trappy for methods that might seem to work in some particular cases but will not work in the general case, like getChildren which will not work if you have a specialized BulkScorer or iterating over positions which will not work if you are in a MultiCollector and another leaf collector consumes positions too.

So I think we should restrict what can be seen from a collector to avoid such traps.

---

Migrated from LUCENE-6228 by Adrien Grand (@jpountz), resolved Sep 04 2018
Attachments: LUCENE-6228.patch (versions: 6)
Linked issues:

• Remove ToParentBlockJoinCollector [LUCENE-6959] #8016

[asfimport]

Adrien Grand (@jpountz) (migrated from JIRA)

Simple patch that adds a new Score interface with only two methods freq() and score(). This interface is extended by Scorer and FakeScore (previously FakeScorer) and is exposed in collectors instead of Scorer. I think it's better?

[asfimport]

Ryan Ernst (@rjernst) (migrated from JIRA)

Having both Score and Scorer is really confusing...can we have the interface still be Scorer and the abstract class be something else...maybe ScoringEnum?

[asfimport]

Adrien Grand (@jpountz) (migrated from JIRA)

This is a great suggestion as it makes collectors easier to migrate and is also consistent with the fact that Scorer(ScoringEnum in the patch) extends PostingsEnum. Here is a new patch.

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

I like this, particularly if I'm about to start adding more position-aware methods to ScoringEnum.

I think FilterScorer should be FilterScoringEnum now? Similarly with AssertingScorer.

The javadocs on LeafCollector should still refer to Scorer, not ScoringEnum

Can we get rid of the FakeScorers in the taxonomy and expression modules, and in the index Sorter now?

[asfimport]

Adrien Grand (@jpountz) (migrated from JIRA)

Thanks Alan, I applied your suggestions: here is a new patch.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

another maybe more intuitive option is Scorable (interface) and keep Scorer as is, just implementing the interface.

[asfimport]

Ryan Ernst (@rjernst) (migrated from JIRA)

another maybe more intuitive option is Scorable (interface) and keep Scorer as is, just implementing the interface.

I like that idea also. I was only trying to suggest something that would have some larger difference in name between the two.

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

Here's a patch implementing @rmuir's idea of a Scorable interface. This seems like the least intrusive way of doing this?

As this changes LeafCollector, which is a fairly heavily-used part of the API, I think this should only apply to master

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

The casts back to Scorer in some of the tests are concerning, because if code just does this then i'm not certain how much this abstraction is helping.

Can we try something like moving getChildren from Scorer to this interface (it can still have default empty implementation with a default method)? Then maybe the casts would be removed but also ChildScorer would be restricted (like mentioned in the description of the issue) from doing illegal stuff.

We should also check performance, its a bit scary to add interfaces here (maybe an abstract class is an alternative) and we should ensure that this patch isn't causing hotspot to go crazy because of FakeScorer and Scorer impls.

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

Can we try something like moving getChildren from Scorer to this interface

This just moves the existing problem with getChildren() to the new interface though? I think I'd rather leave it on Scorer and make it accessible through IndexSearcher or something similar in a follow-up issue. I can change the tests in question to not use Collectors.

We should also check performance

Will run it through luceneutil and see what happens

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

This just moves the existing problem with getChildren() to the new interface though? I think I'd rather leave it on Scorer and make it accessible through IndexSearcher or something similar in a follow-up issue. I can change the tests in question to not use Collectors.

I don't think it does, i think it fixes the API? Instead of the following on Scorer:

  Collection<ChildScorer> getChildren();
  class ChildScorer {
    Scorer child;
    relationship;
 }

we should have on Scorable:

  Collection<ChildScorer> getChildren();
  class ChildScorer {
    Scorable child;
    relationship;
 }

This means that getChildren becomes "safe" and you can't call methods on Scorer that you shouldnt be invoking. As far as I understand, its part of the motivation behind this whole issue.

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

The issue with getChildren() is that you can't access it if a BulkScorer is being used - it's a separate problem to this issue, which is preventing people calling iterator() from a Collector. Or rather, it's the same sort of problem, in that we should prevent people from calling getChildren() from a Collector, because they might be holding a FakeScorer which can't support it.

[asfimport]

Robert Muir (@rmuir) (migrated from JIRA)

I don't agree, sorry. We don't need to prevent anyone from calling getChildren() on a FakeScorer, it can just return empty which is the default implementation.

What we need to prevent is someone calling advance() based on the results of getChildren, that is key: and honestly if we can't prevent that with this issue, then the abstraction isn't right, its the whole point of doing this :)

Stuff like what getChildren() on a FakeScorer or BulkScorer does, that stuff is harmless, and those things are the separate issues that we shouldnt worry about.

[asfimport]

Alan Woodward (@romseygeek) (migrated from JIRA)

What we need to prevent is someone calling advance() based on the results of getChildren

But the current patch does that by just not having getChildren() on Scorable. And just as you can get round that by casting, you could cast ChildScorer.child to a Scorer as well if you wanted to.

I guess the disagreement is over what the getChildren() API is actually for. The tests are using it to check that constant score wrappers and the like are still giving us the correct root scorer, so I think casting is OK there?

Another idea I had was to replace getChildren() entirely with a visitor API, and have a method like IndexSearcher.explain(). So you call searcher.visitScorers(Query q, int doc, ScorerVisitor visitor) and it will then visit all the scorers that are positioned on that doc. Maybe I should open another issue to deal with that first?

[asfimport]

Adrien Grand (@jpountz) (migrated from JIRA)

I think this Scorable interface (or base class) is already great progress as it hides advance and other methods that collectors should never call. I tend to agree with Alan that Scorable should not have getChildren either but since this proves controversial, maybe we can keep it on Scorable for now and discuss it in a separate issue?

Another idea I had was to replace getChildren() entirely with a visitor API

For the record, this used to be a visitor API before, see #4403. As far as I'm concerned, I tend to like exposing well-known java collections better than a custom visitor API. Visitors also introduce some challenges in that case due to the tree nature of scorers. For instance should it only expose leaves or internal nodes as well? And what relationship should it assign to a Scorer that is a SHOULD clause of a BooleanQuery which is itself a MUST clause of another BooleanQuery?