OK, great! Thanks Marc. I plan on merging the PR today.
Cheers,
-Greg
On Thu, Dec 29, 2022 at 3:23 PM Marc D'Mello <marcd2...@gmail.com> wrote:
> Hi Greg,
>
> I'm also OK merging as is since this is a new feature and doesn't affect
> any of the current functionality. I also think there are no glaring issues
> with the API in its current state. However, I do think that merging the
> range and rangeonrange functionality makes sense and I like Adrien's
> suggestion of providing factory methods. I think if we merge in its current
> state we should create a new issue to refactor the range and
> rangeonrange faceting package into one and follow the RangeFieldQuery model
> more closely.
>
> On Thu, Dec 29, 2022 at 2:58 PM Greg Miller <gsmil...@gmail.com> wrote:
>
>> Hey Marc-
>>
>> I don't want to speak for Adrien as he might have something different in
>> mind, but I think that's more-or-less the idea. I'm not sure the factory
>> methods belong on the LongRange/DoubleRange classes, or if separate classes
>> should be created for this purpose (which is more how I thought of it)?
>>
>> To do this cleanly though, I'd really like us to try to consolidate all
>> the "range related" faceting functionality into one java package and
>> consolidate the API a bit. As part of this, I think we can be a little
>> smarter about not duplicating the "range" classes themselves.
>>
>> All this said, given that I think your "range on range" faceting PR is
>> ready to be merged as it currently exists, and has been through a number of
>> iteration already, I'm OK if we want to merge that work as it stands and
>> follow up with revisiting the API/naming/etc. as a future project. What do
>> you think?
>>
>> Cheers,
>> -Greg
>>
>> On Tue, Dec 13, 2022 at 7:23 PM Marc D'Mello <marcd2...@gmail.com> wrote:
>>
>>> Hi,
>>>
>>> I'm a bit unsure about what is being suggested. Is the idea to rename
>>> range#LongRange and rangeonrange#LongRange to LongFieldFacets and
>>> LongRangeFacets respectively and stick the static getters in there? In that
>>> case, I also think that the idea makes a lot of sense and that it would
>>> match our current range query API much better.
>>>
>>> In addition, looking at document#LongRange, there are queries like
>>> newContainsQuery() and newWithinQuery() that we can probably mimic to
>>> avoid exposing RangeFieldQuery.QueryType to the user.
>>>
>>> On Tue, Dec 13, 2022 at 5:04 PM Greg Miller <gsmil...@gmail.com> wrote:
>>>
>>>> Thanks for the suggestion Adrien. I like this idea! Marc- what do you
>>>> think?
>>>>
>>>> We might need to rework the package structure under the facets module
>>>> to make this clean, but that might not be a terrible thing anyway. The
>>>> existing sub-packages will make it challenging to get the visibility right.
>>>> I think it would be ideal to flatten the package so we can reduce
>>>> visibility of the class definitions and only expose the factory methods.
>>>>
>>>> Cheers,
>>>> -Greg
>>>>
>>>> On Tue, Dec 13, 2022 at 01:18 Adrien Grand <jpou...@gmail.com> wrote:
>>>>
>>>>> I wonder if the facets actually require a different name, since they
>>>>> look to me like a generalization of range facets for range fields,
>>>>> while we previously only supported range facets on numeric fields. We
>>>>> could keep calling them range facets?
>>>>>
>>>>> Maybe we could use the same model we used for queries by not exposing
>>>>> query classes to users and providing factory methods, e.g. we could
>>>>> have something like:
>>>>>
>>>>> public class LongFieldFacets {
>>>>>
>>>>> public static Facets getRangeFacetCounts(String field,
>>>>> FacetsCollector hits, LongRange... ranges) {
>>>>> return new LongRangeFacetCounts(...);
>>>>> }
>>>>>
>>>>> }
>>>>>
>>>>> public class LongRangeFacets {
>>>>>
>>>>> // same function name
>>>>> public static Facets getRangeFacetCounts(String field,
>>>>> FacetsCollector hits, RangeFieldQuery.QueryType queryType,
>>>>> LongRange... ranges) {
>>>>> return new LongRangeOnRangeFacetCounts(...);
>>>>> }
>>>>>
>>>>> }
>>>>>
>>>>> We'd still need to give a name for these classes, but the name would
>>>>> be less important since these class names would be only for ourselves.
>>>>> Users would never see them and refer to this new functionality as
>>>>> range facets on range fields?
>>>>>
>>>>> On Mon, Dec 12, 2022 at 10:11 PM Gus Heck <gus.h...@gmail.com> wrote:
>>>>> >
>>>>> > In that case, maybe "Range Logic Faceting" ?
>>>>> >
>>>>> > Relation seems too broad and too overloaded elsewhere, makes me
>>>>> think of RDBMS, related-ness, joins and such via word associations.
>>>>> >
>>>>> > On Mon, Dec 12, 2022 at 3:27 PM Greg Miller <gsmil...@gmail.com>
>>>>> wrote:
>>>>> >>
>>>>> >> Thank for the suggestion! I like the descriptiveness of it. My only
>>>>> hesitation is that is supports more than range intersection based on the
>>>>> provided QueryType instance (e.g., within, contains). I _imagine_ that
>>>>> intersection will be most common, but I don’t really know of course. I
>>>>> thought about generalizing your suggestion to something like “Range
>>>>> Relation Faceting,” but fear that would be confusing.
>>>>> >>
>>>>> >> Thanks again!
>>>>> >>
>>>>> >> Cheers,
>>>>> >> -Greg
>>>>> >>
>>>>> >> On Mon, Dec 12, 2022 at 10:19 Gus Heck <gus.h...@gmail.com> wrote:
>>>>> >>>
>>>>> >>> Maybe "Range Intersect Faceting"?
>>>>> >>>
>>>>> >>> On Mon, Dec 12, 2022 at 1:11 PM Greg Miller <gsmil...@gmail.com>
>>>>> wrote:
>>>>> >>>>
>>>>> >>>> Folks-
>>>>> >>>>
>>>>> >>>> Naming is hard! (But you all know that already).
>>>>> >>>>
>>>>> >>>> Marc D'Mello and I have been working on a new faceting
>>>>> implementation that's meant to complement Lucene's existing range-relation
>>>>> queries (e.g., LongRange#newIntersectsQuery, DoubleRange#newContainsQuery,
>>>>> LongRangeDocValuesField#newSlowIntersectsQuery, etc.). Well, I should say
>>>>> Marc is working on the change and I'm just providing nit-picky feedback on
>>>>> his PR, which is here: https://github.com/apache/lucene/pull/11901.
>>>>> The general idea of this feature is to allow users to get facet counts for
>>>>> these sorts of range-relation filters before they're applied. For example,
>>>>> if a user is indexing ranges with their documents, they may have a set of
>>>>> query-ranges they want to facet on, based on some range relationship
>>>>> (e.g.,
>>>>> intersection, contains, etc.).
>>>>> >>>>
>>>>> >>>> As a concrete example, imagine that documents contain a price
>>>>> range (maybe a document represents some e-commerce product but the price
>>>>> varies based on some configuration options), and a user wants to build a
>>>>> price range filter that applies filtering based on whether-or-not the two
>>>>> ranges intersect (i.e., DoubleRange#newIntersectsQuery to apply a price
>>>>> range filter). This user wants faceting capabilities over the different
>>>>> price ranges they want to make available, so they need a way to facet over
>>>>> a list of provided query-ranges, based on the "intersect" relationship
>>>>> with
>>>>> the doc-encoded ranges. That's what Marc's "RangeOnRange" faceting is
>>>>> trying to accomplish.
>>>>> >>>>
>>>>> >>>> In my opinion, the PR is really close to being ready (thanks
>>>>> again Marc!), but I'm wondering if we can come up with a more descriptive
>>>>> name. As it currently stands, the feature is termed "RangeOnRange
>>>>> Faceting," which feels just a bit wonky to me. That said, I can't really
>>>>> come up with anything better.
>>>>> >>>>
>>>>> >>>> ** Does anyone have suggestions on a better name? **
>>>>> >>>>
>>>>> >>>> Any / all suggestions appreciated! (And of course, any other
>>>>> input on the PR is welcome if anyone is interested).
>>>>> >>>>
>>>>> >>>> Cheers,
>>>>> >>>> -Greg
>>>>> >>>
>>>>> >>>
>>>>> >>>
>>>>> >>> --
>>>>> >>> http://www.needhamsoftware.com (work)
>>>>> >>> http://www.the111shift.com (play)
>>>>> >
>>>>> >
>>>>> >
>>>>> > --
>>>>> > http://www.needhamsoftware.com (work)
>>>>> > http://www.the111shift.com (play)
>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Adrien
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
>>>>> For additional commands, e-mail: dev-h...@lucene.apache.org
>>>>>
>>>>>
