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-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
