gsmiller commented on PR #13568: URL: https://github.com/apache/lucene/pull/13568#issuecomment-2250492306
Thanks @epotyom ! 1. Agreed with you on all this. I'd consider two different types of users here though. I imagine one set of users who really just want to use common faceting paradigms out-of-the-box. For example, count-based faceting on a default taxonomy-based facet field. It will be significantly more complex for them to interact with this new set of abstractions/APIs to do that in comparison to the current facets module. If I were one of these users, it would be a hard sell to get me to use this new module. I imagine another set of users who have some high customization needs (e.g., computing many different aggregations for the same set of facet dims) that will really benefit from the new API. I'm thinking more of this first set of users. Having some convenient APIs to interact with that do the common things would probably cover 90+% of most users needs. Also, agreed with having demo code for both. Oh, also, also... I think it's fine to add additional sugar syntax/helpers as a follow-up if we feel good about the core API/abstractions. No need to pile more on to this change. 2. Ah, let me make sure I'm looking at the latest code. I don't think I have the most recent changes in my view so I'll fix that. I'll try to be more specific with feedback as I look more closely today. In general though, I agree that using java packages to organize code for developers is useful, but I wouldn't do it at the cost of the API surface. Modern IDEs make it very easy to navigate code in various ways without the need for an organizing java package structure. It's a balance and hard to get it perfect sometimes, but my feedback would be to bias towards an organization that creates a cleaner API surface when in doubt. 3. In terms of the ordinal abstraction feedback, it's possible I just need a little more time to "get used to it," but my reaction is coming from looking at the discrete long value counting case and seeing this indirection of mapping from long values to ordinals (and back). That indirection feels a bit forced to me. But as I said, I can't think of a better alternative really beyond duplicating code. I'll think about it some more as I do another pass through the change today. 4. Hmm, ok probably fine to leave as-is but I'll think about it some more. Maybe I'll try to sketch up a small code diff to illustrate what I had in mind and see if it's useful. 5. No, not suggesting that we don't reuse the containers that spill over from the PQ. If I have some time, let me see if I can sketch up what I mean in a quick code diff. -- 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
