epotyom commented on issue #13671: URL: https://github.com/apache/lucene/issues/13671#issuecomment-2307498253
The main reason for adding `CollectorOwner` class was to allow different drill-sideways dimensions to use `CollectorManager`s with different Collector `C` and result `T` types. Except for rolling back IndexSearcher and DrillSideways changes, I see three paths forward for this issue: **1st option**: Use the same `C` and `T` types for all drill-sideways dimensions. It's likely that most users don't need different types for dimensions. And the ones that do need it can use `MultiCollectorManager` and cast `Object` result to the right type. See also this comment https://github.com/apache/lucene/pull/13568#issuecomment-2239124290 and response https://github.com/apache/lucene/pull/13568#issuecomment-2239235073 Pros: we can rollback `IndexSearcher` changes that add new public `search` method. **2nd option** is to change `CollectorManager` to keep the list of `Collector`s. This requires making converting it from interface to class, or maintain the list in every interface implementation. I believe both solutions are not ideal, they are not backwards-compatible and not convenient for users. E.g. if we convert it to class, one won't be able to create a CollectorManager that also extends some other class. Pros: allow different `C` and `T` types per drill-sideways dimensions without casting and without separate CollectorOwner class. **3rd option**: Make CollectorOwner less of a separate entity. E.g. we can make it nested static class inside `CollectorManager` interface, and add default method to the interface to wrap. Something like: ``` public interface CollectorManager<C extends Collector, T> { // existing methods C newCollector() throws IOException; T reduce(Collection<C> collectors) throws IOException; // new method to wrap this CollectorManager default Container<C, T> getContainer() { return new Container<>(this); } final static class Container<C, T> { // all CollectorOwner methods } } ``` Pros: allow different `C` and `T` types per drill-sideways dimension without casting; both IndexSearcher and DrillSideways(Query) don't have to manage lists of `Collector`s themselves. I personally prefer 3rd option, but I suppose 1st option is the least controversial? Links to related conversations in the original PR: - https://github.com/apache/lucene/pull/13568#discussion_r1675802255 - https://github.com/apache/lucene/pull/13568#discussion_r1681255620 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org For additional commands, e-mail: issues-h...@lucene.apache.org
