Should the static search methods in FacetsCollector take a FacetsCollector as last argument?

[javanna]

I have bumped into this and wondered for a while, why do the following methods take a generic Collector as last argument?

public static TopDocs search(IndexSearcher searcher, Query q, int n, Collector fc)

public static TopFieldDocs search(IndexSearcher searcher, Query q, int n, Sort sort, Collector fc)

public static TopFieldDocs search(IndexSearcher searcher, Query q, int n, Sort sort, boolean doDocScores, Collector fc)

public static TopDocs searchAfter(IndexSearcher searcher, ScoreDoc after, Query q, int n, Collector fc)

public static TopDocs searchAfter(IndexSearcher searcher, ScoreDoc after, Query q, int n, Sort sort, Collector fc)

public static TopDocs searchAfter(IndexSearcher searcher, ScoreDoc after, Query q, int n, Sort sort, boolean doDocScores, Collector fc)

Are these methods ever called providing a collector that's not an instance of FacetsCollector? If so, should these methods to be moved to a more appropriate location, outside of FacetsCollector? If not, should that argument become FacetsCollector?

This is somewhat related to #12892 in that if we want to remove search(Query, Collector) we need to get rid of the usages in these static methods. We can either change the signature of these methods to take a generic CollectorManager, or a FacetsCollectorManager if we decide that it's more appropriate to restrict the type of accepted collectors.

[gsmiller]

My vote would be to be more restrictive with these signatures and specify FacetsCollectorManager given that these are sugar methods meant to make it easier to do faceting while also the "main" search. Today's faceting module all works against FacetsCollector explicitly in the various APIs, so having a generic Collector or CollectorManager here seems confusing IMO.

[javanna]

I agree @gsmiller , I am inclined to be stricter. Also, while we make this breaking change, we can go directly from Collector to FacetsCollectorManager. I am also considering moving these static methods to FacetsCollectorManager. It feels weird to have them exposed to FacetsCollector but require the corresponding manager.