| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
… calculation, and added a test
There was a problem hiding this comment.
LGTM, good catch @zacharymorn
Sorry, something went wrong.
| : "There is duplicated field [" | ||
| + field.field | ||
| + "] used to construct MultiNormsLeafSimScorer"; | ||
| duplicateCheckingSet.add(field.field); |
There was a problem hiding this comment.
Could be assert duplicateCheckingSet.add(field.field) == false ?
Sorry, something went wrong.
There was a problem hiding this comment.
Ah yes. I assume you meant assert duplicateCheckingSet.add(field.field) and have updated it accordingly.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks for catching this!
Sorry, something went wrong.
| dir.close(); | ||
| } | ||
|
|
||
| public void testSameScoreAndCollectionBetweenCompleteAndTopScores() throws IOException { |
There was a problem hiding this comment.
Could you explain how this test relates to the fix? Would it make sense to have a more targeted test similar to testCopyField?
Sorry, something went wrong.
There was a problem hiding this comment.
This test was actually developed in another related PR #418, and it would generate duplicated field-weight pairs in fields object to trigger the condition. As the existing tests don't currently trigger that condition, I thought this would be a good test to put in first for both PRs.
I guess if we were to have a focused test for this PR, it would still look something very similar to this test (and testCopyField), maybe something like the following, and it just ensures the test run through without assertion exception. The delta between the tests are just I removed some doc content randomizations and result comparison between TOP_SCORE and COMPLETE collection.
public void testShouldRunWithoutAssertionException() throws IOException {
int numDocs =
randomBoolean()
? atLeast(1000)
: atLeast(128 * 8 * 8 * 3); // make sure some terms have skip data
int numMatchDoc = randomIntBetween(200, 500);
int numHits = atMost(100);
int boost1 = Math.max(1, random().nextInt(5));
int boost2 = Math.max(1, random().nextInt(5));
Directory dir = newDirectory();
Similarity similarity = randomCompatibleSimilarity();
IndexWriterConfig iwc = new IndexWriterConfig();
iwc.setSimilarity(similarity);
RandomIndexWriter w = new RandomIndexWriter(random(), dir, iwc);
// adding potentially matching doc
for (int i = 0; i < numMatchDoc; i++) {
Document doc = new Document();
int freqA = random().nextInt(20) + 1;
if (randomBoolean()) {
for (int j = 0; j < freqA; j++) {
doc.add(new TextField("a", "foo", Store.NO));
}
}
freqA = random().nextInt(20) + 1;
if (randomBoolean()) {
for (int j = 0; j < freqA; j++) {
doc.add(new TextField("a", "foo" + j, Store.NO));
}
}
freqA = random().nextInt(20) + 1;
if (randomBoolean()) {
for (int j = 0; j < freqA; j++) {
doc.add(new TextField("a", "zoo", Store.NO));
}
}
int freqB = random().nextInt(20) + 1;
if (randomBoolean()) {
for (int j = 0; j < freqB; j++) {
doc.add(new TextField("b", "zoo", Store.NO));
}
}
freqB = random().nextInt(20) + 1;
if (randomBoolean()) {
for (int j = 0; j < freqB; j++) {
doc.add(new TextField("b", "zoo" + j, Store.NO));
}
}
int freqC = random().nextInt(20) + 1;
for (int j = 0; j < freqC; j++) {
doc.add(new TextField("c", "bla" + j, Store.NO));
}
w.addDocument(doc);
}
IndexReader reader = w.getReader();
IndexSearcher searcher = newSearcher(reader);
searcher.setSimilarity(similarity);
CombinedFieldQuery query =
new CombinedFieldQuery.Builder()
.addField("a", (float) boost1)
.addField("b", (float) boost2)
.addTerm(new BytesRef("foo"))
.addTerm(new BytesRef("zoo"))
.build();
TopScoreDocCollector completeCollector =
TopScoreDocCollector.create(numHits, null, Integer.MAX_VALUE);
searcher.search(query, completeCollector);
reader.close();
w.close();
dir.close();
}
I guess I'm fine either way? If you prefer a more focused test, I can replace it with the above one.
Sorry, something went wrong.
There was a problem hiding this comment.
I think starting with this more focused test makes sense for this PR.
Sorry, something went wrong.
There was a problem hiding this comment.
Sounds good. I've replaced the test with a more focused one.
Sorry, something went wrong.
|
Thanks @jimczi @jtibshirani for the review and feedback! |
Sorry, something went wrong.
There was a problem hiding this comment.
Looks good to me too!
Sorry, something went wrong.
|
Will backport this to 8.11 and 8x. |
Sorry, something went wrong.
Hi @jimczi @jtibshirani @mikemccand, just to confirm as I saw there was a thread on the proper handling of branch_8x, this change should be backported to branches branch_8_11 (version 8.11.2), branch_9_0 (version 9.0.1) & branch_9x (version 9.1.0), but not to branch branch_8x right? |
Sorry, something went wrong.
|
Correct, changes should no longer be backported to branch_8x. |
Sorry, something went wrong.
…calculation (apache#444) (cherry picked from commit 07ee3ba)
…calculation (apache#444) (cherry picked from commit 07ee3ba)
| Back | FazBrowse Home | New Git URL |
Description
Updated field-weight used in CombinedFieldQuery scoring calculation
Tests
Checklist
Please review the following and check all that apply: