You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "rafalh (via GitHub)" <gi...@apache.org> on 2023/06/07 10:51:18 UTC
[GitHub] [lucene] rafalh opened a new pull request, #12354: Fix docFreq in score calculation after rewrite of boolean query consisting of blended query and boosted term query
rafalh opened a new pull request, #12354:
URL: https://github.com/apache/lucene/pull/12354
### Description
When there is a boolean query consisting of a fuzzy query and a boosted term query during rewrite:
1. fuzzy query is replaced by `BlendedTermQuery` with a series of term queries with a matching edit distance
2. `BlendedTermQuery` is replaced by a series of boosted term queries with a non-null `termStates` that have one common `docFreq` value that is false for some terms (see `BlendedTermQuery::adjustFrequencies`).
3. Because `TermQuery::equals` implementation was not taking into account `termStates`, both the generated term query with non-null `termStates` and original boosted term query were merged together. Resulting `TermQuery` `termStates` depended on hash code that is based on Solr startup time (can be changed using `tests.seed` property). Because of that similarities that use `docFreq` can return wrong score.
This PR changes `equals` and `hashCode` implementation in `TermQuery` so one generated from fuzzy query and original one are not merged together anymore. Also added a test making sure it works as intended. Test was failing for `tests.seed=1`.
Fixes #10309
--
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
Re: [PR] Fix docFreq in score calculation after rewrite of boolean query consisting of blended query and boosted term query [lucene]
Posted by "rafalh (via GitHub)" <gi...@apache.org>.
rafalh commented on code in PR #12354:
URL: https://github.com/apache/lucene/pull/12354#discussion_r2534213550
##########
lucene/core/src/java/org/apache/lucene/search/TermQuery.java:
##########
@@ -264,11 +264,25 @@ public TermStates getTermStates() {
/** Returns true iff <code>other</code> is equal to <code>this</code>. */
@Override
public boolean equals(Object other) {
- return sameClassAs(other) && term.equals(((TermQuery) other).term);
+ if (!sameClassAs(other)) {
Review Comment:
Negation was removed
--
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
Re: [PR] Fix docFreq in score calculation after rewrite of boolean query consisting of blended query and boosted term query [lucene]
Posted by "rafalh (via GitHub)" <gi...@apache.org>.
rafalh commented on code in PR #12354:
URL: https://github.com/apache/lucene/pull/12354#discussion_r2534212883
##########
lucene/core/src/java/org/apache/lucene/search/TermQuery.java:
##########
@@ -264,11 +264,25 @@ public TermStates getTermStates() {
/** Returns true iff <code>other</code> is equal to <code>this</code>. */
@Override
public boolean equals(Object other) {
- return sameClassAs(other) && term.equals(((TermQuery) other).term);
+ if (!sameClassAs(other)) {
+ return false;
+ }
+ TermQuery otherTermQuery = (TermQuery) other;
+ if (!term.equals(otherTermQuery.term)) {
+ return false;
+ }
+ if (perReaderTermState != null && otherTermQuery.perReaderTermState != null) {
+ return perReaderTermState.docFreq() == otherTermQuery.perReaderTermState.docFreq();
+ }
+ return perReaderTermState == null && otherTermQuery.perReaderTermState == null;
}
@Override
public int hashCode() {
- return classHash() ^ term.hashCode();
+ int hash = classHash() ^ term.hashCode();
+ if (perReaderTermState != null) {
+ hash ^= Integer.hashCode(perReaderTermState.docFreq());
Review Comment:
Done, but I'm not sure if calculating hash code from just one field is okay
--
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
Re: [PR] Fix docFreq in score calculation after rewrite of boolean query consisting of blended query and boosted term query [lucene]
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita merged PR #12354:
URL: https://github.com/apache/lucene/pull/12354
--
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
Re: [PR] Fix docFreq in score calculation after rewrite of boolean query consisting of blended query and boosted term query [lucene]
Posted by "rafalh (via GitHub)" <gi...@apache.org>.
rafalh commented on code in PR #12354:
URL: https://github.com/apache/lucene/pull/12354#discussion_r2534209126
##########
lucene/core/src/java/org/apache/lucene/search/TermQuery.java:
##########
@@ -264,11 +264,25 @@ public TermStates getTermStates() {
/** Returns true iff <code>other</code> is equal to <code>this</code>. */
@Override
public boolean equals(Object other) {
- return sameClassAs(other) && term.equals(((TermQuery) other).term);
+ if (!sameClassAs(other)) {
+ return false;
+ }
+ TermQuery otherTermQuery = (TermQuery) other;
+ if (!term.equals(otherTermQuery.term)) {
+ return false;
+ }
+ if (perReaderTermState != null && otherTermQuery.perReaderTermState != null) {
+ return perReaderTermState.docFreq() == otherTermQuery.perReaderTermState.docFreq();
Review Comment:
Done. I didn't do it in the first place because `TermStates` has more fields so it felt wrong to implement `equals`/`hashCode` based only on one field, but at the same time I wasn't sure what is the meaning of other fields and I was worried that comparing them too would affect performance. Let me know if new code is okay for you or if I should make some adjustments
--
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
[GitHub] [lucene] stefanvodita commented on a diff in pull request #12354: Fix docFreq in score calculation after rewrite of boolean query consisting of blended query and boosted term query
Posted by "stefanvodita (via GitHub)" <gi...@apache.org>.
stefanvodita commented on code in PR #12354:
URL: https://github.com/apache/lucene/pull/12354#discussion_r1278283127
##########
lucene/core/src/java/org/apache/lucene/search/TermQuery.java:
##########
@@ -264,11 +264,25 @@ public TermStates getTermStates() {
/** Returns true iff <code>other</code> is equal to <code>this</code>. */
@Override
public boolean equals(Object other) {
- return sameClassAs(other) && term.equals(((TermQuery) other).term);
+ if (!sameClassAs(other)) {
Review Comment:
Lucene code generally prefers to avoid `!` for negations, so this would be `sameClassAs(other) == false`.
##########
lucene/core/src/java/org/apache/lucene/search/TermQuery.java:
##########
@@ -264,11 +264,25 @@ public TermStates getTermStates() {
/** Returns true iff <code>other</code> is equal to <code>this</code>. */
@Override
public boolean equals(Object other) {
- return sameClassAs(other) && term.equals(((TermQuery) other).term);
+ if (!sameClassAs(other)) {
+ return false;
+ }
+ TermQuery otherTermQuery = (TermQuery) other;
+ if (!term.equals(otherTermQuery.term)) {
+ return false;
+ }
+ if (perReaderTermState != null && otherTermQuery.perReaderTermState != null) {
+ return perReaderTermState.docFreq() == otherTermQuery.perReaderTermState.docFreq();
Review Comment:
What do you think of implementing `equals` for `TermStates` and delegating this part to it? I see `docFreq` can throw an exception and we could handle that case too.
##########
lucene/core/src/java/org/apache/lucene/search/TermQuery.java:
##########
@@ -264,11 +264,25 @@ public TermStates getTermStates() {
/** Returns true iff <code>other</code> is equal to <code>this</code>. */
@Override
public boolean equals(Object other) {
- return sameClassAs(other) && term.equals(((TermQuery) other).term);
+ if (!sameClassAs(other)) {
+ return false;
+ }
+ TermQuery otherTermQuery = (TermQuery) other;
+ if (!term.equals(otherTermQuery.term)) {
+ return false;
+ }
+ if (perReaderTermState != null && otherTermQuery.perReaderTermState != null) {
+ return perReaderTermState.docFreq() == otherTermQuery.perReaderTermState.docFreq();
+ }
+ return perReaderTermState == null && otherTermQuery.perReaderTermState == null;
}
@Override
public int hashCode() {
- return classHash() ^ term.hashCode();
+ int hash = classHash() ^ term.hashCode();
+ if (perReaderTermState != null) {
+ hash ^= Integer.hashCode(perReaderTermState.docFreq());
Review Comment:
Similar to the question about `equals`, what if we implemented `TermStates.hashCode`?
--
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
Re: [PR] Fix docFreq in score calculation after rewrite of boolean query consisting of blended query and boosted term query [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12354:
URL: https://github.com/apache/lucene/pull/12354#issuecomment-1880902826
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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
Re: [PR] Fix docFreq in score calculation after rewrite of boolean query consisting of blended query and boosted term query [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #12354:
URL: https://github.com/apache/lucene/pull/12354#issuecomment-1790844104
Thank you @rafalh! Query scores depending on `HashMap` iteration order is really awful. And thank you @stefanvodita for reviewing. @rafalh do you want to fold in the feedback maybe? Thanks!
--
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
Re: [PR] Fix docFreq in score calculation after rewrite of boolean query consisting of blended query and boosted term query [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12354:
URL: https://github.com/apache/lucene/pull/12354#issuecomment-3413343273
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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
Re: [PR] Fix docFreq in score calculation after rewrite of boolean query consisting of blended query and boosted term query [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12354:
URL: https://github.com/apache/lucene/pull/12354#issuecomment-3599572084
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
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
Re: [PR] Fix docFreq in score calculation after rewrite of boolean query consisting of blended query and boosted term query [lucene]
Posted by "rafalh (via GitHub)" <gi...@apache.org>.
rafalh commented on PR #12354:
URL: https://github.com/apache/lucene/pull/12354#issuecomment-3542011329
Sorry for long delay @stefanvodita . I totally lost track of this PR
--
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