madrob commented on a change in pull request #592:
URL: https://github.com/apache/solr/pull/592#discussion_r799827730
##########
File path: solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
##########
@@ -866,8 +876,48 @@ public BitDocSet getLiveDocSet() throws IOException {
// but the addition of setLiveDocs means we needed to add volatile to
"liveDocs".
BitDocSet docs = liveDocs;
if (docs == null) {
- //note: maybe should instead calc manually by segment, using
FixedBitSet.copyOf(segLiveDocs); avoid filter cache?
- liveDocs = docs = getDocSetBits(matchAllDocsQuery);
+ switch (leafContexts.size()) {
+ case 0:
+ assert numDocs() == 0;
+ docs = new BitDocSet(BitDocSet.empty().getFixedBitSet(), 0);
Review comment:
This is confusing about why we need to wrap a `empty()` in a new
BitDocSet. Clearer if we use the super type.
```suggestion
docs = new BitDocSet(DocSet.empty().getFixedBitSet(), 0);
```
##########
File path: solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
##########
@@ -866,8 +876,48 @@ public BitDocSet getLiveDocSet() throws IOException {
// but the addition of setLiveDocs means we needed to add volatile to
"liveDocs".
BitDocSet docs = liveDocs;
if (docs == null) {
- //note: maybe should instead calc manually by segment, using
FixedBitSet.copyOf(segLiveDocs); avoid filter cache?
- liveDocs = docs = getDocSetBits(matchAllDocsQuery);
+ switch (leafContexts.size()) {
+ case 0:
+ assert numDocs() == 0;
+ docs = new BitDocSet(BitDocSet.empty().getFixedBitSet(), 0);
+ break;
+ case 1:
+ final Bits onlySegLiveDocs =
leafContexts.get(0).reader().getLiveDocs();
+ final FixedBitSet fbs;
+ if (onlySegLiveDocs == null) {
+ final int onlySegMaxDoc = maxDoc();
+ fbs = new FixedBitSet(onlySegMaxDoc);
+ fbs.set(0, onlySegMaxDoc);
Review comment:
Can you add a comment mentioning the contract of LeafReader.getLiveDocs
that if it returns null then we set all the docs live?
##########
File path: solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
##########
@@ -866,8 +876,48 @@ public BitDocSet getLiveDocSet() throws IOException {
// but the addition of setLiveDocs means we needed to add volatile to
"liveDocs".
BitDocSet docs = liveDocs;
if (docs == null) {
Review comment:
Is there a race condition here when multiple match all docs queries come
in at the same time? I think that before if there were multiple queries here at
the same time then they would all sync on the same result in the filterCache.
Hard to tell, this is tricky logic. Probably good to add a test or two?
##########
File path: solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
##########
@@ -866,8 +876,48 @@ public BitDocSet getLiveDocSet() throws IOException {
// but the addition of setLiveDocs means we needed to add volatile to
"liveDocs".
BitDocSet docs = liveDocs;
if (docs == null) {
- //note: maybe should instead calc manually by segment, using
FixedBitSet.copyOf(segLiveDocs); avoid filter cache?
- liveDocs = docs = getDocSetBits(matchAllDocsQuery);
+ switch (leafContexts.size()) {
+ case 0:
+ assert numDocs() == 0;
+ docs = new BitDocSet(BitDocSet.empty().getFixedBitSet(), 0);
+ break;
+ case 1:
+ final Bits onlySegLiveDocs =
leafContexts.get(0).reader().getLiveDocs();
+ final FixedBitSet fbs;
+ if (onlySegLiveDocs == null) {
+ final int onlySegMaxDoc = maxDoc();
+ fbs = new FixedBitSet(onlySegMaxDoc);
+ fbs.set(0, onlySegMaxDoc);
+ } else {
+ fbs = FixedBitSet.copyOf(onlySegLiveDocs);
+ }
+ assert fbs.cardinality() == numDocs();
+ docs = new BitDocSet(fbs, numDocs());
+ break;
+ default:
+ final FixedBitSet bs = new FixedBitSet(maxDoc());
+ for (LeafReaderContext ctx : leafContexts) {
+ final LeafReader r = ctx.reader();
+ final Bits segLiveDocs = r.getLiveDocs();
+ final int segDocBase = ctx.docBase;
+ int segOrd = r.maxDoc() - 1;
+ if (segLiveDocs == null) {
+ do {
+ bs.set(segDocBase + segOrd);
Review comment:
Can we call the two-argument version of bs.set here? It looks like the
Lucene version does some magic to be a bit faster than a straight loop.
##########
File path: solr/core/src/test/org/apache/solr/search/TestMainQueryCaching.java
##########
@@ -0,0 +1,227 @@
+/*
+ * 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.util.Map;
+
+import org.apache.solr.SolrTestCaseJ4;
+import org.apache.solr.core.SolrCore;
+import org.apache.solr.metrics.MetricsMap;
+import org.apache.solr.metrics.SolrMetricManager;
+import org.junit.AfterClass;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import static org.apache.solr.common.util.Utils.fromJSONString;
+
+/**
+ * Verify caching interactions between main query and filterCache
+ */
+public class TestMainQueryCaching extends SolrTestCaseJ4 {
Review comment:
👍
##########
File path: solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
##########
@@ -866,8 +876,48 @@ public BitDocSet getLiveDocSet() throws IOException {
// but the addition of setLiveDocs means we needed to add volatile to
"liveDocs".
BitDocSet docs = liveDocs;
if (docs == null) {
- //note: maybe should instead calc manually by segment, using
FixedBitSet.copyOf(segLiveDocs); avoid filter cache?
- liveDocs = docs = getDocSetBits(matchAllDocsQuery);
+ switch (leafContexts.size()) {
+ case 0:
+ assert numDocs() == 0;
+ docs = new BitDocSet(BitDocSet.empty().getFixedBitSet(), 0);
+ break;
+ case 1:
+ final Bits onlySegLiveDocs =
leafContexts.get(0).reader().getLiveDocs();
+ final FixedBitSet fbs;
+ if (onlySegLiveDocs == null) {
+ final int onlySegMaxDoc = maxDoc();
+ fbs = new FixedBitSet(onlySegMaxDoc);
+ fbs.set(0, onlySegMaxDoc);
+ } else {
+ fbs = FixedBitSet.copyOf(onlySegLiveDocs);
+ }
+ assert fbs.cardinality() == numDocs();
+ docs = new BitDocSet(fbs, numDocs());
+ break;
+ default:
+ final FixedBitSet bs = new FixedBitSet(maxDoc());
+ for (LeafReaderContext ctx : leafContexts) {
+ final LeafReader r = ctx.reader();
+ final Bits segLiveDocs = r.getLiveDocs();
+ final int segDocBase = ctx.docBase;
+ int segOrd = r.maxDoc() - 1;
+ if (segLiveDocs == null) {
+ do {
+ bs.set(segDocBase + segOrd);
+ } while (segOrd-- > 0);
+ } else {
+ do {
+ if (segLiveDocs.get(segOrd)) {
+ bs.set(segDocBase + segOrd);
Review comment:
Same here, two argument version.
##########
File path: solr/core/src/java/org/apache/solr/search/SolrIndexSearcher.java
##########
@@ -1299,6 +1302,41 @@ public DocList getDocList(Query query, List<Query>
filterList, Sort lsort, int o
public static final int GET_DOCLIST = 0x02; // get the documents actually
returned in a response
public static final int GET_SCORES = 0x01;
+ private static boolean isConstantScoreQuery(Query q) {
+ if (q instanceof BoostQuery) {
+ // ConstantScoreQueries are often (always?) wrapped in BoostQuery to
assign specific score
Review comment:
Ok, thanks for checking. We might also want to check for `WrappedQuery`?
Not sure.
--
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]
