Skip to content

Ensure vector queries handle advanceShallow correctly#14858

Merged
benwtrent merged 3 commits intoapache:branch_9_12from
benwtrent:bugfix/14857
Jun 30, 2025
Merged

Ensure vector queries handle advanceShallow correctly#14858
benwtrent merged 3 commits intoapache:branch_9_12from
benwtrent:bugfix/14857

Conversation

@benwtrent
Copy link
Copy Markdown
Member

closes: #14857

Copy link
Copy Markdown
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does getMaxScore need fixing as well? I see that it adds context.docBase too.

public int advanceShallow(int docid) {
if (docid == NO_MORE_DOCS) {
return NO_MORE_DOCS;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it instead be something like below?

if (docid >= context.reader.maxDoc()) { // out of range
  return NO_MORE_DOCS;
} 

Otherwise we may be computing blocks based on hits that belong to other segments?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah! Let me get that

@vigyasharma
Copy link
Copy Markdown
Contributor

does getMaxScore need fixing as well? I see that it adds context.docBase too.

I suppose this gets handled by the idx < upper check on L427, which would ensure we stop at segment boundary?

@vigyasharma
Copy link
Copy Markdown
Contributor

Sneaky bug! Thanks for fixing @benwtrent

@benwtrent
Copy link
Copy Markdown
Member Author

@jpountz what do you think if we just backport: #12146 ?

That handles the issue and greatly simplifies things. I am actually not sure why it wasn't backported to begin with.

@jpountz
Copy link
Copy Markdown
Contributor

jpountz commented Jun 28, 2025

This looks like a safe change to backport to me.

@benwtrent
Copy link
Copy Markdown
Member Author

@jpountz @vigyasharma I changed this PR to just be a backport of #12146

It fixes both maxscore & advshallow bugs while greatly simplifying the code.

@benwtrent benwtrent merged commit 16b9a87 into apache:branch_9_12 Jun 30, 2025
2 checks passed
@benwtrent benwtrent deleted the bugfix/14857 branch June 30, 2025 13:05
elasticsearchmachine pushed a commit to elastic/elasticsearch that referenced this pull request Jul 1, 2025
Basically, advanceShallow for knn queries can flip back from hitting
NO_MORE_DOCS to a valid doc ID again. This can cause search higher level
queries to flip back and forth breaking assumptions and causing a CPU
core to be locked up in-definitely (until server reboot).

Applies the fix provided here, but requires gathering score docs again:
apache/lucene#14858

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants