Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Assign a dummy simScorer in TermsWeight if score is not needed #12383

Merged
merged 5 commits into from
Jun 30, 2023
Merged

Assign a dummy simScorer in TermsWeight if score is not needed #12383

merged 5 commits into from
Jun 30, 2023

Conversation

sgup432
Copy link
Contributor

@sgup432 sgup432 commented Jun 22, 2023

Description

Related Issue - #12297

@sgup432
Copy link
Contributor Author

sgup432 commented Jun 22, 2023

@jpountz Can you check on this?

@sgup432
Copy link
Contributor Author

sgup432 commented Jun 23, 2023 •
edited
Loading

@msfroh Can you also check?

msfroh
msfroh reviewed Jun 23, 2023
View reviewed changes
Copy link
Contributor

@msfroh msfroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you have some small formatting issues caught by the precommit checks:

503 actionable tasks: 503 executed
          @@ -75,12 +75,13 @@
           ········if·(scoreMode.needsScores())·{
           ··········this.simScorer·=·similarity.scorer(boost,·collectionStats,·termStats);
           ········}·else·{
          -··········this.simScorer·=·new·Similarity.SimScorer()·{
          -············@Override
          -············public·float·score(float·freq,·long·norm)·{
          -··············return·0f;
          -············}
          -··········};
          +··········this.simScorer·=
          +··············new·Similarity.SimScorer()·{
          +················@Override
          +················public·float·score(float·freq,·long·norm)·{
          +··················return·0f;
          +················}
          +··············};
           ········}
           ······}
           ····}
  
  IMPORTANT: run the top-level './gradlew tidy' to format code automatically (see help/formatting.txt for more info).

lucene/queries/src/test/org/apache/lucene/queries/function/TestFunctionScoreQuery.java Outdated Show resolved Hide resolved
@sgup432
Copy link
Contributor Author

sgup432 commented Jun 29, 2023

@jpountz @msfroh I have addressed comments.

jpountz
jpountz approved these changes Jun 29, 2023
View reviewed changes
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left minor suggestions but it looks good to me. Can you add a CHANGES entry under 9.8?

lucene/core/src/test/org/apache/lucene/search/TestTermQuery.java Outdated Show resolved Hide resolved
lucene/core/src/test/org/apache/lucene/search/TestTermQuery.java Outdated Show resolved Hide resolved
@sgup432
Copy link
Contributor Author

sgup432 commented Jun 29, 2023

I left minor suggestions but it looks good to me. Can you add a CHANGES entry under 9.8?

@jpountz Sure. I have added a CHANGE entry.

@jpountz jpountz merged commit 40ee6e5 into apache:main Jun 30, 2023
4 checks passed
@jpountz jpountz added this to the 9.8.0 milestone Jun 30, 2023
jpountz pushed a commit to jpountz/lucene that referenced this pull request Jun 30, 2023
@sgup432 @jpountz
Assign a dummy simScorer in TermsWeight if score is not needed (apach…
aa04747 
…e#12383)
@jpountz jpountz mentioned this pull request Jul 11, 2023
Closed
javanna added a commit to javanna/elasticsearch that referenced this pull request Sep 8, 2023
@javanna
[TEST] Address ExplainableScriptIT failure
46001af 
ExplainableScriptIT tests the output of an explainable score script. The
script declares that it does not need the score, in fact it returns the
score as the value of a field from each document. The explain output
though includes the explanation of the sub query, whose score is
completely replaced by function score. The test assertson the
sub query explanation which is thought inaccurate as the script declares
that it needs no score.

This test issue was made more evident by
apache/lucene#12383  as the sub query score
became 0 . The solution is to not include the sub query explanation in
the script explain output and remove the assertions that depend on that
part of the output.
@javanna javanna mentioned this pull request Sep 8, 2023
Merged
javanna added a commit to javanna/elasticsearch that referenced this pull request Sep 8, 2023
@javanna
[TEST] Address ExplainableScriptIT failure
08ab5ce 
ExplainableScriptIT tests the output of an explainable score script. The
script declares that it does not need the score, in fact it returns the
score as the value of a field from each document. The explain output
though includes the explanation of the sub query, whose score is
completely replaced by function score. The test assertson the
sub query explanation which is thought inaccurate as the script declares
that it needs no score.

This test issue was made more evident by
apache/lucene#12383  as the sub query score
became 0 . The solution is to not include the sub query explanation in
the script explain output and remove the assertions that depend on that
part of the output.
javanna added a commit to elastic/elasticsearch that referenced this pull request Sep 8, 2023
@javanna
[TEST] Address ExplainableScriptIT failure (#99351)
aa3f0b6 
ExplainableScriptIT tests the output of an explainable score script. The
script declares that it does not need the score, in fact it returns the
score as the value of a field from each document. The explain output
though includes the explanation of the sub query, whose score is
completely replaced by function score. The test assertson the
sub query explanation which is thought inaccurate as the script declares
that it needs no score.

This test issue was made more evident by
apache/lucene#12383  as the sub query score
became 0 . The solution is to not include the sub query explanation in
the script explain output and remove the assertions that depend on that
part of the output.
javanna added a commit to elastic/elasticsearch that referenced this pull request Sep 8, 2023
@javanna
[TEST] Address ExplainableScriptIT failure (#99351)
0fafb99 
ExplainableScriptIT tests the output of an explainable score script. The
script declares that it does not need the score, in fact it returns the
score as the value of a field from each document. The explain output
though includes the explanation of the sub query, whose score is
completely replaced by function score. The test assertson the
sub query explanation which is thought inaccurate as the script declares
that it needs no score.

This test issue was made more evident by
apache/lucene#12383  as the sub query score
became 0 . The solution is to not include the sub query explanation in
the script explain output and remove the assertions that depend on that
part of the output.
alex-spies pushed a commit to dreamquster/elasticsearch that referenced this pull request Sep 8, 2023
@javanna @alex-spies
[TEST] Address ExplainableScriptIT failure (elastic#99351)
e515cf3 
ExplainableScriptIT tests the output of an explainable score script. The
script declares that it does not need the score, in fact it returns the
score as the value of a field from each document. The explain output
though includes the explanation of the sub query, whose score is
completely replaced by function score. The test assertson the
sub query explanation which is thought inaccurate as the script declares
that it needs no score.

This test issue was made more evident by
apache/lucene#12383  as the sub query score
became 0 . The solution is to not include the sub query explanation in
the script explain output and remove the assertions that depend on that
part of the output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msfroh msfroh msfroh left review comments

@jpountz jpountz jpountz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
9.8.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sgup432 @jpountz @msfroh