bjacobowitz commented on PR #13109: URL: https://github.com/apache/lucene/pull/13109#issuecomment-2248546608
@romseygeek I'm wondering if maybe we should make those functions `protected final` as you suggest, but also make some of the `CandidateMatcher` implementations public. Right now, outside of the `org.apache.lucene.monitor` package, there is no basic concrete implementation of CandidateMatcher that is publicly availble to be subclassed. Today we have the following CandidateMatcher implementations, and none of the basic ones are public: - [output of factory ExplainingMatch::MATCHER](https://github.com/apache/lucene/blob/acbd7141404d503e7ca89bec520a175482d41bce/lucene/monitor/src/java/org/apache/lucene/monitor/ExplainingMatch.java#L30-L54) (anonymous `CandidateMatcher`, can't be subclassed) - [output of factory HighlightsMatch::MATCHER](https://github.com/apache/lucene/blob/acbd7141404d503e7ca89bec520a175482d41bce/lucene/monitor/src/java/org/apache/lucene/monitor/HighlightsMatch.java#L44-L96) (anonymous `CandidateMatcher`, can't be subclassed) - [output of factory QueryMatch::SIMPLE_MATCHER](https://github.com/apache/lucene/blob/acbd7141404d503e7ca89bec520a175482d41bce/lucene/monitor/src/java/org/apache/lucene/monitor/QueryMatch.java#L37-L49) (anonymous `CollectingMatcher`, can't be subclassed) - [output of factory ScoringMatch::matchWithSimilarity](https://github.com/apache/lucene/blob/acbd7141404d503e7ca89bec520a175482d41bce/lucene/monitor/src/java/org/apache/lucene/monitor/ScoringMatch.java#L29-L50) (anonymous `CollectingMatcher`, can't be subclassed) - [output of factory QueryTimeListener::timingMatcher](https://github.com/apache/lucene/blob/acbd7141404d503e7ca89bec520a175482d41bce/lucene/monitor/src/java/org/apache/lucene/monitor/QueryTimeListener.java#L36-L61) (anonymous `CandidateMatcher`, can't be subclassed, plus the factory is package-private) - [CollectingMatcher](https://github.com/apache/lucene/blob/acbd7141404d503e7ca89bec520a175482d41bce/lucene/monitor/src/java/org/apache/lucene/monitor/CollectingMatcher.java#L28) (package-private and abstract, not available to be subclassed) - [ParallelMatcher](https://github.com/apache/lucene/blob/acbd7141404d503e7ca89bec520a175482d41bce/lucene/monitor/src/java/org/apache/lucene/monitor/ParallelMatcher.java#L47) (public but not basic, already composed of other `CandidateMatcher` instances via factory) - [PartitionMatcher](https://github.com/apache/lucene/blob/acbd7141404d503e7ca89bec520a175482d41bce/lucene/monitor/src/java/org/apache/lucene/monitor/PartitionMatcher.java#L44) (public but not basic, but already composed of other `CandidateMatcher` instances via factory) If we're thinking about making `CandidateMatcher` functions protected to give subclasses have access to them, I think we would need to make some of the basic implementations of `CandidateMatcher` public, so that they're available to be extended outside the `org.apache.lucene.monitor` package. Again I'll use `ParallelMatcher` as an example. First we [make the `reportError` and `finish` functions `protected final` in `CandidateMatcher`](https://gist.github.com/bjacobowitz/28f6b0b62b3a8828f54ec1d544563cfd#file-candidatematcher-java) as you suggested. If we then [expose a basic implementation of CandidateMatcher](https://gist.github.com/bjacobowitz/28f6b0b62b3a8828f54ec1d544563cfd#file-simplecandidatematcher-java) (I've extracted the CandidateMatcher returned by QueryMatch.SIMPLE_MATCHER, calling it SimpleCandidateMatcher), then outside the package we can [create a subclass of that](https://gist.github.com/bjacobowitz/28f6b0b62b3a8828f54ec1d544563cfd#file-candidatematcherextension-java) (I'm calling it CandidateMatcherExtension here) with public functions that call the now-protected functions of the superclass. That gives our `ParallelMatcher` implementation the ability to [make full use of the basic candidate matcher through the subclass](https://gist.github.com/bjacobowi tz/28f6b0b62b3a8828f54ec1d544563cfd#file-parallelmatcher-java) (see the comments labeled "SOLUTION"). I think this would solve this issue without requiring us to dramatically change function permissions. The only additional requirement would be to expose some implementing CandidateMatcher classes, but I think that might be okay (we've already exposed ParallelMatcher and PartitionMatcher). Should I go ahead with this approach? -- 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
