Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add boundary scanner and max fragment length support to unified highl… #2746

Merged
merged 4 commits into from
May 4, 2017

Conversation

Mpdreamz
Copy link
Member

@Mpdreamz Mpdreamz commented May 2, 2017

…igther as per elastic/elasticsearch#23431

@russcam russcam changed the title add boundary scanner and max fragment lenght support to unified highl… add boundary scanner and max fragment length support to unified highl… May 4, 2017
Copy link
Contributor

@russcam russcam left a comment

Choose a reason for hiding this comment

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

@@ -115,5 +148,13 @@ public class HighlightDescriptor<T> : DescriptorBase<HighlightDescriptor<T> ,IHi
public HighlightDescriptor<T> BoundaryCharacters(string boundaryCharacters) => Assign(a => a.BoundaryChars = boundaryCharacters);

public HighlightDescriptor<T> BoundaryMaxSize(int boundaryMaxSize) => Assign(a => a.BoundaryMaxSize = boundaryMaxSize);
Copy link
Contributor

Choose a reason for hiding this comment

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

I realise this is not part of the changes here, but it looks like boundary_max_size should be boundary_max_scan:

https://github.com/elastic/elasticsearch/blob/4863a9cfdf2ca5a2fdb503e5dab9372a0462cd20/core/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/AbstractHighlighterBuilder.java#L64-L64

I think it makes sense to update as part of this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

elastic/elasticsearch#1614 we're in good company of making this mistake :)

Copy link
Member Author

Choose a reason for hiding this comment

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

added _scan and obsoleted _size

@Mpdreamz
Copy link
Member Author

Mpdreamz commented May 4, 2017

Good catch @russcam added support for fragmenter as well.

@russcam
Copy link
Contributor

russcam commented May 4, 2017

++ I think fragmenter should also be added to the highlighter tests

@Mpdreamz
Copy link
Member Author

Mpdreamz commented May 4, 2017

@russcam added test and also copied the properties to IHighlightField as well.

@Mpdreamz Mpdreamz merged commit 54175c8 into 5.4 May 4, 2017
@Mpdreamz Mpdreamz mentioned this pull request May 4, 2017
@Mpdreamz
Copy link
Member Author

Mpdreamz commented May 4, 2017

ported to master

@Mpdreamz Mpdreamz deleted the feature/unified-highlighter-options branch May 4, 2017 14:15
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.

None yet

2 participants