Reduce IndexOrDocValuesQuery DV penalty from 8x to 6x - #16576
Conversation
The 8x penalty was set in LUCENE-7897 (2017) before DocValuesSkipper existed. With block-level skipping, DV is competitive with points for selective leads in conjunctions. Reducing to 6x lets the DV path be chosen in the crossover regime, yielding 1.78x throughput on selective-lead queries without regressing any other case.
utafrali
left a comment
utafrali
left a comment
There was a problem hiding this comment.
The core one-line threshold change is well-justified by the included JMH benchmark and the reasoning that DocValuesSkipper narrows the DV-vs-points gap since the original 8x heuristic was introduced. Approving with a few suggestions to tidy up stale exploratory comments and small polish items in the benchmark.
| // 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; |
There was a problem hiding this 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.
| @Param({"1000000", "10000000"}) | ||
| public int docCount; | ||
|
|
||
| @Setup(Level.Trial) |
There was a problem hiding this 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.
| * penalty predates DocValuesSkipper (2017). With block-level skipping, DV is competitive. | ||
| */ | ||
| @State(Scope.Thread) | ||
| @BenchmarkMode(Mode.Throughput) |
There was a problem hiding this 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.
| } | ||
| return new BooleanQuery.Builder() | ||
| .add(lead.build(), Occur.FILTER) | ||
| .add(range, Occur.FILTER) |
There was a problem hiding this 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.
| w.close(); | ||
| searcher = new IndexSearcher(reader); | ||
| searcher.setQueryCache(null); | ||
|
|
There was a problem hiding this 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.
2 participants
Revisit the threshold in IndexOrDocValuesQuery for picking points vs DV (especially now with DocVlauesSkipper).
With block-level skipping, DV is competitive with points for selective leads in conjunctions. Reducing to 6x lets the DV path be chosen in the crossover regime, yielding 1.78x throughput on selective-lead queries without regressing any other case.
/4 was also tested but caused a 43% regression at 20% lead + 10M docs due to BKD cost estimation noise at
the threshold boundary
Benchmark
AMD EPYC 7R32 (c5a.2xlarge), JDK 25, 2 forks, 5 iters × 3s.
Index: monotonic timestamps + 100-bucket keyword field, forceMerge(1).
Queries: TermQuery lead (FILTER) + LongField.newRangeQuery (FILTER).
The crossover10 case (10% lead + 80% range, 1M docs) shows 1.78x: the reduced
divisor switches from points to DV, and DV with skip blocks is faster for this
selective-lead, broad-range pattern. All other cases show no regression.
Fix #16425