zacharymorn commented on pull request #418: URL: https://github.com/apache/lucene/pull/418#issuecomment-988548076
> These numbers are super interesting. Let's go with the approach of you baseline for now since it performs better, sorry for putting you on a track that performs worse. :) No problem! It's actually quite enjoyable for me to explore different approaches and compare their performance characteristics. I may also miss certain aspects as well, so this discussion helps to verify my understanding also! I've re-implemented the other approach in https://github.com/apache/lucene/pull/418/commits/cbbd28bb2964228542e72b112f3b2f5f3164fae7. > Regarding the new utility class, I was hoping that it could be a higher-level abstraction, eg. a `SynonymImpactsSource`, rather than low-level utility methods? Yeah I did give that a try earlier, but I ended up with the utility methods approach as I saw some potential issue. I implemented it again in https://github.com/apache/lucene/pull/418/commits/502d44e7ff0b6de6061a4899589e4523e9bf74e8. If my understanding was correct, the intention of using higher-level abstraction there was to reduce code duplication by using a loop over `Map<Field, SynonymImpactsSource>` or even `Map<Field, SynonymImpacts>` in the `numLevels`, `getDocIdUpTo` and (in particular) `getImpacts` implementations in `CombinedFieldsQuery`? However, I noticed that for `getImpacts`, it doesn't seems to be easily decomposable / delegate-able to `getImpacts` of `SynonymImpactsSource` or `SynonymImpacts`. As that method takes in a `level` and internally looks up a `docIdUpTo` of the field to get a list of impacts with that boundary, for `CombinedFieldsQuery` we actually need a `docIdUpTo` that would be valid across all fields, hence it couldn't be abstracted away in the loop https://github.com/apache/lucene/pull/418/commits/502d44e7ff0b6de6061a4899589e4523e9bf74e8#diff-62e151fca32e7560ef90ee8eab1adaffac3073f23701aaf0c17a02e250727ed3R547-R550 . Given this restriction, I feel using utility methods may wor k better in removing duplication in code? -- 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
