epotyom commented on PR #13568: URL: https://github.com/apache/lucene/pull/13568#issuecomment-2247513924
> I checked the new commits. Looks good! Thank you for the feedback @stefanvodita ! > A few points: > 1. Can you add CHANGES entries, please? I've added CHANGES.txt commits! > 2. `recordedOrds` and `isEmpty` are tricky in `MultiFacetsRecorder`. Maybe they should behave as if the recorder is empty, since it's not recording anything itself, just serving as a wrapper for other recorders. Or maybe we should concatenate all the ordinals from all the recorders, including duplicates? Or maybe throwing exception is correct. I don't know yet, and I think exceptions are fine for this PR, but I'm mentioning it so we can think about it and change it in the future if need be. We can think about merging results from all sub-recorders, but I'm not sure what would be the use case for it? I think we should throw an exception for now, and if we find a use case for returning ordinals from `MultiFacetsRecorder` we implement it! As for duplicates, we will probably want to dedup them? But again, maybe we find a use case where we need duplicates, I'm not sure. >3. This is speculative, but let's say we want to support non-decomposable aggregations in the future, e.g. percentiles, which require the recorder to store all the values until we reduce. If I want p99, p99.9, and p99.99, I don't want to store the values three times, once for each recorder, which is how it would work with `MultiFacetsRecorder`. Maybe in the future there is a place for a recorder that can be reduced many different ways. To put it another way, recording and reducing could be decoupled. But like I said, this is a lot of speculation and I don't suggest we change it in this PR. Interesting idea, I think we would have to come up with a new recorder implementation, which can take required percentiles as constructor arguments, and do what you suggest - record values for each doc and build results in `reduce`, or alternatively use some approximation algorithm (e.g. [this](https://github.com/timescale/timescaledb-toolkit/blob/main/docs/uddsketch.md)) -- 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
