jimczi commented on code in PR #16180: URL: https://github.com/apache/lucene/pull/16180#discussion_r3650026741
########## lucene/core/src/test/org/apache/lucene/search/TestSkipBlockRangeIteratorIntoBitSet.java: ########## @@ -0,0 +1,1095 @@ +/* + * 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.lucene.search; + +import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.Locale; +import java.util.Random; +import java.util.function.LongUnaryOperator; +import org.apache.lucene.codecs.lucene104.Lucene104Codec; +import org.apache.lucene.document.Document; +import org.apache.lucene.document.NumericDocValuesField; +import org.apache.lucene.document.SortedDocValuesField; +import org.apache.lucene.document.SortedNumericDocValuesField; +import org.apache.lucene.index.DirectoryReader; +import org.apache.lucene.index.DocValues; +import org.apache.lucene.index.DocValuesSkipper; +import org.apache.lucene.index.IndexWriter; +import org.apache.lucene.index.IndexWriterConfig; +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.index.NumericDocValues; +import org.apache.lucene.index.SortedDocValues; +import org.apache.lucene.index.SortedNumericDocValues; +import org.apache.lucene.search.BooleanClause.Occur; +import org.apache.lucene.store.Directory; +import org.apache.lucene.util.BytesRef; +import org.apache.lucene.util.FixedBitSet; + +/** + * Tests the {@code intoBitSet()} bulk path of {@link DocValuesRangeIterator} over single-valued + * numeric doc values, including YES, YES_IF_PRESENT, and MAYBE block states. + */ +public class TestSkipBlockRangeIteratorIntoBitSet extends BaseDocValuesSkipperTests { Review Comment: Tiny thing: the name reads like it's testing `SkipBlockRangeIterator`, but it's really driving `DocValuesRangeIterator` and the codec's `rangeIntoBitSet`. Something like `TestDocValuesRangeIteratorIntoBitSet` would match what's actually under test. ########## lucene/core/src/java/org/apache/lucene/index/SortedDocValues.java: ########## @@ -95,6 +96,34 @@ public TermsEnum termsEnum() throws IOException { return new SortedDocValuesTermsEnum(this); } + /** + * Fills {@code bitSet} with the doc IDs in {@code [fromDoc, toDoc)} whose ordinals are in {@code + * [minOrd, maxOrd]}. This is a bulk operation that avoids per-doc virtual dispatch overhead. + * + * <p>The default implementation falls back to per-doc evaluation via {@link #advanceExact} and + * {@link #ordValue}. Subclasses with random-access storage (e.g., dense fixed-bitsPerValue + * fields) can override this for significantly better performance. + * + * @param fromDoc first doc ID to evaluate (inclusive) + * @param toDoc last doc ID to evaluate (exclusive) + * @param minOrd lower bound of the ordinal range (inclusive) + * @param maxOrd upper bound of the ordinal range (inclusive) + * @param bitSet the bitset to fill + * @param offset subtracted from each doc ID before setting the bit + */ + public void ordinalRangeIntoBitSet( + int fromDoc, int toDoc, long minOrd, long maxOrd, FixedBitSet bitSet, int offset) + throws IOException { + for (int d = fromDoc; d < toDoc; d++) { + if (advanceExact(d)) { Review Comment: Why `advanceExact` per-doc here instead of iterating with `advance`/`nextDoc`? This is really just a range scan over the docs that have a value, which is the same pattern as the default `DocIdSetIterator.intoBitSet` and the old `BulkOrdinalRangeIterator`. Both just hop present docs with `nextDoc`. Going per-doc with `advanceExact` means sparse fields walk every gap doc (and in `IndexedDISI`'s SPARSE blocks each present doc does a little seek-back overshoot on the way), so it's a small regression there for no real win on the dense side, since dense overrides with the SIMD path anyway. Something like this keeps it consistent and makes the sparse case fall out for free: ```java for (int doc = docID() >= fromDoc ? docID() : advance(fromDoc); doc < toDoc; doc = nextDoc()) { long ord = ordValue(); if (ord >= minOrd && ord <= maxOrd) { bitSet.set(doc - offset); } } ``` Overshooting `toDoc` (even to `NO_MORE_DOCS`) is fine btw, the outer `BulkBlockRangeIterator` re-syncs the iterator through `advanceDisi`, so the final cursor position doesn't matter. And I think this is also why you had to teach the test fakes `advanceExact`. The old path only ever needed `nextDoc`/`advance`/`ordValue`, so with the iteration version those `UnsupportedOperationException` stubs can just stay. -- 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]
