javanna commented on PR #15557: URL: https://github.com/apache/lucene/pull/15557#issuecomment-4235791234
Hey @gaobinlong sorry for the delay in getting back to you. I had a look at this PR. I am not super familiar with the grouping API, so I may not be the best person to advise on how to move forward with the change. My initial thoughts: this PR introduces a new public API that is only used in a test. Can it be used by users as-is like such test does? Are there other places where Lucene itself could rely on this new collector manager in production code? it seems like `GroupingSearch` is a good candidate but it relies on `MultiCollector` which complicates things quite a bit and needs some rework. I am also not entirely sure about the `Collection<?>` generic type, does that need to be as broad? All in all, I am trying to determine if this is a step forward in the right direction. It may make sense to play further with GroupingSearch or in alternative keep this change small but introduce the manager only to the test that it affects, unless we are 100% sure that Lucene users can use the new manager as-is. What do you think? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
