You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@lucene.apache.org by ge...@apache.org on 2020/01/08 12:37:13 UTC
[lucene-solr] branch jira/solr-13890 updated: SOLR-13890: Address
review comments
This is an automated email from the ASF dual-hosted git repository.
gerlowskija pushed a commit to branch jira/solr-13890
in repository https://gitbox.apache.org/repos/asf/lucene-solr.git
The following commit(s) were added to refs/heads/jira/solr-13890 by this push:
new cf1b39a SOLR-13890: Address review comments
cf1b39a is described below
commit cf1b39aaa9295461ed8514b14fff0cf38e10ed79
Author: Jason Gerlowski <ge...@apache.org>
AuthorDate: Wed Jan 8 07:36:39 2020 -0500
SOLR-13890: Address review comments
Thanks to David and Mikhail for review.
---
.../org/apache/lucene/search/TwoPhaseIterator.java | 2 +-
solr/CHANGES.txt | 6 +-
.../search/PerSegmentViewDocIdSetIterator.java | 80 -------------------
.../org/apache/solr/search/TermsQParserPlugin.java | 92 ++++++++--------------
.../apache/solr/search/TestTermsQParserPlugin.java | 22 +++---
solr/solr-ref-guide/src/other-parsers.adoc | 8 +-
6 files changed, 54 insertions(+), 156 deletions(-)
diff --git a/lucene/core/src/java/org/apache/lucene/search/TwoPhaseIterator.java b/lucene/core/src/java/org/apache/lucene/search/TwoPhaseIterator.java
index 437a85a..ff472e7 100644
--- a/lucene/core/src/java/org/apache/lucene/search/TwoPhaseIterator.java
+++ b/lucene/core/src/java/org/apache/lucene/search/TwoPhaseIterator.java
@@ -64,7 +64,7 @@ public abstract class TwoPhaseIterator {
TwoPhaseIteratorAsDocIdSetIterator(TwoPhaseIterator twoPhaseIterator) {
this.twoPhaseIterator = twoPhaseIterator;
- this.approximation = twoPhaseIterator.approximation();
+ this.approximation = twoPhaseIterator.approximation;
}
@Override
diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt
index b3e5858..1062390 100644
--- a/solr/CHANGES.txt
+++ b/solr/CHANGES.txt
@@ -188,9 +188,9 @@ Improvements
Regardless of the settings, the passages may be sized differently than before. (Nándor Mátravölgyi, David Smiley)
* SOLR-13890: Add "top-level" DV implementation for {!terms} queries. This approach tends to be more efficient for
- queries withvery large numbers of terms. The new implementation is used by default for method=docValuesTermsFilter
- terms queries that are searching 500 or more terms. Users wishing to ignore this heuristic can choose their
- implementation explicitly by setting a "submethod" value of either "toplevel" or "persegment".
+ queries with very large numbers of terms. The new implementation is used by default for method=docValuesTermsFilter
+ terms queries that are searching 500 or more terms. Users wishing to ignore this heuristic can choose the
+ docValuesTermsFilterTopLevel or docValuesTermsFilterPerSegment methods directly.
Optimizations
---------------------
diff --git a/solr/core/src/java/org/apache/solr/search/PerSegmentViewDocIdSetIterator.java b/solr/core/src/java/org/apache/solr/search/PerSegmentViewDocIdSetIterator.java
deleted file mode 100644
index bff4512..0000000
--- a/solr/core/src/java/org/apache/solr/search/PerSegmentViewDocIdSetIterator.java
+++ /dev/null
@@ -1,80 +0,0 @@
-/*
- * 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 java.io.IOException;
-
-import org.apache.lucene.search.DocIdSetIterator;
-
-/**
- * A {@link org.apache.lucene.search.DocIdSetIterator} which allows single-segment access to a wrapped top-level (multi-segment) DISI.
- *
- * Useful as a shim between a top-level data structure, and logic that attempts to operate on it using segment-level docIds.
- * The same wrapped DISI can be used serially by multiple PerSegmentViewDocIdSetIterators
- */
-public class PerSegmentViewDocIdSetIterator extends DocIdSetIterator {
-
- private final DocIdSetIterator disi;
- private final int docBase;
- private int nextDocBase;
- //TODO Should I have some notion of where the segment ends, so that other DISI's being lined up here aren't comparing docs across segments (which will never match)
-
- public PerSegmentViewDocIdSetIterator(DocIdSetIterator topLevelIterator, int docBase, int nextDocBase) {
- this.disi = topLevelIterator;
- this.docBase = docBase;
- this.nextDocBase = nextDocBase;
- }
-
- @Override
- public int docID() {
- return translateTopLevelDocIdToPerSegment(disi.docID());
- }
-
- @Override
- public int nextDoc() throws IOException {
- return translateTopLevelDocIdToPerSegment(disi.nextDoc());
- }
-
- @Override
- public int advance(int perSegmentTarget) throws IOException {
- if (perSegmentTarget == NO_MORE_DOCS) {
- return NO_MORE_DOCS;
- }
-
- final int topLevelTarget = perSegmentTarget + docBase;
- if (topLevelTarget >= nextDocBase) {
- return NO_MORE_DOCS; // There may be other docs in subsequent segments, but since this view only covers one segment...
- }
- return translateTopLevelDocIdToPerSegment(disi.advance(topLevelTarget));
- }
-
- private int translateTopLevelDocIdToPerSegment(int topLevelDocId) {
- if (topLevelDocId == NO_MORE_DOCS) {
- return NO_MORE_DOCS;
- }
- if (topLevelDocId >= NO_MORE_DOCS) {
- return NO_MORE_DOCS;
- }
- return topLevelDocId - docBase;
- }
-
- @Override
- public long cost() {
- return disi.cost();
- }
-}
diff --git a/solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java b/solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
index 1c0386e..30b1190 100644
--- a/solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
+++ b/solr/core/src/java/org/apache/solr/search/TermsQParserPlugin.java
@@ -19,7 +19,6 @@ package org.apache.solr.search;
import java.io.IOException;
import java.lang.invoke.MethodHandles;
import java.util.Arrays;
-import java.util.List;
import java.util.Locale;
import java.util.regex.Pattern;
@@ -43,8 +42,6 @@ import org.apache.solr.schema.PointField;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import static org.apache.lucene.search.DocIdSetIterator.NO_MORE_DOCS;
-
/**
* Finds documents whose specified field has any of the specified values. It's like
* {@link TermQParserPlugin} but multi-valued, and supports a variety of internal algorithms.
@@ -65,20 +62,17 @@ public class TermsQParserPlugin extends QParserPlugin {
/** Choose the internal algorithm */
private static final String METHOD = "method";
- private static final String SUBMETHOD = "submethod";
- private static final String TOPLEVEL_SUBMETHOD = "toplevel";
- private static final String PERSEGMENT_SUBMETHOD = "persegment";
private static enum Method {
termsFilter {
@Override
- Query makeFilter(String fname, BytesRef[] bytesRefs, SolrParams localParams) {
+ Query makeFilter(String fname, BytesRef[] bytesRefs) {
return new TermInSetQuery(fname, bytesRefs);// constant scores
}
},
booleanQuery {
@Override
- Query makeFilter(String fname, BytesRef[] byteRefs, SolrParams localParams) {
+ Query makeFilter(String fname, BytesRef[] byteRefs) {
BooleanQuery.Builder bq = new BooleanQuery.Builder();
for (BytesRef byteRef : byteRefs) {
bq.add(new TermQuery(new Term(fname, byteRef)), BooleanClause.Occur.SHOULD);
@@ -88,7 +82,7 @@ public class TermsQParserPlugin extends QParserPlugin {
},
automaton {
@Override
- Query makeFilter(String fname, BytesRef[] byteRefs, SolrParams localParams) {
+ Query makeFilter(String fname, BytesRef[] byteRefs) {
ArrayUtil.timSort(byteRefs); // same sort algo as TermInSetQuery's choice
Automaton union = Automata.makeStringUnion(Arrays.asList(byteRefs)); // input must be sorted
return new AutomatonQuery(new Term(fname), union);//constant scores
@@ -96,26 +90,25 @@ public class TermsQParserPlugin extends QParserPlugin {
},
docValuesTermsFilter {//on 4x this is FieldCacheTermsFilter but we use the 5x name any way
@Override
- Query makeFilter(String fname, BytesRef[] byteRefs, SolrParams localParams) {
- final String type = localParams.get(SUBMETHOD);
-
- if (type == null) {
- // TODO Further tune this heuristic number
- return (byteRefs.length > 700) ? new TopLevelDocValuesTermsQuery(fname, byteRefs) : new DocValuesTermsQuery(fname, byteRefs);
- }
-
- if (!(TOPLEVEL_SUBMETHOD.equals(type) || PERSEGMENT_SUBMETHOD.equals(type))) {
- throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Invalid terms 'submethod' specified; only 'toplevel' and 'persegment' supported");
- }
-
- if (TOPLEVEL_SUBMETHOD.equals(type)) {
- return new TopLevelDocValuesTermsQuery(fname, byteRefs);
- }
+ Query makeFilter(String fname, BytesRef[] byteRefs) {
+ // TODO Further tune this heuristic number
+ return (byteRefs.length > 700) ? new TopLevelDocValuesTermsQuery(fname, byteRefs) : new DocValuesTermsQuery(fname, byteRefs);
+ }
+ },
+ docValuesTermsFilterTopLevel {
+ @Override
+ Query makeFilter(String fname, BytesRef[] byteRefs) {
+ return new TopLevelDocValuesTermsQuery(fname, byteRefs);
+ }
+ },
+ docValuesTermsFilterPerSegment {
+ @Override
+ Query makeFilter(String fname, BytesRef[] byteRefs) {
return new DocValuesTermsQuery(fname, byteRefs);
}
};
- abstract Query makeFilter(String fname, BytesRef[] byteRefs, SolrParams localParams);
+ abstract Query makeFilter(String fname, BytesRef[] byteRefs);
}
@Override
@@ -124,10 +117,7 @@ public class TermsQParserPlugin extends QParserPlugin {
@Override
public Query parse() throws SyntaxError {
String fname = localParams.get(QueryParsing.F);
- FieldType ft = req.getSchema().getFieldTypeNoEx(fname);
- if (ft == null) {
- throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "Field name [" + fname + "] does not exist");
- }
+ FieldType ft = req.getSchema().getFieldType(fname);
String separator = localParams.get(SEPARATOR, ",");
String qstr = localParams.get(QueryParsing.V);//never null
Method method = Method.valueOf(localParams.get(METHOD, Method.termsFilter.name()));
@@ -164,26 +154,15 @@ public class TermsQParserPlugin extends QParserPlugin {
bytesRefs[i] = term.toBytesRef();
}
- return method.makeFilter(fname, bytesRefs, localParams);
+ return method.makeFilter(fname, bytesRefs);
}
};
}
- private static abstract class TopLevelTwoPhaseIterator extends TwoPhaseIterator {
- private final int docBase;
- private final int nextDocBase;
- public TopLevelTwoPhaseIterator(DocIdSetIterator approximation, int docBase, int nextDocBase) {
- super(new PerSegmentViewDocIdSetIterator(approximation, docBase, nextDocBase));
-
- this.docBase = docBase;
- this.nextDocBase = nextDocBase;
- }
- }
-
private static class TopLevelDocValuesTermsQuery extends DocValuesTermsQuery implements ExtendedQuery{
private final String fieldName;
- private SortedSetDocValues values;
- private LongBitSet queryTermOrdinals;
+ private SortedSetDocValues topLevelDocValues;
+ private LongBitSet topLevelTermOrdinals;
private boolean matchesAtLeastOneTerm = false;
private boolean cache = true;
private boolean cacheSeparately = false;
@@ -196,34 +175,33 @@ public class TermsQParserPlugin extends QParserPlugin {
}
public Weight createWeight(IndexSearcher searcher, final ScoreMode scoreMode, float boost) throws IOException {
- values = DocValues.getSortedSet(((SolrIndexSearcher)searcher).getSlowAtomicReader(), fieldName);
- queryTermOrdinals = new LongBitSet(values.getValueCount());
+ topLevelDocValues = DocValues.getSortedSet(((SolrIndexSearcher)searcher).getSlowAtomicReader(), fieldName);
+ topLevelTermOrdinals = new LongBitSet(topLevelDocValues.getValueCount());
PrefixCodedTerms.TermIterator iterator = getTerms().iterator();
- long lastOrdFound = 0;
+ long lastTermOrdFound = 0;
for(BytesRef term = iterator.next(); term != null; term = iterator.next()) {
- long ord = lookupTerm(values, term, lastOrdFound);
- if (ord >= 0L) {
+ long currentTermOrd = lookupTerm(topLevelDocValues, term, lastTermOrdFound);
+ if (currentTermOrd >= 0L) {
matchesAtLeastOneTerm = true;
- queryTermOrdinals.set(ord);
- lastOrdFound = ord;
+ topLevelTermOrdinals.set(currentTermOrd);
+ lastTermOrdFound = currentTermOrd;
}
}
return new ConstantScoreWeight(this, boost) {
public Scorer scorer(LeafReaderContext context) throws IOException {
- if (!matchesAtLeastOneTerm) {
+ SortedSetDocValues segmentDocValues = context.reader().getSortedSetDocValues(fieldName);
+ if (segmentDocValues == null) {
return null;
}
final int docBase = context.docBase;
- final List<LeafReaderContext> allLeaves = context.parent.leaves();
- final int nextDocBase = (allLeaves.size() == context.ord + 1) ? NO_MORE_DOCS:
- context.parent.leaves().get(context.ord + 1).docBase;
- return new ConstantScoreScorer(this, this.score(), scoreMode, new TopLevelTwoPhaseIterator(values, docBase, nextDocBase) {
+ return new ConstantScoreScorer(this, this.score(), scoreMode, new TwoPhaseIterator(segmentDocValues) {
public boolean matches() throws IOException {
- for(long ord = values.nextOrd(); ord != -1L; ord = values.nextOrd()) {
- if (queryTermOrdinals.get(ord)) {
+ topLevelDocValues.advanceExact(docBase + approximation.docID());
+ for(long ord = topLevelDocValues.nextOrd(); ord != -1L; ord = topLevelDocValues.nextOrd()) {
+ if (topLevelTermOrdinals.get(ord)) {
return true;
}
}
diff --git a/solr/core/src/test/org/apache/solr/search/TestTermsQParserPlugin.java b/solr/core/src/test/org/apache/solr/search/TestTermsQParserPlugin.java
index 7131742..5f5250c 100644
--- a/solr/core/src/test/org/apache/solr/search/TestTermsQParserPlugin.java
+++ b/solr/core/src/test/org/apache/solr/search/TestTermsQParserPlugin.java
@@ -74,27 +74,21 @@ public class TestTermsQParserPlugin extends SolrTestCaseJ4 {
class TermsParams {
public String method;
- public String submethod;
public boolean cache;
public TermsParams(String method, boolean cache) {
- this(method, null, cache);
- }
-
- public TermsParams(String method, String submethod, boolean cache) {
this.method = method;
- this.submethod = submethod;
this.cache = cache;
}
+
public String buildQuery(String fieldName, String commaDelimitedTerms) {
- final String submethodString = (submethod != null) ? " submethod=" + submethod : "";
- return "{!terms f=" + fieldName + " method=" + method + submethodString + " cache=" + cache + "}" + commaDelimitedTerms;
+ return "{!terms f=" + fieldName + " method=" + method + " cache=" + cache + "}" + commaDelimitedTerms;
}
}
@Test
- public void testTermsMethodAndSubmethodEquivalency() {
+ public void testTermsMethodEquivalency() {
// Run queries with a variety of 'method' and postfilter options.
final TermsParams[] methods = new TermsParams[] {
new TermsParams("termsFilter", true),
@@ -103,10 +97,12 @@ public class TestTermsQParserPlugin extends SolrTestCaseJ4 {
new TermsParams("booleanQuery", false),
new TermsParams("automaton", true),
new TermsParams("automaton", false),
- new TermsParams("docValuesTermsFilter", "toplevel", true),
- new TermsParams("docValuesTermsFilter", "toplevel", false),
- new TermsParams("docValuesTermsFilter", "persegment", true),
- new TermsParams("docValuesTermsFilter", "persegment", false)
+ new TermsParams("docValuesTermsFilter", true),
+ new TermsParams("docValuesTermsFilter", false),
+ new TermsParams("docValuesTermsFilterTopLevel", true),
+ new TermsParams("docValuesTermsFilterTopLevel", false),
+ new TermsParams("docValuesTermsFilterPerSegment", true),
+ new TermsParams("docValuesTermsFilterPerSegment", false)
};
for (TermsParams method : methods) {
diff --git a/solr/solr-ref-guide/src/other-parsers.adoc b/solr/solr-ref-guide/src/other-parsers.adoc
index 6d1f445..4a6a911 100644
--- a/solr/solr-ref-guide/src/other-parsers.adoc
+++ b/solr/solr-ref-guide/src/other-parsers.adoc
@@ -1031,13 +1031,17 @@ The field on which to search. This parameter is required.
Separator to use when parsing the input. If set to " " (a single blank space), will trim additional white space from the input terms. Defaults to a comma (`,`).
`method`::
-An optional parameter used to determine which of several query implementations should be used by Solr. Options are restricted to: `termsFilter`, `booleanQuery`, `automaton`, or `docValuesTermsFilter`. If unspecified, the default value is `termsFilter`. Each implementation has its own performance characteristics, and users are encouraged to experiment to determine which implementation is most performant for their use-case. Heuristics are given below.
+An optional parameter used to determine which of several query implementations should be used by Solr. Options are restricted to: `termsFilter`, `booleanQuery`, `automaton`, `docValuesTermsFilterPerSegment`, `docValuesTermsFilterTopLevel` or `docValuesTermsFilter`. If unspecified, the default value is `termsFilter`. Each implementation has its own performance characteristics, and users are encouraged to experiment to determine which implementation is most performant for their use-case [...]
+
`booleanQuery` creates a `BooleanQuery` representing the request. Scales well with index size, but poorly with the number of terms being searched for.
+
`termsFilter` the default `method`. Uses a `BooleanQuery` or a `TermInSetQuery` depending on the number of terms. Scales well with index size, but only moderately with the number of query terms.
+
-`docValuesTermsFilter` uses doc values data structures to process the request. This method scales well to a large numbers of query terms. It encompasses two implementations or submethods. Solr uses heuristics to choose between these at runtime, but users can also pick explicitly by providing a `submethod` parameter with either `toplevel` or `persegment` as a value. The `persegment` implementation is more general purpose, while `toplevel` is geared for anyone with particularly high nu [...]
+`docValuesTermsFilter` chooses between the `docValuesTermsFilterTopLevel` and `docValuesTermsFilterPerSegment` methods (see below) using the number of query terms as a rough heuristic. Users should typically use this method instead of using `docValuesTermsFilterTopLevel` or `docValuesTermsFilterPerSegment` directly, unless they've done performance testing to validate one or the other methods on queries of all sizes. Depending on the implementation picked, this method may rely on expensi [...]
++
+`docValuesTermsFilterTopLevel` uses top-level doc values data structures to find results. These data structures are more efficient as the number of query terms grows high (over several hundred). But they are also expensive to build and need to be populated lazily after each commit, causing a sometimes-noticeable slowdown on the first query after each commit. If you commit frequently, you may benefit from adding a static warming query to your `solrconfig.xml` so that this is done as a p [...]
++
+`docValuesTermsFilterPerSegment` uses segment-level doc values data structures to find results. This is more general purpose than the corresponding "top level" method, but doesn't have the same advantages at large numbers of query terms (see above).
+
`automaton` creates an `AutomatonQuery` representing the request with each term forming a union. Scales well with index size and moderately with the number of query terms.