Omit SimpleText codec in TestTermOrdValComparatorAdaptiveSkipping - #16557
Merged
dweiss merged 1 commit intoAug 26, 2026
Merged
Conversation
Member
|
Imo if it's building that many docs we should consider disabling simple text just so the test doesn't take forever or consume lots of resources |
Contributor
Author
|
Yes, that's what I did - just added an exclusion annotation. I just wondered if it's ok for this test not to work with SimpleText (even if it's slow). |
dweiss
added a commit
that referenced
this pull request
Aug 26, 2026
Contributor
Author
|
I've merged this to main and 10x. Definitely won't hurt. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This failure reproduces https://jenkins.thetaphi.de/job/Lucene-nightly-main-Linux/1922/:
I let Claude analyze the cause and it says what's shown below. I am not familiar with this code at all, @romseygeek - does it make sense to you? We can skip SimpleText in this test (like the patch does) but I wonder if it's something that should be addressed deeper:
Root cause — the adaptive-disable warmup budget is measured in skipper block crossings, and SimpleText's skipper blocks are tiny.
The chain of events:
prunes the "z" half (totalHits well below 200k).
crosses WARMUP_BOUNDARY_CROSSINGS = 16 boundaries without ever observing an effective skip (result > target), it permanently flips to DISABLED
(TermOrdValComparator.java:747).
SimpleTextDocValuesReader.java:929). So while scanning the competitive "a" prefix — where no skipping is possible because every doc is competitive — a boundary is
crossed every 8 docs, and docIDRunEnd() can't extend runs past one 8-doc block since there are no higher levels. The 16-crossing budget is exhausted around doc ~130,
still 99,870 docs away from the "z" region where skipping would have kicked in. Skipping is disabled, the whole segment is collected, and totalHits == 200000 on
both sides of the comparison.
"a" half in a handful of large advances, so the first advance past the a/z boundary produces a real skip well within the 16-crossing budget, flipping the state to
permanently ACTIVE.
So the test's assumption — "clustered data ⇒ skipping stays active" — only holds for codecs whose skipper blocks are large (or multi-level). It's arguably also a mild
weakness of the heuristic itself (the warmup budget's real-world meaning scales with the codec's block size: 16 crossings samples 64k docs on the default codec but only
128 docs on SimpleText), but SimpleText is a test-only codec, so the practical fix is on the test side: annotate the class with @SuppressCodecs("SimpleText") (or force
a codec with a realistic skipper), the same way other skipper-granularity-sensitive tests do. Note the previous fix on this test (d02fb2a) addressed a different
randomization axis (merge policy); this one is the codec axis. Want me to apply the @SuppressCodecs fix?