dsmiley commented on a change in pull request #2:
URL: https://github.com/apache/solr/pull/2#discussion_r593413834
##########
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:
I see that this class is heavily borrowing from
MultiTermQueryConstantScoreWrapper as its own comments say. In that case,
let's go the other way and embrace DocIdSet in this class; don't add DocSet to
SegState. You could add a trivial DocIdSet (via anonymous class?) that simply
returns the iterator from the DocSet.
> or as a DocSet that pertains to a single segment only (if that's even a
thing?)
No; by definition a DocSet is global. It's not generic like Bits.
##########
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:
Lets scope this to BitDocSet (not SortedIntDocSet). If you are thinking
of this as a new optimization... then I'm not sure it's worth it. Maybe? I
guess a benchmark will say. It adds complexity. If this is about an
optimization, then for BitDocSet I don't think we need the cached floor arcs
but maybe just need a leaf ord based bitmap to indicate which leaves are empty.
##########
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:
Okay; you are adding complexity for an optimization (see my previous
comment). I wonder if it pays off?
##########
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:
oooooh, gotcha!
----------------------------------------------------------------
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]
