gsmiller commented on a change in pull request #751:
URL: https://github.com/apache/lucene/pull/751#discussion_r835436710
##########
File path:
lucene/facet/src/java/org/apache/lucene/facet/LongValueFacetCounts.java
##########
@@ -348,6 +348,9 @@ private void increment(long value) {
@Override
public FacetResult getTopChildren(int topN, String dim, String... path) {
+ if (topN <= 0) {
Review comment:
It also occurs to me that this is somewhat cumbersome to require every
sub-class to do this validation if they want to. I don't think we have to solve
it here, but could you open a spin-off issue to capture the idea of potentially
moving this validation into a common parent class so each concrete
implementation doesn't need to do this check?
Along those lines, if we do introduce a `protected` method here, can you
please mark it `@lucene.experimental` with a note that it may not exist in
future versions of Lucene? I'm not sure we actually want to support this as
part of our API surface long-term. Once we add this, it's possible that
user-implemented sub-classes could start relying on it, and I'd like to be able
to remove it easily without jumping through back-compat hoops.
--
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
