mdmarshmallow commented on PR #841: URL: https://github.com/apache/lucene/pull/841#issuecomment-1148091050
Ok so I took a look at the additions you made to get more of an understanding of what is going on here. I'll try to explain my understanding, and let me know if there is anything wrong with what I'm saying. I think the biggest difference between our implementations is the naming scheme. So it seems like the `FacetSetMatcher` class is equivalent to a `HyperRectangle`. So we have an `ExactFacetSetMatcher`, which would be equivalent to making a HyperRectangle where all the min's and the max's are the same for every range (in other words, a point), and I think you proposed above a `RangeFacetSetMatcher`, which would just be a regular hyper rectangle (albeit under a different name). I wanted to address this though: > I hope that with this API we'll also pave the way for users to realize > they can implement their own `FacetSetMatcher`, for instance treating the > first 2 dimensions as range, and 3rd and 4th as exact (again, to create > specialized matchers). I think this is something that we should provide out of the box right? It seems like it could be common enough if someone is using this functionality. Maybe something like `RangedAndExactFacetSetMatcher` that lets user specify which which dimensions they want as exact matches, the details don't matter too much right now. Also for this point: > I also think that the proposed API under `facetset` is easier to > extend, even though I'm sure we can re-structure the `hyperrectangle` > package to allow for such extension. Essentially you want a _Reader_ which > knows how to read the `long[]` and a `Filter/Matcher/Whatever` which > interprets them, returning a boolean or something else. That part is the > extension point we'd hope users to implement, which will make the > underlying storage of the points an abstraction that users don't have to > deal with. I get where your coming from, but I still feel like overriding the `matches` function is still a bit of an expert use case and no super clear maybe? I have a proposal here that may be even more convoluted and a bit crazy, but I'll just put it out there in case. So starting with `FacetSet`. What if we made that `abstract FacetSet<T>` and provide an `abstract long[] writeToLong(T... values)`. Then the users will be able to store any value by extending this class and overwriting `writeToLong`. For example `LongFacetSet extends FacetSet<Long>`. Now with the matchers, I think if we provide an `ExactFacetSetMatcher`, a `RangeFacetSetMatcher`, and a `RangeAndFacetSetMatcher`, I think that would provide for all users' facet matching needs. So instead of letting the user override `match()`, we can create an `abstract readToLong()` that would read the field that the users created and stored. So for example `LongExactFacetSetMatcher extends ExactFacetSetMatcher`. Or maybe we can figure out a way to combine all the types of matchers into one to make this more simple, but we would try to have a more friendly API than what `HyperRectangle` has currently. Let me know what you think of this. -- 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
