You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2022/02/19 00:34:12 UTC
[GitHub] [solr] Caa52 opened a new pull request #658: Implement check for null scorer in DocSetQuery to simplify TestFilteredDocIdSet.testNullDocIdSet
Caa52 opened a new pull request #658:
URL: https://github.com/apache/solr/pull/658
This is a follow-up improvement to the creation of DocSetQuery, reference https://issues.apache.org/jira/browse/SOLR-15257
In the overriden Scorer in `DocSetQuery.createWeight` we do not currently impose a null check on `docSet`. Implementing a null check on `scorer` here could simplify some of the implementations in `TestFilteredDocIdSet`.
This PR also investigates at what seems to be an unnecessary redundancy in `TestFilteredDocIdSet`, i.e `testNullDocIdSet` seemingly functions the same as `testNullIteratorFilteredDocIdSet`, therefore potentially rendering `testNullIteratorFilteredDocIdSet` pointless as is.
- [X] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [X] I have created a Jira issue and added the issue ID to my pull request title.
- [X] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [X] I have developed this patch against the `main` branch.
- [X] I have run `./gradlew check`.
- [X] I have added tests for my changes.
- [X] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob merged pull request #658: Implement check for null scorer in DocSetQuery to simplify TestFilteredDocIdSet.testNullDocIdSet
Posted by GitBox <gi...@apache.org>.
madrob merged pull request #658:
URL: https://github.com/apache/solr/pull/658
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] Caa52 commented on a change in pull request #658: Implement check for null scorer in DocSetQuery to simplify TestFilteredDocIdSet.testNullDocIdSet
Posted by GitBox <gi...@apache.org>.
Caa52 commented on a change in pull request #658:
URL: https://github.com/apache/solr/pull/658#discussion_r810416385
##########
File path: solr/core/src/test/org/apache/solr/search/TestFilteredDocIdSet.java
##########
@@ -133,24 +133,8 @@ public void testNullDocIdSet() throws Exception {
// Now search w/ a Query which returns a null Scorer
DocSetQuery f = new DocSetQuery(null) {
@Override
- public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float boost) {
- return new Weight(this) {
-
- @Override
- public Explanation explain(LeafReaderContext context, int doc) {
- return Explanation.match(0f, "No match on id " + doc);
- }
-
- @Override
- public Scorer scorer(LeafReaderContext leafReaderContext) {
- return null;
- }
-
- @Override
- public boolean isCacheable(LeafReaderContext ctx) {
- return false;
- }
- };
+ public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float boost) throws IOException {
+ return super.createWeight(searcher, scoreMode, 0);
Review comment:
DocSetQuery now takes in a "boost" score. This is due for an update
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] Caa52 commented on a change in pull request #658: Implement check for null scorer in DocSetQuery to simplify TestFilteredDocIdSet.testNullDocIdSet
Posted by GitBox <gi...@apache.org>.
Caa52 commented on a change in pull request #658:
URL: https://github.com/apache/solr/pull/658#discussion_r810415836
##########
File path: solr/core/src/test/org/apache/solr/search/TestFilteredDocIdSet.java
##########
@@ -133,24 +133,8 @@ public void testNullDocIdSet() throws Exception {
// Now search w/ a Query which returns a null Scorer
DocSetQuery f = new DocSetQuery(null) {
@Override
- public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float boost) {
- return new Weight(this) {
-
- @Override
- public Explanation explain(LeafReaderContext context, int doc) {
- return Explanation.match(0f, "No match on id " + doc);
- }
-
- @Override
- public Scorer scorer(LeafReaderContext leafReaderContext) {
- return null;
- }
-
- @Override
- public boolean isCacheable(LeafReaderContext ctx) {
- return false;
- }
- };
Review comment:
We no longer need weight's abstract methods here since` scorer` now handles null in `DocSetQuery`
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] Caa52 commented on a change in pull request #658: Implement check for null scorer in DocSetQuery to simplify TestFilteredDocIdSet.testNullDocIdSet
Posted by GitBox <gi...@apache.org>.
Caa52 commented on a change in pull request #658:
URL: https://github.com/apache/solr/pull/658#discussion_r810423251
##########
File path: solr/core/src/test/org/apache/solr/search/TestFilteredDocIdSet.java
##########
@@ -133,24 +133,8 @@ public void testNullDocIdSet() throws Exception {
// Now search w/ a Query which returns a null Scorer
DocSetQuery f = new DocSetQuery(null) {
@Override
- public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float boost) {
- return new Weight(this) {
-
- @Override
- public Explanation explain(LeafReaderContext context, int doc) {
- return Explanation.match(0f, "No match on id " + doc);
- }
-
- @Override
- public Scorer scorer(LeafReaderContext leafReaderContext) {
- return null;
- }
-
- @Override
- public boolean isCacheable(LeafReaderContext ctx) {
- return false;
- }
- };
+ public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float boost) throws IOException {
Review comment:
Actually that's right, we shouldn't need it anymore, it's basically pointless given the changes we have in DocSetQuery. I'll run the tests to confirm. 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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] Caa52 commented on a change in pull request #658: Implement check for null scorer in DocSetQuery to simplify TestFilteredDocIdSet.testNullDocIdSet
Posted by GitBox <gi...@apache.org>.
Caa52 commented on a change in pull request #658:
URL: https://github.com/apache/solr/pull/658#discussion_r810423251
##########
File path: solr/core/src/test/org/apache/solr/search/TestFilteredDocIdSet.java
##########
@@ -133,24 +133,8 @@ public void testNullDocIdSet() throws Exception {
// Now search w/ a Query which returns a null Scorer
DocSetQuery f = new DocSetQuery(null) {
@Override
- public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float boost) {
- return new Weight(this) {
-
- @Override
- public Explanation explain(LeafReaderContext context, int doc) {
- return Explanation.match(0f, "No match on id " + doc);
- }
-
- @Override
- public Scorer scorer(LeafReaderContext leafReaderContext) {
- return null;
- }
-
- @Override
- public boolean isCacheable(LeafReaderContext ctx) {
- return false;
- }
- };
+ public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float boost) throws IOException {
Review comment:
Actually that's right, we shouldn't need it anymore, given the changes we have in DocSetQuery. I'll run the tests to confirm. 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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] Caa52 commented on pull request #658: Implement check for null scorer in DocSetQuery to simplify TestFilteredDocIdSet.testNullDocIdSet
Posted by GitBox <gi...@apache.org>.
Caa52 commented on pull request #658:
URL: https://github.com/apache/solr/pull/658#issuecomment-1045453377
@madrob please review 🙂
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #658: Implement check for null scorer in DocSetQuery to simplify TestFilteredDocIdSet.testNullDocIdSet
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #658:
URL: https://github.com/apache/solr/pull/658#discussion_r814461124
##########
File path: solr/core/src/test/org/apache/solr/search/TestFilteredDocIdSet.java
##########
@@ -131,46 +131,7 @@ public void testNullDocIdSet() throws Exception {
Assert.assertEquals(1, searcher.search(new MatchAllDocsQuery(), 10).totalHits.value);
// Now search w/ a Query which returns a null Scorer
- DocSetQuery f = new DocSetQuery(null) {
- @Override
- public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float boost) {
- return new Weight(this) {
-
- @Override
- public Explanation explain(LeafReaderContext context, int doc) {
- return Explanation.match(0f, "No match on id " + doc);
- }
-
- @Override
- public Scorer scorer(LeafReaderContext leafReaderContext) {
- return null;
- }
-
- @Override
- public boolean isCacheable(LeafReaderContext ctx) {
- return false;
- }
- };
- }
-
- @Override
- public String toString(String s) {
- return "nullDocIdSetFilter";
- }
-
- @Override
- public void visit(QueryVisitor queryVisitor) {}
-
- @Override
- public boolean equals(Object o) {
- return o == this;
- }
-
- @Override
- public int hashCode() {
- return System.identityHashCode(this);
- }
- };
+ DocSetQuery f = new DocSetQuery(DocSet.empty());
Review comment:
awesome :-)
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] Caa52 commented on a change in pull request #658: Implement check for null scorer in DocSetQuery to simplify TestFilteredDocIdSet.testNullDocIdSet
Posted by GitBox <gi...@apache.org>.
Caa52 commented on a change in pull request #658:
URL: https://github.com/apache/solr/pull/658#discussion_r810416385
##########
File path: solr/core/src/test/org/apache/solr/search/TestFilteredDocIdSet.java
##########
@@ -133,24 +133,8 @@ public void testNullDocIdSet() throws Exception {
// Now search w/ a Query which returns a null Scorer
DocSetQuery f = new DocSetQuery(null) {
@Override
- public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float boost) {
- return new Weight(this) {
-
- @Override
- public Explanation explain(LeafReaderContext context, int doc) {
- return Explanation.match(0f, "No match on id " + doc);
- }
-
- @Override
- public Scorer scorer(LeafReaderContext leafReaderContext) {
- return null;
- }
-
- @Override
- public boolean isCacheable(LeafReaderContext ctx) {
- return false;
- }
- };
+ public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float boost) throws IOException {
+ return super.createWeight(searcher, scoreMode, 0);
Review comment:
DocSetQuery has now been changed to pass in a "boost" score. This is due for an update
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] dsmiley commented on a change in pull request #658: Implement check for null scorer in DocSetQuery to simplify TestFilteredDocIdSet.testNullDocIdSet
Posted by GitBox <gi...@apache.org>.
dsmiley commented on a change in pull request #658:
URL: https://github.com/apache/solr/pull/658#discussion_r810448586
##########
File path: solr/core/src/java/org/apache/solr/search/DocSetQuery.java
##########
@@ -81,6 +81,9 @@ public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float bo
return new ConstantScoreWeight(this, boost) {
@Override
public Scorer scorer(LeafReaderContext context) {
+ if (docSet == null) {
Review comment:
How could docSet be null? I think it can't; and if I'm right we should maybe even use `Objects.requireNonNull ` in the constructor
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] madrob commented on a change in pull request #658: Implement check for null scorer in DocSetQuery to simplify TestFilteredDocIdSet.testNullDocIdSet
Posted by GitBox <gi...@apache.org>.
madrob commented on a change in pull request #658:
URL: https://github.com/apache/solr/pull/658#discussion_r810420603
##########
File path: solr/core/src/test/org/apache/solr/search/TestFilteredDocIdSet.java
##########
@@ -133,24 +133,8 @@ public void testNullDocIdSet() throws Exception {
// Now search w/ a Query which returns a null Scorer
DocSetQuery f = new DocSetQuery(null) {
@Override
- public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float boost) {
- return new Weight(this) {
-
- @Override
- public Explanation explain(LeafReaderContext context, int doc) {
- return Explanation.match(0f, "No match on id " + doc);
- }
-
- @Override
- public Scorer scorer(LeafReaderContext leafReaderContext) {
- return null;
- }
-
- @Override
- public boolean isCacheable(LeafReaderContext ctx) {
- return false;
- }
- };
+ public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float boost) throws IOException {
Review comment:
Do we need this method at all?
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] Caa52 commented on a change in pull request #658: Implement check for null scorer in DocSetQuery to simplify TestFilteredDocIdSet.testNullDocIdSet
Posted by GitBox <gi...@apache.org>.
Caa52 commented on a change in pull request #658:
URL: https://github.com/apache/solr/pull/658#discussion_r813053915
##########
File path: solr/core/src/java/org/apache/solr/search/DocSetQuery.java
##########
@@ -81,6 +81,9 @@ public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode, float bo
return new ConstantScoreWeight(this, boost) {
@Override
public Scorer scorer(LeafReaderContext context) {
+ if (docSet == null) {
Review comment:
Thanks for suggesting this. It helped me learn more about `Objects.requireNonNull` and use cases. 🙂
--
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@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org