madrob commented on a change in pull request #529:
URL: https://github.com/apache/solr/pull/529#discussion_r787145033
##########
File path: solr/core/src/java/org/apache/solr/search/DocSet.java
##########
@@ -121,7 +121,7 @@ public int andNotSize(DocSet other) {
* was generated from the top-level MultiReader that the Lucene search
* methods will be invoked with.
*/
- public abstract Filter getTopFilter();
+ public abstract DocSetQuery makeQuery();
Review comment:
Does this need to return the specialized `DocSetQuery` or could it
simply be `Query`?
##########
File path: solr/core/src/java/org/apache/solr/search/PostFilter.java
##########
@@ -18,6 +18,7 @@
import org.apache.lucene.search.IndexSearcher;
+
Review comment:
dangling whitespace change
##########
File path: solr/core/src/java/org/apache/solr/search/DocSetQuery.java
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.search;
+
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.ConstantScoreScorer;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.QueryVisitor;
+import org.apache.lucene.search.ScoreMode;
+import org.apache.lucene.search.Scorer;
+import org.apache.lucene.search.Weight;
+
+import java.io.IOException;
+import java.util.Objects;
+
+public class DocSetQuery extends SolrConstantScoreQuery {
+ protected final DocSet docSet;
+
+ public DocSetQuery(DocSet docSet) {
+ super();
+ this.docSet = docSet;
+ }
+
+ public DocSet getDocSet() {
+ return docSet;
+ }
Review comment:
Unused?
##########
File path: solr/core/src/java/org/apache/solr/search/BitsFilteredDocIdSet.java
##########
@@ -23,15 +23,11 @@
/**
* This implementation supplies a filtered DocIdSet, that excludes all
- * docids which are not in a Bits instance. This is especially useful in
- * {@link org.apache.solr.search.Filter} to apply the {@code acceptDocs}
- * passed to {@code getDocIdSet()} before returning the final DocIdSet.
+ * docids which are not in a Bits instance.
*
* @see DocIdSet
- * @see org.apache.solr.search.Filter
*/
public final class BitsFilteredDocIdSet extends FilteredDocIdSet {
Review comment:
Since we got rid of the Filter in BitDocSet, I don't think this class is
used anywhere anymore and can be deleted.
##########
File path: solr/core/src/test/org/apache/solr/search/TestFilteredDocIdSet.java
##########
@@ -127,22 +130,49 @@ public void testNullDocIdSet() throws Exception {
IndexSearcher searcher = newSearcher(reader);
Assert.assertEquals(1, searcher.search(new MatchAllDocsQuery(),
10).totalHits.value);
- // Now search w/ a Filter which returns a null DocIdSet
- Filter f = new Filter() {
+ // Now search w/ a Query which returns a null DocIdSet
+ Query f = new Query() {
@Override
- public DocIdSet getDocIdSet(LeafReaderContext context, Bits acceptDocs) {
- return null;
+ public Weight createWeight(IndexSearcher searcher, ScoreMode scoreMode,
float boost) {
+ return new Weight(this) {
+
+ @Override
+ public Explanation explain(LeafReaderContext context, int doc)
throws IOException {
+ final Scorer scorer = scorer(context);
+ final boolean match = (scorer != null &&
scorer.iterator().advance(doc) == doc);
+ if (match) {
+ assert scorer.score() == 0f;
+ return Explanation.match(0f, "Match on id " + doc);
Review comment:
This code will never be called, should simplify our implementation of
`explain` based on knowing that `scorer` always returns `null`. This same
comment applies later in the same file and also in TestSort.
##########
File path: solr/core/src/test/org/apache/solr/search/TestDocSet.java
##########
@@ -533,21 +535,21 @@ private void doTestIteratorEqual(Bits bits,
Supplier<DocIdSetIterator>... disiSu
}
}
+
/**
* Tests equivalence among {@link DocIdSetIterator} instances retrieved from
{@link BitDocSet} and {@link SortedIntDocSet}
- * implementations, via {@link DocSet#getTopFilter()}/{@link
Filter#getDocIdSet(LeafReaderContext, Bits)} and directly
- * via {@link DocSet#iterator(LeafReaderContext)}.
- * Also tests corresponding random-access {@link Bits} instances retrieved
via {@link DocSet#getTopFilter()}/
- * {@link Filter#getDocIdSet(LeafReaderContext, Bits)}/{@link
DocIdSet#bits()}.
+ * implementations, via {@link DocSet#makeQuery()} and directly via {@link
DocSet#iterator(LeafReaderContext)}.
+ * Also tests corresponding random-access {@link Bits} instances retrieved
via {@link DocSet#makeQuery()}/
+ * {@link DocIdSet#bits()}.
*/
public void doFilterTest(IndexReader reader) throws IOException {
IndexReaderContext topLevelContext = reader.getContext();
FixedBitSet bs = getRandomSet(reader.maxDoc(),
rand.nextInt(reader.maxDoc()+1));
DocSet a = new BitDocSet(bs);
DocSet b = getIntDocSet(bs);
- Filter fa = a.getTopFilter();
- Filter fb = b.getTopFilter();
+// Query fa = a.makeQuery();
+// Query fb = b.makeQuery();
Review comment:
This feels like we're now testing less than we were before. Can you
explain why your patch removes testing without adding in equivalent
functionality?
##########
File path: solr/core/src/java/org/apache/solr/search/BitDocSet.java
##########
@@ -311,72 +307,8 @@ public long cost() {
};
}
- @Override
- public Filter getTopFilter() {
- // TODO: if cardinality isn't cached, do a quick measure of sparseness
- // and return null from bits() if too sparse.
-
- return new Filter() {
- final FixedBitSet bs = bits;
-
- @Override
- public DocIdSet getDocIdSet(final LeafReaderContext context, final Bits
acceptDocs) {
- // all Solr DocSets that are used as filters only include live docs
- final Bits acceptDocs2 = acceptDocs == null ? null :
(context.reader().getLiveDocs() == acceptDocs ? null : acceptDocs);
-
- return BitsFilteredDocIdSet.wrap(new DocIdSet() {
- @Override
- public DocIdSetIterator iterator() {
- return BitDocSet.this.iterator(context);
- }
-
- @Override
- public long ramBytesUsed() {
- return BitDocSet.this.ramBytesUsed();
- }
-
- @Override
- public Bits bits() {
- if (context.isTopLevel) {
- return bits;
- }
-
- final int base = context.docBase;
- final int length = context.reader().maxDoc();
- final FixedBitSet bs = bits;
-
- return new Bits() {
- @Override
- public boolean get(int index) {
- return bs.get(index + base);
- }
-
- @Override
- public int length() {
- return length;
- }
- };
- }
-
- }, acceptDocs2);
- }
-
- @Override
- public String toString(String field) {
- return "BitSetDocTopFilter";
- }
-
- @Override
- public boolean equals(Object other) {
- return sameClassAs(other) &&
- Objects.equals(bs, getClass().cast(other).bs);
- }
-
- @Override
- public int hashCode() {
- return classHash() * 31 + bs.hashCode();
- }
- };
+ public DocSetQuery makeQuery() {
+ return new DocSetQuery(this);
Review comment:
Should we be concerned that DocSetQuery doesn't implement an equivalent
to `bits()`?
##########
File path:
solr/core/src/java/org/apache/solr/search/function/ValueSourceRangeFilter.java
##########
@@ -76,33 +78,50 @@ public boolean isIncludeUpper() {
return includeUpper;
}
+// @Override
+// public DocIdSet getDocIdSet(LeafReaderContext context, Bits acceptDocs)
throws IOException {
+// return null;
+// }
Review comment:
This should be deleted.
##########
File path: solr/core/src/java/org/apache/solr/search/DocSetQuery.java
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.search;
+
+import org.apache.lucene.index.LeafReaderContext;
+import org.apache.lucene.search.ConstantScoreScorer;
+import org.apache.lucene.search.DocIdSetIterator;
+import org.apache.lucene.search.IndexSearcher;
+import org.apache.lucene.search.QueryVisitor;
+import org.apache.lucene.search.ScoreMode;
+import org.apache.lucene.search.Scorer;
+import org.apache.lucene.search.Weight;
+
+import java.io.IOException;
+import java.util.Objects;
+
+public class DocSetQuery extends SolrConstantScoreQuery {
+ protected final DocSet docSet;
Review comment:
Why not private?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
