utafrali commented on code in PR #16576: URL: https://github.com/apache/lucene/pull/16576#discussion_r3880606832
########## lucene/benchmark-jmh/src/java/org/apache/lucene/benchmark/jmh/IndexOrDocValuesQueryBenchmark.java: ########## @@ -0,0 +1,182 @@ +/* + * 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.benchmark.jmh; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Comparator; +import java.util.concurrent.TimeUnit; +import java.util.stream.Stream; +import org.apache.lucene.document.Document; +import org.apache.lucene.document.Field; +import org.apache.lucene.document.KeywordField; +import org.apache.lucene.document.LongField; +import org.apache.lucene.index.DirectoryReader; +import org.apache.lucene.index.IndexReader; +import org.apache.lucene.index.IndexWriter; +import org.apache.lucene.index.IndexWriterConfig; +import org.apache.lucene.index.Term; +import org.apache.lucene.search.BooleanClause.Occur; +import org.apache.lucene.search.BooleanQuery; +import org.apache.lucene.search.IndexSearcher; +import org.apache.lucene.search.Query; +import org.apache.lucene.search.TermQuery; +import org.apache.lucene.search.TopDocs; +import org.apache.lucene.store.Directory; +import org.apache.lucene.store.MMapDirectory; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Level; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.TearDown; +import org.openjdk.jmh.annotations.Warmup; + +/** + * Benchmarks the points-vs-DV decision in IndexOrDocValuesQuery (LUCENE-7897 penalty). The 8x + * penalty predates DocValuesSkipper (2017). With block-level skipping, DV is competitive. + */ +@State(Scope.Thread) +@BenchmarkMode(Mode.Throughput) +@OutputTimeUnit(TimeUnit.SECONDS) +@Warmup(iterations = 3, time = 3) +@Measurement(iterations = 5, time = 5) +@Fork(value = 2, warmups = 1) +public class IndexOrDocValuesQueryBenchmark { + + private Directory dir; + private IndexReader reader; + private IndexSearcher searcher; + private Path path; + + private BooleanQuery crossover10Query; + private BooleanQuery crossover20Query; + private BooleanQuery crossover30Query; + private BooleanQuery dvFavorableQuery; + private BooleanQuery pointsFavorableQuery; + + @Param({"1000000", "10000000"}) + public int docCount; + + @Setup(Level.Trial) Review Comment: The inline threshold analysis comments here reference 8x/4x/2x variants but never mention the actual 6x threshold shipped in this PR. For a reader landing on this benchmark after the merge, the reasoning will be confusing. Consider rewriting these comments in terms of the current 6x threshold (e.g. `// 10% lead + 80% range: indexCost/6 = 133K > 100K leadCost → DV`) or dropping the exploratory notes altogether. ########## lucene/benchmark-jmh/src/java/org/apache/lucene/benchmark/jmh/IndexOrDocValuesQueryBenchmark.java: ########## @@ -0,0 +1,182 @@ +/* + * 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.benchmark.jmh; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Comparator; +import java.util.concurrent.TimeUnit; +import java.util.stream.Stream; +import org.apache.lucene.document.Document; +import org.apache.lucene.document.Field; +import org.apache.lucene.document.KeywordField; +import org.apache.lucene.document.LongField; +import org.apache.lucene.index.DirectoryReader; +import org.apache.lucene.index.IndexReader; +import org.apache.lucene.index.IndexWriter; +import org.apache.lucene.index.IndexWriterConfig; +import org.apache.lucene.index.Term; +import org.apache.lucene.search.BooleanClause.Occur; +import org.apache.lucene.search.BooleanQuery; +import org.apache.lucene.search.IndexSearcher; +import org.apache.lucene.search.Query; +import org.apache.lucene.search.TermQuery; +import org.apache.lucene.search.TopDocs; +import org.apache.lucene.store.Directory; +import org.apache.lucene.store.MMapDirectory; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Level; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.TearDown; +import org.openjdk.jmh.annotations.Warmup; + +/** + * Benchmarks the points-vs-DV decision in IndexOrDocValuesQuery (LUCENE-7897 penalty). The 8x + * penalty predates DocValuesSkipper (2017). With block-level skipping, DV is competitive. + */ +@State(Scope.Thread) +@BenchmarkMode(Mode.Throughput) Review Comment: The PR description states "5 iters × 3s" but the annotations here are `@Measurement(iterations = 5, time = 5)` and `@Warmup(iterations = 3, time = 3)`. Please reconcile the description and code so future readers reproducing the numbers know the exact settings used. ########## lucene/benchmark-jmh/src/java/org/apache/lucene/benchmark/jmh/IndexOrDocValuesQueryBenchmark.java: ########## @@ -0,0 +1,182 @@ +/* + * 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.benchmark.jmh; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Comparator; +import java.util.concurrent.TimeUnit; +import java.util.stream.Stream; +import org.apache.lucene.document.Document; +import org.apache.lucene.document.Field; +import org.apache.lucene.document.KeywordField; +import org.apache.lucene.document.LongField; +import org.apache.lucene.index.DirectoryReader; +import org.apache.lucene.index.IndexReader; +import org.apache.lucene.index.IndexWriter; +import org.apache.lucene.index.IndexWriterConfig; +import org.apache.lucene.index.Term; +import org.apache.lucene.search.BooleanClause.Occur; +import org.apache.lucene.search.BooleanQuery; +import org.apache.lucene.search.IndexSearcher; +import org.apache.lucene.search.Query; +import org.apache.lucene.search.TermQuery; +import org.apache.lucene.search.TopDocs; +import org.apache.lucene.store.Directory; +import org.apache.lucene.store.MMapDirectory; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Level; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.TearDown; +import org.openjdk.jmh.annotations.Warmup; + +/** + * Benchmarks the points-vs-DV decision in IndexOrDocValuesQuery (LUCENE-7897 penalty). The 8x + * penalty predates DocValuesSkipper (2017). With block-level skipping, DV is competitive. + */ +@State(Scope.Thread) +@BenchmarkMode(Mode.Throughput) +@OutputTimeUnit(TimeUnit.SECONDS) +@Warmup(iterations = 3, time = 3) +@Measurement(iterations = 5, time = 5) +@Fork(value = 2, warmups = 1) +public class IndexOrDocValuesQueryBenchmark { + + private Directory dir; + private IndexReader reader; + private IndexSearcher searcher; + private Path path; + + private BooleanQuery crossover10Query; + private BooleanQuery crossover20Query; + private BooleanQuery crossover30Query; + private BooleanQuery dvFavorableQuery; + private BooleanQuery pointsFavorableQuery; + + @Param({"1000000", "10000000"}) + public int docCount; + + @Setup(Level.Trial) + public void setup() throws Exception { + path = Files.createTempDirectory("idvqBench"); + dir = MMapDirectory.open(path); + + IndexWriterConfig iwc = new IndexWriterConfig(); + IndexWriter w = new IndexWriter(dir, iwc); + + int numBuckets = 100; + for (int i = 0; i < docCount; i++) { + Document doc = new Document(); + doc.add(new LongField("timestamp", i, Field.Store.NO)); + doc.add(new KeywordField("bucket", "b" + (i % numBuckets), Field.Store.NO)); + w.addDocument(doc); + } + w.forceMerge(1); + reader = DirectoryReader.open(w); + w.close(); + searcher = new IndexSearcher(reader); + searcher.setQueryCache(null); + + Query range80 = LongField.newRangeQuery("timestamp", 0, docCount * 4L / 5); + + // 10% lead + 80% range. 8x→points, 4x/2x→DV. + crossover10Query = buildConjunction(range80, 10, numBuckets); + + // 20% lead + 80% range. 8x/4x→points, 2x→DV. + // indexCost=800K, leadCost=200K. 8x: 100K<=200K→pts. 4x: 200K<=200K→pts. 2x: 400K>200K→DV. + crossover20Query = buildConjunction(range80, 20, numBuckets); + + // 30% lead + 80% range. 8x/4x/2x→points. + // indexCost=800K, leadCost=300K. 2x: 400K>300K→DV. So 2x still DV here. + // Actually 2x threshold = 400K > 300K → DV. Need 1x for points: 800K > 300K → DV. + // So 30% lead is still DV at 2x. This tests DV with more docs to check. + crossover30Query = buildConjunction(range80, 30, numBuckets); + + // DV favorable: 1% lead + 80% range. Both 8x and 4x choose DV. + dvFavorableQuery = + new BooleanQuery.Builder() + .add(new TermQuery(new Term("bucket", "b0")), Occur.FILTER) + .add(range80, Occur.FILTER) + .build(); + + // Points favorable: 50% lead + 5% range. Both choose points. + Query range5 = LongField.newRangeQuery("timestamp", 0, docCount / 20); + BooleanQuery.Builder lead50 = new BooleanQuery.Builder(); + for (int i = 0; i < 50; i++) { + lead50.add(new TermQuery(new Term("bucket", "b" + i)), Occur.SHOULD); + } + pointsFavorableQuery = + new BooleanQuery.Builder() + .add(lead50.build(), Occur.FILTER) + .add(range5, Occur.FILTER) + .build(); + } + + private static BooleanQuery buildConjunction(Query range, int leadBuckets, int totalBuckets) { + BooleanQuery.Builder lead = new BooleanQuery.Builder(); + for (int i = 0; i < leadBuckets; i++) { + lead.add(new TermQuery(new Term("bucket", "b" + i)), Occur.SHOULD); + } + return new BooleanQuery.Builder() + .add(lead.build(), Occur.FILTER) + .add(range, Occur.FILTER) Review Comment: The manual `Files.walk` + `File.delete` teardown swallows failures silently and doesn't close the walk stream on the delete path. Lucene already has `IOUtils.rm(Path...)` which handles this correctly and is used by other JMH benchmarks in this module. Prefer `IOUtils.rm(path)` here. ########## lucene/benchmark-jmh/src/java/org/apache/lucene/benchmark/jmh/IndexOrDocValuesQueryBenchmark.java: ########## @@ -0,0 +1,182 @@ +/* + * 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.benchmark.jmh; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Comparator; +import java.util.concurrent.TimeUnit; +import java.util.stream.Stream; +import org.apache.lucene.document.Document; +import org.apache.lucene.document.Field; +import org.apache.lucene.document.KeywordField; +import org.apache.lucene.document.LongField; +import org.apache.lucene.index.DirectoryReader; +import org.apache.lucene.index.IndexReader; +import org.apache.lucene.index.IndexWriter; +import org.apache.lucene.index.IndexWriterConfig; +import org.apache.lucene.index.Term; +import org.apache.lucene.search.BooleanClause.Occur; +import org.apache.lucene.search.BooleanQuery; +import org.apache.lucene.search.IndexSearcher; +import org.apache.lucene.search.Query; +import org.apache.lucene.search.TermQuery; +import org.apache.lucene.search.TopDocs; +import org.apache.lucene.store.Directory; +import org.apache.lucene.store.MMapDirectory; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Level; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.TearDown; +import org.openjdk.jmh.annotations.Warmup; + +/** + * Benchmarks the points-vs-DV decision in IndexOrDocValuesQuery (LUCENE-7897 penalty). The 8x + * penalty predates DocValuesSkipper (2017). With block-level skipping, DV is competitive. + */ +@State(Scope.Thread) +@BenchmarkMode(Mode.Throughput) +@OutputTimeUnit(TimeUnit.SECONDS) +@Warmup(iterations = 3, time = 3) +@Measurement(iterations = 5, time = 5) +@Fork(value = 2, warmups = 1) +public class IndexOrDocValuesQueryBenchmark { + + private Directory dir; + private IndexReader reader; + private IndexSearcher searcher; + private Path path; + + private BooleanQuery crossover10Query; + private BooleanQuery crossover20Query; + private BooleanQuery crossover30Query; + private BooleanQuery dvFavorableQuery; + private BooleanQuery pointsFavorableQuery; + + @Param({"1000000", "10000000"}) + public int docCount; + + @Setup(Level.Trial) + public void setup() throws Exception { + path = Files.createTempDirectory("idvqBench"); + dir = MMapDirectory.open(path); + + IndexWriterConfig iwc = new IndexWriterConfig(); + IndexWriter w = new IndexWriter(dir, iwc); + + int numBuckets = 100; + for (int i = 0; i < docCount; i++) { + Document doc = new Document(); + doc.add(new LongField("timestamp", i, Field.Store.NO)); + doc.add(new KeywordField("bucket", "b" + (i % numBuckets), Field.Store.NO)); + w.addDocument(doc); + } + w.forceMerge(1); + reader = DirectoryReader.open(w); + w.close(); + searcher = new IndexSearcher(reader); + searcher.setQueryCache(null); + Review Comment: At `docCount = 10_000_000`, `range80 = LongField.newRangeQuery("timestamp", 0, docCount * 4L / 5)` is shared across all queries, but the boolean queries built from it are stored as fields. Since the docs are inserted in monotonic order and force-merged into one segment, the timestamp distribution is perfectly sequential. This is fine for measuring the point-vs-DV decision, but worth calling out in the class Javadoc so readers don't mistake these results for a general query workload; monotonic timestamps are close to a best case for BKD. ########## lucene/core/src/java/org/apache/lucene/search/IndexOrDocValuesQuery.java: ########## @@ -174,8 +174,10 @@ public Scorer get(long leadCost) throws IOException { // At equal costs, doc values tend to be worse than points since they // still need to perform one comparison per document while points can // do much better than that given how values are organized. So we give - // an arbitrary 8x penalty to doc values. - final long threshold = cost() >>> 3; + // a 6x penalty to doc values. (Reduced from 8x: DocValuesSkipper + // enables block-level skipping that makes DV competitive with points + // for selective leads in conjunctions.) + final long threshold = cost() / 6; Review Comment: Minor: switching from `cost() >>> 3` to `cost() / 6` moves from an unsigned bit shift to a signed division. Since `cost()` is documented to be non-negative this is functionally fine, but if you want to keep the unsigned semantics of the previous code as a defensive measure, `Long.divideUnsigned(cost(), 6)` would preserve that. Not a blocker given `cost()` contract. -- 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]
