Skip to content

Conversation

@yangfeng123456
Copy link

Summary
This PR fixes a small issue in FastVectorHighlighter where boost values were not fully preserved when overlapping phrases were merged (#15442).
The change is limited to the boost accumulation logic, and a unit test is added to cover the case.

Motivation
While investigating FastVectorHighlighter behavior with overlapping phrase queries, I noticed that part of the boost information could be lost during phrase merging.
This can lead to highlight scoring that does not fully reflect the original query boosts.
This mainly affects legacy Elasticsearch and Solr setups that still rely on FVH.

Implementation
Preserve boost values when merging overlapping WeightedPhraseInfo instances
Add a small null-safety guard around TokenGroup boost handling
Add a focused test reproducing the issue

Risks
The change is small and limited to FVH internals, and should be safe to backport to 10.3.

@yangfeng123456
Copy link
Author

Hi @dweiss @dsmiley @jainankitk ,
This PR fixes the FastVectorHighlighter boost issue (related issue: #15442) by preserving boost values when merging overlapping phrases. A unit test has been added to cover this case.
CI has passed, and the changes are small, limited to FVH internals.
I would greatly appreciate it if you could review this PR at your convenience. Thank you very much! I’ll address any feedback promptly.

Copy link
Contributor

@jainankitk jainankitk left a comment

Choose a reason for hiding this comment

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

Thanks @yangfeng123456 for this contribution. While the change itself looks fine to me, I am wondering if branch_10_3 is the right place for this. 10.4 is already being discussed, so it should be part of main and branch_10_x

@yangfeng123456
Copy link
Author

Thanks @yangfeng123456 for this contribution. While the change itself looks fine to me, I am wondering if branch_10_3 is the right place for this. 10.4 is already being discussed, so it should be part of main and branch_10_x

Thanks for the feedback! I’ll close this PR and resubmit the fix based on main as suggested.

@yangfeng123456
Copy link
Author

Thanks @yangfeng123456 for this contribution. While the change itself looks fine to me, I am wondering if branch_10_3 is the right place for this. 10.4 is already being discussed, so it should be part of main and branch_10_x

Thanks for the suggestion earlier. I’ve resubmitted the change based on main here: #15635

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.

2 participants