[GitHub] [lucene] zacharymorn commented on pull request #418: LUCENE-10061: Implements dynamic pruning support for CombinedFieldsQuery

[GitBox <gi...@apache.org>]

zacharymorn commented on pull request #418:
URL: https://github.com/apache/lucene/pull/418#issuecomment-1026512709


   Hi @jpountz , just want to check again to see if I have addressed your concern with the utility approach?  
   
   To better illustrate the potential issue I'm considering with the higher level abstraction approach, I've copied over some key code snippets here: 
   
   If we were to extract out `SynonymImpactsSource` (from `SynonymQuery`), it may look something like this:
   ```
   public class SynonymImpactsSource implements ImpactsSource {
   
     private final ImpactsEnum[] impactsEnums;
     private final Impacts[] impacts;
     private final float[] boosts;
     private Impacts lead;
   
     public SynonymImpactsSource(ImpactsEnum[] impactsEnums, float[] boosts) {
       ...
     }
   
     @Override
     public Impacts getImpacts() throws IOException {
       // Use the impacts that have the lower next boundary as a lead.
       // It will decide on the number of levels and the block boundaries.
       if (lead == null) {
         Impacts tmpLead = null;
         for (int i = 0; i < impactsEnums.length; ++i) {
           impacts[i] = impactsEnums[i].getImpacts();
           if (tmpLead == null || impacts[i].getDocIdUpTo(0) < tmpLead.getDocIdUpTo(0)) {
             tmpLead = impacts[i];
           }
         }
         lead = tmpLead;
       }
       return new Impacts() {
   
         @Override
         public int numLevels() {
           // Delegate to the lead
           return lead.numLevels();
         }
   
         @Override
         public int getDocIdUpTo(int level) {
           // Delegate to the lead
           return lead.getDocIdUpTo(level);
         }
   
         @Override
         public List<Impact> getImpacts(int level) {
           final int docIdUpTo = getDocIdUpTo(level); 
           return ImpactsMergingUtils.mergeImpactsPerField(
               impactsEnums, impacts, boosts, docIdUpTo, false);
         }
       };
     }
   
     @Override
     public void advanceShallow(int target) throws IOException {
       for (ImpactsEnum impactsEnum : impactsEnums) {
         if (impactsEnum.docID() < target) {
           impactsEnum.advanceShallow(target);
         }
       }
     }
   
     public Impacts[] impacts() {
       return impacts;
     }
   }
   ```
   
   However, the above `SynonymImpactsSource#getImpacts(level)` may not be directly usable for `CombinedFieldsQuery`, as illustrated below
   ```
   Impacts CombinedFieldsQuery$ImpactsSource#getImpacts {
      for (entry : Map<Field, SynonymImpactsSource>) {
           // this is potentially problematic, as this methods only takes in level info, and it internally looks up a field-specific docIdUpTo to get valid impacts (see definition above). However, for CombinedFieldQuery, docIdUpTo may be different among different fields with same level 
           combine SynonymImpactsSource.getImpacts(level) 
      }
     return combined Impacts
   }
   ```
   
   Hence we may actually need to change some APIs if we were to make `SynonymImpactsSource` to work in `CombinedFieldQuery` use case ?


-- 
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-unsubscribe@lucene.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org