Skip to content

GITHUB#12370: early-terminate string sort when field is missing from the whole index - #16411

Open
serhiy-bzhezytskyy wants to merge 1 commit into
apache:mainfrom
serhiy-bzhezytskyy:GITHUB-12370-earlyterm-missing-sortfield-v2
Open

GITHUB#12370: early-terminate string sort when field is missing from the whole index#16411
serhiy-bzhezytskyy wants to merge 1 commit into
apache:mainfrom
serhiy-bzhezytskyy:GITHUB-12370-earlyterm-missing-sortfield-v2

Conversation

@serhiy-bzhezytskyy

Copy link
Copy Markdown

Closes #12370.

Description

Sorting on a string field that is missing from the whole index currently scans every document instead of early-terminating.

When the sort field doesn't exist anywhere in the segment, every document ties on the "missing" value. With no tie-breaker, once the top-N priority queue is full, all remaining documents are non-competitive and can be skipped — but TermOrdValComparator didn't recognize this case, so it visited every doc.

Fix

TermOrdValComparator now mirrors the existing singleSort competitive-iterator recognition to the minOrd path, gated on bottomValue == null (i.e. the field is missing from the segment). This lets the comparator signal non-competitiveness and skip once the queue is full, exactly as it already does for present fields.

Correctness is unchanged — results are identical; this is purely an optimization (so it's filed under Optimizations in CHANGES.txt).

Tests

Added a regression test to TestSortOptimization that asserts early termination happens (non-vacuous — it fails without the fix). ./gradlew :lucene:core:test --tests TestSortOptimization → 29 tests, all green on current main.

…the whole index

When sorting on a string (SortedSet/SortedDoc) field that does not exist
anywhere in the index and the sort has no tie breaker, all documents tie on
the missing value. Once the top-N queue is full, a further missing value can
no longer compete, so the remaining documents should be skipped instead of
fully collected.

TermOrdValComparator only recognized this on the maxOrd side; the ascending
minOrd branch hard-coded minOrd=-1 ("missing still competitive"), which tripped
the guard that disables skipping. Mirror the singleSort recognition onto the
minOrd side, gated on bottomValue==null (a genuine missing value in the queue),
which is stronger than bottomOrd==missingOrd and so does not misfire in the
sort-missing-first mixed-segment case.

Signed-off-by: Serhiy Bzhezytskyy <me@serhiy-bzhezytskyy.com>
@serhiy-bzhezytskyy

Copy link
Copy Markdown
Author

Are there any objections to its merger? I am ready to resolve them, just let me know. Thanks

@msokolov

Copy link
Copy Markdown
Contributor

It makes sense; I have two slight concerns/questions: (1) we are adding some small cost in a hot spot that usually doesn't help. I assume it's small enough not to worry, but it would be good to domenstrate with benchmarks since this is supposed to be a performance improvement. (2) Can the approach be extended to the case where there is only a single value for every document in the segment (not missing), or the case where all docs in the queue have missing value and we are sorting missing first?

@romseygeek

Copy link
Copy Markdown
Contributor

Thanks @serhiy-bzhezytskyy, this looks great! I'd echo @msokolov's request for a benchmark - I don't think it's likely that this is in a hot path, as we already have various checks to ensure that updateCompetitiveIterator() isn't called that's often, but it would be good to double-check.

On Mike's second point, this is actually something I've been working on separately and have just opened a PR for: #16424 - currently it only applies to doc-values range queries but I plan on integrating it into TopFieldCollector too.

@serhiy-bzhezytskyy

Copy link
Copy Markdown
Author

Ran the benchmark. Short answer: no measurable difference on this hardware — not one task in the suite came in below p=0.05, including the sort tasks themselves.

Setup: luceneutil wikimedium10m, baseline 9d09aa71eef (this branch's parent, rather than current main, so the one unrelated commit ahead doesn't fold into the delta) vs candidate af9873067eb. Both competitors search the same index, 20 JVM iterations, 20 warmups.

                   TaskQPS baseline   StdDev    QPS candidate   StdDev    Pct diff   p-value
      HighTermTitleSort   154.09     (10.2%)      151.53      (12.0%)   -1.7%        0.638
   HighTermTitleBDVSort    43.32      (9.0%)       44.56      (11.7%)   +2.9%        0.385
  HighTermDayOfYearSort   448.00     (16.8%)      448.09      (18.8%)    0.0%        0.997
             TermDTSort   440.18     (15.7%)      447.19      (16.2%)   +1.6%        0.752
               PKLookup   551.21      (2.2%)      545.92       (3.9%)   -1.0%        0.339
                Respell    97.71      (2.9%)       97.17       (4.5%)   -0.5%        0.647

Whole-suite spread was BrowseDateSSDVFacets -8.5% to AndMissingHigh +8.1%, with median per-task stddev 13.3%. The -1.7% on HighTermTitleSort sits well inside its own 10-12% deviation, so I'd read it as no effect rather than as a small cost — and HighTermTitleBDVSort, which sorts the same field without going through the changed comparator, moved the same amount in the other direction.

One caveat on how much this is worth: this is a laptop, and a 13.3% median floor can't rule out a small single-digit cost. An earlier run of the same two shas on the same index gave HighTermTitleSort -45.1% at p=0.002 — that turned out to be a stray process eating 5+ cores, and after quiescing the machine it became the -1.7% above. So I'd treat this as "nothing showed up at 20 iterations on one machine" rather than a tight bound. If a run on the nightly hardware would be more convincing, that seems worth doing before trusting the number.

@romseygeek on extending to the single-value and missing-first cases — #16424 looks like it covers that ground properly, and I'd rather not duplicate it. Its only overlap with this PR is CHANGES.txt. If it'd be useful once that lands, I can check the missing-field case against it, since that's the scenario this patch targets and it isn't in the standard task set.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve comparator when search sort field does not exist

3 participants