bjacobowitz commented on PR #13109: URL: https://github.com/apache/lucene/pull/13109#issuecomment-2017958689
> The `protected` visibility on `matchQuery()` should already be fine here - you can override or call protected methods from within subclasses. I think making `reportError()` and `finish()` `protected final` would be the neatest solution. @romseygeek On further reflection, I think protected visibility may not be sufficient for a practical solution here, even when using subclasses. I'll again use `ParallelMatcher` as an example, since I think it's a fairly representative use case. `ParallelMatcher` [takes in a `MatcherFactory`](https://github.com/apache/lucene/blob/75e1ebc4501733a6a9857858b3fd9c30304234d0/lucene/monitor/src/java/org/apache/lucene/monitor/ParallelMatcher.java#L66) to [create instances of `CandidateMatcher`](https://github.com/apache/lucene/blob/75e1ebc4501733a6a9857858b3fd9c30304234d0/lucene/monitor/src/java/org/apache/lucene/monitor/ParallelMatcher.java#L120), with the intent to [take advantage of the `matchQuery` implementation in the factory's generated instances of `CandidateMatcher`](https://github.com/apache/lucene/blob/75e1ebc4501733a6a9857858b3fd9c30304234d0/lucene/monitor/src/java/org/apache/lucene/monitor/ParallelMatcher.java#L129). If we try to reimplement `ParallelMatcher` outside the Lucene Monitor package, we can't call `matchQuery` on the factory-built `CandidateMatcher` from outside (i.e. on an instance other than `this`) because of `matchQuery`'s protected access -- the only reason the existing `ParallelMatcher` implementati on can do this is because it is in the Lucene Monitor and therefore has package-local access to the protected functions on another object in the same package. To access and invoke the implementation of matchQuery, we would need to create a subclass of the concrete `CandidateMatcher` implementation generated by the factory, which isn't directly possible, because the factory pattern creates a situation where the concrete class isn't known. We could approach a solution using an alternate factory implementation that would return a subclass of a known concrete type with hidden functions exposed. For example, we could create a subclass of `CollectingMatcher` (let's call it `ExposedCollectingMatcher`) with a public function `exposedMatchQuery` which calls `this.matchQuery()`, and then we could create a factory to return that subclass (it might be called `ExposedCollectingMatcherFactory`). The problem with that approach is that it breaks down in cases where the concrete implementation is not available to be subclassed, as is the case for several anonymous `CandidateMatcher` implementations (e.g. instances generated by [`MATCHER` factory in `ExplainingMatch`](https://github.com/apache/lucene/blob/75e1ebc4501733a6a9857858b3fd9c30304234d0/lucene/monitor/src/java/org/apache/lucene/monitor/ExplainingMatch.java#L32-L54)). Ideally we would like a factory that returns a more generic class that extends the functionality of a `CandidateMatcher`, which is what `WrappedCandidateMatcher` provides. Of course another solution would be to simply make these functions public, but I understand the desire to avoid that if possible. -- 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
