zacharymorn commented on pull request #418: URL: https://github.com/apache/lucene/pull/418#issuecomment-997460370
> > 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 [cbbd28b](https://github.com/apache/lucene/commit/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 [502d44e](https://github.com/apache/lucene/commit/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 [502d44e#diff-62e151fca32e7560ef90ee8eab1adaffac3073f23701aaf0c17a02e250727ed3R547-R550](https://github.com/apache/lucene/commit/502d44e7ff0b6de6061a4899589e4523e9bf74e8#diff-62e151fca32e7560ef90ee8eab1adaffac3073f23701aaf0c17a02e250 727ed3R547-R550) . Given this restriction, I feel using utility methods may work better in removing duplication in code? Hi @jpountz , just want to circle back to this discussion and see if you may have further suggestions to the above analysis? -- 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
