Add +searchAfter task modifier and searchAfter benchmark coverage (#306) - #595
Add +searchAfter task modifier and searchAfter benchmark coverage (#306)#595serhiy-bzhezytskyy wants to merge 4 commits into
+searchAfter task modifier and searchAfter benchmark coverage (#306)#595Conversation
Adds a +searchAfter=<long> task modifier so sort benchmarks can exercise the deep-pagination (searchAfter) path, not just search(q, topN, sort). TaskParser parses the modifier and builds a FieldDoc from the given value for single-field int/long sorts. SearchTask threads the FieldDoc through and calls searcher.searchAfter(after, q, topN, sort) when set; it is folded into equals()/hashCode() so cross-run result verification and task de-duplication stay correct. This closes a blind spot noted in the Lucene 10.0 search-after regression postmortem (apache/lucene#13856): luceneutil had no searchAfter coverage. Fixes mikemccand#306
Adds a TermDayOfYearSortSearchAfter category (5 tasks) to wikinightly.tasks that sorts on dayOfYear with +searchAfter=330 -- the tail of the 1..366 range, so the non-competitive prefix is exercised via dynamic pruning. Respects the per-category 5-task cap (issue mikemccand#226).
Optional: adds a 'Sorting with searchAfter / deep pagination' section to the nightlyBench.py report for the new task category. Separated so it can be dropped if the report is populated another way.
|
Yay first light -- I can't wait! Adding |
mikemccand
left a comment
There was a problem hiding this comment.
Thank you @serhiy-bzhezytskyy -- bit by bit this is how our benchmarks improve, one battle scar at a time :) I added some minor comments.
| this.category = category; | ||
| this.isCountOnly = isCountOnly; | ||
| if (after != null && s == null) { | ||
| throw new IllegalArgumentException("searchAfter requires a sort"); |
There was a problem hiding this comment.
It's kinda strange, but Lucene doesn't actually require a sort to match the after doc. A null sort (which means sort by relevance) could take any random doc in the index with a random relevance as its after, I think? I don't think Lucene validates as its collecting that when it sees that after.doc that its relevance/sort value match after... maybe it should? Anyways, let's leave this as is and require "matched after and sort" here.
| return false; | ||
| } | ||
| if (java.util.Objects.equals( | ||
| after == null ? null : java.util.Arrays.asList(after.fields), |
There was a problem hiding this comment.
Could we use Objects.deepEquals directly on the native arrays instead of converting each to List?
I'm not sure about the blast radius of making equals possibly non-trivially more costly (new objects but they do die young) -- does the benchy use this heavily -- any HashMap checks in hot spots ... probably not?
| private final static Pattern minShouldMatchPattern = Pattern.compile(" \\+minShouldMatch=(\\d+)($| )"); | ||
| private final static Pattern constantScorePattern = Pattern.compile(" \\+constantScore($| )"); | ||
| // +searchAfter=<long>: run the sort as a deep-pagination searchAfter with this value as the | ||
| // `after` sort value (GITHUB#13313). Only meaningful together with a single-field numeric sort. |
There was a problem hiding this comment.
Maybe refine the wording to "We have only implemented the single-valued numeric case here (Lucene's searchAfter can handel any sort type)" or so?
| case LONG -> searchAfterValue; | ||
| case INT -> Math.toIntExact(searchAfterValue); | ||
| default -> throw new IllegalArgumentException( | ||
| "+searchAfter only supports INT/LONG sorts; got: " + sf.getType()); |
There was a problem hiding this comment.
Thank you for handling the two error cases separately and customizing each exception message to match each error case, and, including the failing value!
|
Thanks Mike — excited for first light! One thing still open from the description (#2): want a The one thing I'm still unsure of is a good deep |
|
Thanks Mike, all good points — pushed a commit addressing them:
(And thanks re: the split error cases — that one bit me once, so now I always echo the failing value.) One still-open item from the description that crossed with your review — the |
|
Are there any objections to its merger? I am ready to resolve them, just let me know. Thanks |
|
Hello sorry catching up ... +1 for Actually, there is somewhere a Python tool that I long ago used to do just this for the existing tasks. Maybe add this case to that tool? |
|
On the It's very skewed: with 16 equal-width buckets over 2002→2012, the last one (2011-09-29 → 2012-05-03) holds 22,651,693 of 33,332,620 docs — 68% — and the tail back to 2002 starts at 12 docs. So a round date much before 2010 would skip almost nothing and wouldn't exercise the pruning the task exists to measure. On the One caveat worth flagging: a 300k-doc prefix of the file gives p50 = 2012-04-28, three months off the true 2012-02-11 — the file isn't uniformly shuffled with respect to On the tool: I looked for the Python one and couldn't find it. The closest are |
Adds
searchAfter(deep pagination) coverage to the search benchmarks, closing #306.Today every sort task runs
searcher.search(q, topN, sort); there's nosearchAfterpath. That's the blind spot #306 tracks — it's the spinoff from the Lucene 10.0 search-after regression (apache/lucene#13856), where a dynamic-pruning change (apache/lucene#13221) caused a ~10x slowdown on mostly-sorted data that no benchy caught, and was reverted (apache/lucene#13971). This adds the missing signal.What it does
A
+searchAfter=<long>task modifier, parsed and stripped like the existing+minShouldMatch/+constantScoredirectives:TaskParserparses the modifier and builds aFieldDocfrom the value. Supported for single-field int/long sorts (the numeric dynamic-pruning path); it throws for anything else.SearchTaskthreads theFieldDocthrough and callssearcher.searchAfter(after, q, topN, sort)when set, else the existingsearch(q, topN, sort). It's folded intoequals()/hashCode()so cross-run result verification and task de-duplication stay correct.Tasks
Adds a
TermDayOfYearSortSearchAftercategory (5 queries, respecting the per-category cap from #226) towikinightly.tasks, sorting ondayOfYear(int, 1..366) with+searchAfter=330— the tail of the range, so the non-competitive prefix is exercised by dynamic pruning rather than fully collected.A couple of things I wasn't sure about — happy to adjust:
writeOneLine(...)innightlyBench.pyto display the new category. Add +constantScore task directive to TaskParser and msm tasks #583 (+constantScore) didn't touch that file, so I kept this as its own final commit — happy to drop it if the report populates another way.dayOfYearbecause its range is easy to reason about.lastMod(long, msec) is closer to the mostly-sorted timeseries shape from #13856 — I can add alastModvariant too if you'd prefer that (or as well). I wasn't sure of a good deepaftervalue for it without seeing the data distribution.Testing
Compiles against current Lucene main;
ruff check --previewon the changed Python adds no new findings; thevalidate_nightly_task_countcap check passes for the new category; and the task line parses (modifier stripped, sort type preserved). I haven't run it through a full nightly against a real wikimedium index yet — that "first light" is where the interesting part is, so I'd love to see what it shows once it runs.