gsmiller commented on PR #841: URL: https://github.com/apache/lucene/pull/841#issuecomment-1159091616
@shaie > I struggled back-and-forth between introducing a public final long[] comparableLongs on the abstract FacetSet to the getComparableLongs() method. +1 to the approach you went with. We can always change it, but I like how you have it personally. @mdmarshmallow > I think we should also have some subpackages [...] I generally disagree with this. I _used_ to like breaking down functional areas into packages for organization, but it limits your ability to make classes/methods pkg-visible in order to expose a clean API. I now greatly prefer flatter packages with very limited APIs exposed. As for the `RangeMatching` interface, I'm not sure we need it? I think it's easy enough for a user to construct `LongRange` instances to pass to `RangeFacetSetMatcher` using the static factory methods (`fromDoubles`, `fromFloats`, etc.). It feels overly complicated to introduce `FacetSetRange<...>` and then require the different `FacetSet` implementations to implement these methods to deal with inclusive/exclusive boundaries. My only suggestion here might be to rename `LongRange` to just `Range`. The API may make a little more sense that way if dealing with something other than longs. Also, should we add `double` and `float` methods to `FacetSetDecoder`? And finally, +1 to having demo code that shows how to mix-and-match types within a single point. That would be interesting to write so we make sure this thing is really fully fleshed out. I think it is though. -- 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
