You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by GitBox <gi...@apache.org> on 2020/09/04 23:00:12 UTC
[GitHub] [lucene-solr] jtibshirani opened a new pull request #1834: Make sure to test normal scorers with asserting wrappers.
jtibshirani opened a new pull request #1834:
URL: https://github.com/apache/lucene-solr/pull/1834
When a query is run at the top-level, the searcher uses `Weight#bulkScorer`.
Many queries don't implement this explicitly and instead rely on the default
implementation which delegates to `Weight#scorer`.
Previously `AssertingWeight` would always wrap the delegate's bulk scorer. So
for queries that rely on `Weight#scorer`, we weren't wrapping the scorer or
iterator to run checks. This change proposes that `AssertingWeight#bulkScorer`
sometimes use the default implementation to make sure we also test normal
scorers.
This change would have caught the bug in LUCENE-9501.
----------------------------------------------------------------
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.
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-solr] jpountz commented on a change in pull request #1834: Make sure to test normal scorers with asserting wrappers.
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #1834:
URL: https://github.com/apache/lucene-solr/pull/1834#discussion_r487788434
##########
File path: lucene/CHANGES.txt
##########
@@ -204,7 +204,8 @@ Improvements
* LUCENE-9446: In BooleanQuery rewrite, always remove MatchAllDocsQuery filter clauses
when possible. (Julie Tibshirani)
-* LUCENE-9501: Improve how Asserting* test classes handle singleton doc values.
+* LUCENE-9501: Improve coverage for Asserting* test classes: make sure to handle singleton doc
+ values, and sometimes wrap Scorers by default. (Julie Tibshirani)
Review comment:
I'm not sure I'd understand what "sometimes wrap scorers" would mean as a user reading the changelog. Maybe say something along the lines of "sometimes exercise the `Weight#scorer` API rather than `Weight#bulkScorer` for top-level queries".
----------------------------------------------------------------
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.
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-solr] jpountz merged pull request #1834: Make sure to test normal scorers with asserting wrappers.
Posted by GitBox <gi...@apache.org>.
jpountz merged pull request #1834:
URL: https://github.com/apache/lucene-solr/pull/1834
----------------------------------------------------------------
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.
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-solr] jtibshirani commented on a change in pull request #1834: Make sure to test normal scorers with asserting wrappers.
Posted by GitBox <gi...@apache.org>.
jtibshirani commented on a change in pull request #1834:
URL: https://github.com/apache/lucene-solr/pull/1834#discussion_r488037072
##########
File path: lucene/CHANGES.txt
##########
@@ -204,7 +204,8 @@ Improvements
* LUCENE-9446: In BooleanQuery rewrite, always remove MatchAllDocsQuery filter clauses
when possible. (Julie Tibshirani)
-* LUCENE-9501: Improve how Asserting* test classes handle singleton doc values.
+* LUCENE-9501: Improve coverage for Asserting* test classes: make sure to handle singleton doc
+ values, and sometimes wrap Scorers by default. (Julie Tibshirani)
Review comment:
This is a good point, I updated the changelog entry.
----------------------------------------------------------------
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.
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-solr] jtibshirani commented on a change in pull request #1834: Make sure to test normal scorers with asserting wrappers.
Posted by GitBox <gi...@apache.org>.
jtibshirani commented on a change in pull request #1834:
URL: https://github.com/apache/lucene-solr/pull/1834#discussion_r485055929
##########
File path: lucene/test-framework/src/java/org/apache/lucene/search/AssertingWeight.java
##########
@@ -85,11 +85,18 @@ public long cost() {
@Override
public BulkScorer bulkScorer(LeafReaderContext context) throws IOException {
- BulkScorer inScorer = in.bulkScorer(context);
+ BulkScorer inScorer;
+ // We explicitly test both the delegate's bulk scorer, and also the normal scorer.
+ // This ensures that normal scorers are sometimes tested with an asserting wrapper.
+ if (random.nextBoolean()) {
+ inScorer = in.bulkScorer(context);
+ } else {
+ inScorer = super.bulkScorer(context);
+ }
Review comment:
That makes sense to me, I can reduce the frequency with which we take the `Scorer` path.
I didn't know about the practice of testing both with a top-level query and a filter. To me this change is still valuable, it ensures we check the scorer/ iterator contract sometimes in a clear + solid way.
----------------------------------------------------------------
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.
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-solr] jpountz commented on a change in pull request #1834: Make sure to test normal scorers with asserting wrappers.
Posted by GitBox <gi...@apache.org>.
jpountz commented on a change in pull request #1834:
URL: https://github.com/apache/lucene-solr/pull/1834#discussion_r483967822
##########
File path: lucene/test-framework/src/java/org/apache/lucene/search/AssertingWeight.java
##########
@@ -85,11 +85,18 @@ public long cost() {
@Override
public BulkScorer bulkScorer(LeafReaderContext context) throws IOException {
- BulkScorer inScorer = in.bulkScorer(context);
+ BulkScorer inScorer;
+ // We explicitly test both the delegate's bulk scorer, and also the normal scorer.
+ // This ensures that normal scorers are sometimes tested with an asserting wrapper.
+ if (random.nextBoolean()) {
+ inScorer = in.bulkScorer(context);
+ } else {
+ inScorer = super.bulkScorer(context);
+ }
Review comment:
I know of several tests that test first with a top-level query and then with a filter to make sure that both the bulk scorer and the scorer are tested. With this change, 50% of the time, we wouldn't be testing the bulk scorer, so I wonder if we should replace `random.nextBoolean()` with `frequently()` or something like that.
----------------------------------------------------------------
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.
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