dsmiley commented on a change in pull request #2:
URL: https://github.com/apache/solr/pull/2#discussion_r593178464
##########
File path: solr/core/src/java/org/apache/solr/request/IntervalFacets.java
##########
@@ -292,16 +285,16 @@ private void getCountString() throws IOException {
final SortedDocValues singleton = DocValues.unwrapSingleton(sub);
if (singleton != null) {
// some codecs may optimize SORTED_SET storage for single-valued
fields
- accumIntervalsSingle(singleton, disi, dis.bits());
+ accumIntervalsSingle(singleton, disi, docs.getBits(leaf));
Review comment:
something is suspicious to me around here... I think IntervalFacets may
be doing more work than necessary. We shouldn't need both Bits & DISI from the
same DocSet -- pick one. Can you check?
##########
File path: solr/core/src/java/org/apache/solr/query/SolrRangeQuery.java
##########
@@ -320,16 +319,25 @@ public TermsEnum getTermsEnum(LeafReaderContext ctx)
throws IOException {
private static class SegState {
final Weight weight;
- final DocIdSet set;
+ final DocSet docs;
+ final DocIdSet docIdSet;
Review comment:
Can you easily remove SolrRangeQuery's use of "DocIdSet"? On the Solr
end of things, it's associated with the deprecated Filter that you are
removing. It's a Lucene abstraction I'd prefer to avoid if we can easily do so.
##########
File path: solr/core/src/java/org/apache/solr/search/BitDocSet.java
##########
@@ -237,6 +237,150 @@ public BitDocSet clone() {
return new BitDocSet(bits.clone(), size);
}
+ @Override
+ public Bits getBits(LeafReaderContext context) {
+ 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;
+ }
+ };
+ }
+
+ private static final int NO_DOCS_THIS_SEGMENT = -1;
+ private int[] cachedFloorDocs;
Review comment:
caching this concerns me because DocSets are themselves cached and could
be accessed concurrently. I'm sure there's an answer involving volatile or
other constructs, but my preference would to not bother -- either pre-compute
or avoid it altogether; don't cache.
##########
File path: solr/core/src/java/org/apache/solr/search/SortedIntDocSet.java
##########
@@ -671,107 +679,141 @@ public DocSet union(DocSet other) {
return new BitDocSet(newbits);
}
- @Override
- public Filter getTopFilter() {
- return new Filter() {
+ private int[] cachedOrdIdxMap;
Review comment:
What I was thinking as expressed in the JIRA issue (maybe I didn't say
it clearly) was that this would be calculated at the collection, and thus would
become an argument to create a SortedIntDocSet. Wouldn't be "cached".
##########
File path: solr/core/src/java/org/apache/solr/search/DocSet.java
##########
@@ -63,6 +65,13 @@ public static DocSet empty() {
//TODO switch to DocIdSetIterator in Solr 9?
public abstract DocIterator iterator();
+ /**
+ * Returns an ordered iterator of the documents in the set for the specified
{@link LeafReaderContext}.
+ * <b>NOTE:</b> <code>null</code> should be returned if the filter doesn't
accept any documents otherwise
Review comment:
What "filter"? Anyway, I'd word this simply as "May return null if
there are no matching documents for this leaf"
##########
File path: solr/core/src/java/org/apache/solr/search/SortedIntDocSet.java
##########
@@ -671,107 +679,141 @@ public DocSet union(DocSet other) {
return new BitDocSet(newbits);
}
- @Override
- public Filter getTopFilter() {
- return new Filter() {
+ private int[] cachedOrdIdxMap;
+ private int[] getOrdIdxMap(LeafReaderContext ctx) {
+ final int[] ret;
+ if (ctx.isTopLevel) {
+ // don't bother caching this?
+ ret = new int[] {0, docs.length - 1};
+ } else if (cachedOrdIdxMap != null) {
+ ret = cachedOrdIdxMap;
+ } else {
+ List<LeafReaderContext> leaves =
ReaderUtil.getTopLevelContext(ctx).leaves();
+ ret = new int[leaves.size() << 1];
int lastEndIdx = 0;
-
- @Override
- public DocIdSet getDocIdSet(final LeafReaderContext context, final Bits
acceptDocs) {
- LeafReader reader = context.reader();
- // all Solr DocSets that are used as filters only include live docs
- final Bits acceptDocs2 = acceptDocs == null ? null :
(reader.getLiveDocs() == acceptDocs ? null : acceptDocs);
-
- final int base = context.docBase;
- final int maxDoc = reader.maxDoc();
+ for (LeafReaderContext lrc : leaves) {
+ final int base = lrc.docBase;
+ final int maxDoc = lrc.reader().maxDoc();
final int max = base + maxDoc; // one past the max doc in this
segment.
int sidx = Math.max(0,lastEndIdx);
- if (sidx > 0 && docs[sidx-1] >= base) {
- // oops, the lastEndIdx isn't correct... we must have been used
- // in a multi-threaded context, or the indexreaders are being
- // used out-of-order. start at 0.
- sidx = 0;
- }
if (sidx < docs.length && docs[sidx] < base) {
// if docs[sidx] is < base, we need to seek to find the real start.
sidx = findIndex(docs, base, sidx, docs.length-1);
}
- final int startIdx = sidx;
-
// Largest possible end index is limited to the start index
// plus the number of docs contained in the segment. Subtract 1 since
// the end index is inclusive.
- int eidx = Math.min(docs.length, startIdx + maxDoc) - 1;
+ int eidx = Math.min(docs.length, sidx + maxDoc) - 1;
// find the real end
- eidx = findIndex(docs, max, startIdx, eidx) - 1;
+ eidx = findIndex(docs, max, sidx, eidx) - 1;
+
+ final int mapOrdIdx = lrc.ord << 1;
+ ret[mapOrdIdx] = sidx;
+ ret[mapOrdIdx + 1] = eidx;
+ lastEndIdx = eidx;
+ }
+ cachedOrdIdxMap = ret; // replace atomically after building
+ }
+ return ret;
+ }
+
+ @Override
+ public DocIdSetIterator iterator(LeafReaderContext context) {
+
+ if (docs.length == 0 || context.reader().maxDoc() < 1) {
+ // empty docset or entirely empty segment (verified that the latter
actually happens)
+ // NOTE: wrt the "empty docset" case, this is not just an optimization;
this shortcircuits also
+ // to prevent the static DocSet.EmptyLazyHolder.INSTANCE from having
cachedOrdIdxMap initiated
+ // across different IndexReaders.
+ return null;
+ }
- final int endIdx = eidx;
- lastEndIdx = endIdx;
+ int[] ordIdxMap = getOrdIdxMap(context);
+ final int base = context.docBase;
+ final int mapOrdIdx = context.ord << 1;
+ final int startIdx = ordIdxMap[mapOrdIdx];
+ final int endIdx = ordIdxMap[mapOrdIdx + 1];
+
+ if (startIdx > endIdx) {
+ return null; // verified this does happen
+ }
+
+ return new DocIdSetIterator() {
+ int idx = startIdx;
+ int adjustedDoc = -1;
+
+ @Override
+ public int docID() {
+ return adjustedDoc;
+ }
+
+ @Override
+ public int nextDoc() {
+ return adjustedDoc = (idx > endIdx) ? NO_MORE_DOCS : (docs[idx++] -
base);
+ }
+
+ @Override
+ public int advance(int target) {
+ if (idx > endIdx || target==NO_MORE_DOCS) return
adjustedDoc=NO_MORE_DOCS;
+ target += base;
+
+ // probe next
+ int rawDoc = docs[idx++];
+ if (rawDoc >= target) return adjustedDoc=rawDoc-base;
+
+ int high = endIdx;
+
+ // TODO: probe more before resorting to binary search?
+
+ // binary search
+ while (idx <= high) {
+ int mid = (idx+high) >>> 1;
+ rawDoc = docs[mid];
+
+ if (rawDoc < target) {
+ idx = mid+1;
+ }
+ else if (rawDoc > target) {
+ high = mid-1;
+ }
+ else {
+ idx=mid+1;
+ return adjustedDoc=rawDoc - base;
+ }
+ }
+
+ // low is on the insertion point...
+ if (idx <= endIdx) {
+ return adjustedDoc = docs[idx++] - base;
+ } else {
+ return adjustedDoc=NO_MORE_DOCS;
+ }
+ }
+
+ @Override
+ public long cost() {
+ return docs.length;
Review comment:
Can you provide a more accurate cost since we know exactly how many docs
are in this leaf segment?
##########
File path: solr/core/src/java/org/apache/solr/search/BitDocSet.java
##########
@@ -237,6 +237,150 @@ public BitDocSet clone() {
return new BitDocSet(bits.clone(), size);
}
+ @Override
+ public Bits getBits(LeafReaderContext context) {
+ 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;
+ }
+ };
+ }
+
+ private static final int NO_DOCS_THIS_SEGMENT = -1;
+ private int[] cachedFloorDocs;
+
+ /**
+ * Because `bits.nextSetBit(int)` (called in `nextDoc()`) has no upper
limit, lazily cache
+ * floorDocs for each segment to avoid duplicate scanning of bits (and to
enable optimization
+ * in consumers afforded by returning <code>null</code> when there are no
docs for a given
+ * segment).
+ */
+ private int getFloorDoc(final LeafReaderContext ctx) {
+ assert !ctx.isTopLevel;
+ final int[] floorDocs;
+ final int setMax = bits.length();
+ if (cachedFloorDocs != null) {
+ floorDocs = cachedFloorDocs;
+ } else {
+ List<LeafReaderContext> leaves =
ReaderUtil.getTopLevelContext(ctx).leaves();
+ floorDocs = new int[leaves.size()];
+ int idx = 0;
+ int nextFloorDoc = -1;
+ for (LeafReaderContext c : leaves) {
+ final int base = c.docBase;
+ final int max = base + c.reader().maxDoc();
+ final int recordFloorDoc;
+ if (nextFloorDoc >= max) {
+ recordFloorDoc = NO_DOCS_THIS_SEGMENT;
+ } else if (nextFloorDoc >= base) {
+ recordFloorDoc = nextFloorDoc;
+ } else if (setMax <= base || (nextFloorDoc = bits.nextSetBit(base)) >=
max) {
+ recordFloorDoc = NO_DOCS_THIS_SEGMENT;
+ } else {
+ recordFloorDoc = nextFloorDoc;
+ }
+ floorDocs[idx++] = recordFloorDoc;
+ }
+
+ cachedFloorDocs = floorDocs;
+ }
+ return floorDocs[ctx.ord];
+ }
+
+ @Override
+ public DocIdSetIterator iterator(LeafReaderContext context) {
+ if (context.isTopLevel) {
+ switch (size) {
+ case 0:
+ return null;
+ default:
+ // we have an explicit size; use it
+ return new BitSetIterator(bits, size);
Review comment:
I am very suspicious this could possibly be correct. if context's first
doc is 0 then it works. If context points to a later segment, this needs to be
shifted; no? Same for the "-1" size case just below.
##########
File path: solr/core/src/java/org/apache/solr/search/SortedIntDocSet.java
##########
@@ -671,107 +679,141 @@ public DocSet union(DocSet other) {
return new BitDocSet(newbits);
}
- @Override
- public Filter getTopFilter() {
- return new Filter() {
+ private int[] cachedOrdIdxMap;
+ private int[] getOrdIdxMap(LeafReaderContext ctx) {
+ final int[] ret;
+ if (ctx.isTopLevel) {
+ // don't bother caching this?
+ ret = new int[] {0, docs.length - 1};
+ } else if (cachedOrdIdxMap != null) {
+ ret = cachedOrdIdxMap;
+ } else {
+ List<LeafReaderContext> leaves =
ReaderUtil.getTopLevelContext(ctx).leaves();
+ ret = new int[leaves.size() << 1];
int lastEndIdx = 0;
-
- @Override
- public DocIdSet getDocIdSet(final LeafReaderContext context, final Bits
acceptDocs) {
- LeafReader reader = context.reader();
- // all Solr DocSets that are used as filters only include live docs
- final Bits acceptDocs2 = acceptDocs == null ? null :
(reader.getLiveDocs() == acceptDocs ? null : acceptDocs);
-
- final int base = context.docBase;
- final int maxDoc = reader.maxDoc();
+ for (LeafReaderContext lrc : leaves) {
+ final int base = lrc.docBase;
+ final int maxDoc = lrc.reader().maxDoc();
final int max = base + maxDoc; // one past the max doc in this
segment.
int sidx = Math.max(0,lastEndIdx);
- if (sidx > 0 && docs[sidx-1] >= base) {
- // oops, the lastEndIdx isn't correct... we must have been used
- // in a multi-threaded context, or the indexreaders are being
- // used out-of-order. start at 0.
- sidx = 0;
- }
if (sidx < docs.length && docs[sidx] < base) {
// if docs[sidx] is < base, we need to seek to find the real start.
sidx = findIndex(docs, base, sidx, docs.length-1);
}
- final int startIdx = sidx;
-
// Largest possible end index is limited to the start index
// plus the number of docs contained in the segment. Subtract 1 since
// the end index is inclusive.
- int eidx = Math.min(docs.length, startIdx + maxDoc) - 1;
+ int eidx = Math.min(docs.length, sidx + maxDoc) - 1;
// find the real end
- eidx = findIndex(docs, max, startIdx, eidx) - 1;
+ eidx = findIndex(docs, max, sidx, eidx) - 1;
+
+ final int mapOrdIdx = lrc.ord << 1;
+ ret[mapOrdIdx] = sidx;
+ ret[mapOrdIdx + 1] = eidx;
+ lastEndIdx = eidx;
+ }
+ cachedOrdIdxMap = ret; // replace atomically after building
+ }
+ return ret;
+ }
+
+ @Override
+ public DocIdSetIterator iterator(LeafReaderContext context) {
+
+ if (docs.length == 0 || context.reader().maxDoc() < 1) {
+ // empty docset or entirely empty segment (verified that the latter
actually happens)
+ // NOTE: wrt the "empty docset" case, this is not just an optimization;
this shortcircuits also
+ // to prevent the static DocSet.EmptyLazyHolder.INSTANCE from having
cachedOrdIdxMap initiated
+ // across different IndexReaders.
+ return null;
+ }
- final int endIdx = eidx;
- lastEndIdx = endIdx;
+ int[] ordIdxMap = getOrdIdxMap(context);
+ final int base = context.docBase;
+ final int mapOrdIdx = context.ord << 1;
+ final int startIdx = ordIdxMap[mapOrdIdx];
+ final int endIdx = ordIdxMap[mapOrdIdx + 1];
+
+ if (startIdx > endIdx) {
+ return null; // verified this does happen
+ }
+
+ return new DocIdSetIterator() {
+ int idx = startIdx;
+ int adjustedDoc = -1;
+
+ @Override
+ public int docID() {
+ return adjustedDoc;
+ }
+
+ @Override
+ public int nextDoc() {
+ return adjustedDoc = (idx > endIdx) ? NO_MORE_DOCS : (docs[idx++] -
base);
+ }
+
+ @Override
+ public int advance(int target) {
+ if (idx > endIdx || target==NO_MORE_DOCS) return
adjustedDoc=NO_MORE_DOCS;
+ target += base;
+
+ // probe next
+ int rawDoc = docs[idx++];
+ if (rawDoc >= target) return adjustedDoc=rawDoc-base;
+
+ int high = endIdx;
+
+ // TODO: probe more before resorting to binary search?
+
+ // binary search
+ while (idx <= high) {
+ int mid = (idx+high) >>> 1;
+ rawDoc = docs[mid];
+
+ if (rawDoc < target) {
+ idx = mid+1;
+ }
+ else if (rawDoc > target) {
+ high = mid-1;
+ }
+ else {
+ idx=mid+1;
+ return adjustedDoc=rawDoc - base;
+ }
+ }
+
+ // low is on the insertion point...
+ if (idx <= endIdx) {
+ return adjustedDoc = docs[idx++] - base;
+ } else {
+ return adjustedDoc=NO_MORE_DOCS;
+ }
+ }
+
+ @Override
+ public long cost() {
+ return docs.length;
+ }
+ };
+ }
+
+ @Override
+ public Filter getTopFilter() {
Review comment:
I thought you were including in the scope of this PR to not return
Filter anymore? No worries either way; I'll be making it go away if you don't.
We needn't scope-creep here; I'm bad at that.
##########
File path: solr/core/src/java/org/apache/solr/search/facet/SweepCountAware.java
##########
@@ -165,7 +165,10 @@ public final void incrementCount(int segOrd, int inc, int
maxIdx) {
*/
public void register(CountSlotAcc[] countAccs, LongValues toGlobal, int
maxSegOrd) {
int segOrd = maxSegOrd;
- final int maxIdx = countAccs.length - 1;
+ // NOTE: the `countAccs` array may be oversized (e.g., in the event that
one or more of the domains supplies a <code>null</code>
Review comment:
I love seeing detailed comments like this
##########
File path: solr/core/src/java/org/apache/solr/search/BitDocSet.java
##########
@@ -237,6 +237,150 @@ public BitDocSet clone() {
return new BitDocSet(bits.clone(), size);
}
+ @Override
+ public Bits getBits(LeafReaderContext context) {
+ 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;
+ }
+ };
+ }
+
+ private static final int NO_DOCS_THIS_SEGMENT = -1;
+ private int[] cachedFloorDocs;
+
+ /**
+ * Because `bits.nextSetBit(int)` (called in `nextDoc()`) has no upper
limit, lazily cache
+ * floorDocs for each segment to avoid duplicate scanning of bits (and to
enable optimization
+ * in consumers afforded by returning <code>null</code> when there are no
docs for a given
+ * segment).
+ */
+ private int getFloorDoc(final LeafReaderContext ctx) {
+ assert !ctx.isTopLevel;
+ final int[] floorDocs;
+ final int setMax = bits.length();
+ if (cachedFloorDocs != null) {
+ floorDocs = cachedFloorDocs;
+ } else {
+ List<LeafReaderContext> leaves =
ReaderUtil.getTopLevelContext(ctx).leaves();
+ floorDocs = new int[leaves.size()];
+ int idx = 0;
+ int nextFloorDoc = -1;
+ for (LeafReaderContext c : leaves) {
+ final int base = c.docBase;
+ final int max = base + c.reader().maxDoc();
+ final int recordFloorDoc;
+ if (nextFloorDoc >= max) {
+ recordFloorDoc = NO_DOCS_THIS_SEGMENT;
+ } else if (nextFloorDoc >= base) {
+ recordFloorDoc = nextFloorDoc;
+ } else if (setMax <= base || (nextFloorDoc = bits.nextSetBit(base)) >=
max) {
+ recordFloorDoc = NO_DOCS_THIS_SEGMENT;
+ } else {
+ recordFloorDoc = nextFloorDoc;
+ }
+ floorDocs[idx++] = recordFloorDoc;
+ }
+
+ cachedFloorDocs = floorDocs;
+ }
+ return floorDocs[ctx.ord];
+ }
+
+ @Override
+ public DocIdSetIterator iterator(LeafReaderContext context) {
+ if (context.isTopLevel) {
+ switch (size) {
+ case 0:
+ return null;
+ default:
+ // we have an explicit size; use it
+ return new BitSetIterator(bits, size);
+ case -1:
+ // size has not been computed; use bits.length() as an upper bound
on cost
+ final int maxSize = bits.length();
+ if (maxSize < 1) {
+ return null;
+ } else {
+ return new BitSetIterator(bits, maxSize);
+ }
+ }
+ }
+
+ final int maxDoc = context.reader().maxDoc();
+ if (maxDoc < 1) {
+ // entirely empty segment; verified this actually happens
+ return null;
+ }
+
+ final int firstDocId = getFloorDoc(context);
Review comment:
I'm suspicious that it's necessary cache and use this information. The
LeafReaderContext itself has docBase (which I see you are aware of) and
reader.maxDoc.
##########
File path: solr/core/src/java/org/apache/solr/search/DocSet.java
##########
@@ -131,6 +140,15 @@ public int andNotSize(DocSet other) {
*/
public abstract Bits getBits();
+ /**
+ * A per-segment {@link Bits} instance that has fast random access (as is
generally
+ * required of Bits). In contrast with {@link #getBits()}, only trivial work
should
+ * be done to generate a return value (i.e., if the underlying set natively
supports
+ * random access). This method should return <code>null</code> for sets that
do not
+ * support random access.
+ */
+ public abstract Bits getBits(LeafReaderContext context);
Review comment:
Hmm; I wonder if this is needed.
----------------------------------------------------------------
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:
[email protected]
