Wildcard field names in highlighting should only return fields that can be highlighted #11364

brwe · 2015-05-27T06:52:58Z

When we highlight on fields using wildcards then fields might match that cannot
be highlighted by the specified highlighter. The whole search request then
failed. Instead, check that the field can be highlighted and ignore the field
if it can't.

In addition ignore the exception thrown by plain highlighter if a field conatins
terms larger than 32766.

closes #9881

javanna · 2015-05-27T06:58:27Z

src/main/java/org/elasticsearch/search/highlight/HighlightPhase.java

+                        highlightFields.put(highlightField.name(), highlightField);
+                    }
+                } catch (FetchPhaseExecutionException e) {
+                    if (fieldNameContainsWildcards && e.getCause() instanceof BytesRefHash.MaxBytesLengthExceededException) {


is this a plain highlighter only problem? Not sure, but if so maybe we could handle it in the PlainHighlighter directly rather than in the HighlightPhase?

Yes makes sense. I'll make it so that this Exception will always be caught even if no wildcards are used in the field name.

javanna · 2015-05-27T07:08:42Z

I left some comments ;)

brwe · 2015-05-27T10:39:56Z

@javanna thanks a lot for the review! addressed all comments. want to have another look?

javanna · 2015-05-27T11:46:20Z

src/main/java/org/elasticsearch/search/highlight/HighlightPhase.java

                    if (useFastVectorHighlighter) {
                        highlighterType = "fvh";
-                    } else if (fieldMapper.fieldType().indexOptions() == IndexOptions.DOCS_AND_FREQS_AND_POSITIONS_AND_OFFSETS) {
+                    } else if (highlighters.get("postings").canHighlight(fieldMapper)) {


maybe overkill, but is it worth keeping a list of highlighters ordered by precedence instead? it could even allow plugins to provide their own precedence, but that's maybe too much? I am not sure, just asking

If anyone actually needs that maybe have another issue for it?

alright, that makes sense, apart from the feature for plugins, it might still be nicer to have a sorted list and go through the available highlighters, call canHighlight and highlight using the first one that returns true? I mean rather than getting them by name one by one? Do you like this idea?

It does seem a bit cleaner to use a list.

For what its worth my highlighter would just return true from canHighlight because its defaults let it pick its hit detection strategy based on what is in the mapping.

ok, added a new commit that adds a list. is that what you meant?

javanna · 2015-05-27T11:49:31Z

looks great, left a few more comments, I think this should marked as breaking as it breaks plugins that plug in custom highlighters, maybe @nik9000 would like to have a look too?

nik9000 · 2015-05-27T12:39:08Z

looks great, left a few more comments, I think this should marked as breaking as it breaks plugins that plug in custom highlighters, maybe @nik9000 would like to have a look too?

Just did.

I think its fine. I agree with your point about a sorted list being easier to read. I also think its worth moving any concerns about highlighter plugins registering themselves to the list to another issue. At this point I don't think that is really an important feature.

nik9000 · 2015-05-27T12:55:24Z

src/main/java/org/elasticsearch/search/highlight/HighlightPhase.java

+        sortedListBuilder.add(highlighters.get("fvh"));
+        sortedListBuilder.add(highlighters.get("postings"));
+        sortedListBuilder.add(highlighters.get("plain"));
+        for (Map.Entry highlighter : highlighters.entrySet()) {


This probably isn't required - other highlighters can just be called out in the request if you want them.

agreed we can keep custom highlighters out of the loop for now, anyways they can currently be used only by referring to them with highlighter_type parameter if I remember correctly

That's right - you need to call them out explicitly.

ok, removed them again

brwe · 2015-05-27T12:56:08Z

ok, changed that now to use a list. want to take another look?

nik9000 · 2015-05-27T12:57:26Z

src/main/java/org/elasticsearch/search/highlight/HighlightPhase.java


    @Inject
    public HighlightPhase(Settings settings, Highlighters highlighters) {
        super(settings);
        this.highlighters = highlighters;
+        // TODO: would be nice if each highlighter would have a precedence and we could just sort here by this value
+        ImmutableList.Builder sortedListBuilder = ImmutableList.builder();


This is what I meant, yeah. I'd have made it a private static final ImmutableList<String> instead of Immutable<Highlighter> but it doesn't make much difference.

+1 that is what I would do too

ok, done. But not static, that seems wrong and highlighters is not static either.

brwe · 2015-05-27T13:34:32Z

all done. want to take another look?

javanna · 2015-05-27T13:49:12Z

src/main/java/org/elasticsearch/search/highlight/HighlightPhase.java


    @Inject
    public HighlightPhase(Settings settings, Highlighters highlighters) {
        super(settings);
        this.highlighters = highlighters;
+        // TODO: would be nice if each highlighter would have a precedence and we could just sort here by this value
+        // we could then also add custom highlighters to this list and they could be selected automatically depending on precendence


can you remove this TODO? I'm not sure we are going to implement this after all, nobody needs it for now :)

javanna · 2015-05-27T13:52:43Z

done another review, few more comments :)

brwe · 2015-05-27T14:19:29Z

pushed another commit

nik9000 · 2015-05-27T14:25:51Z

src/main/java/org/elasticsearch/search/highlight/FastVectorHighlighter.java

@@ -63,7 +63,7 @@ public HighlightField highlight(HighlighterContext highlighterContext) {
        FetchSubPhase.HitContext hitContext = highlighterContext.hitContext;
        FieldMapper mapper = highlighterContext.mapper;

-        if (!(mapper.fieldType().storeTermVectors() && mapper.fieldType().storeTermVectorOffsets() && mapper.fieldType().storeTermVectorPositions())) {
+        if (canHighlight(mapper) == false) {


!canHighlight(mapper) ?

the == false is done on purpose to make these comparisons more explicit

Fair enough.

I get the comment usually the other way round with the argument that '== false' is much easier to read and a '!' is easy to miss so I'd rather leave it this way.

javanna · 2015-05-27T14:28:33Z

LGTM besides the two very minor comments I left, thanks for taking care of this @brwe !

nik9000 · 2015-05-27T14:30:50Z

LGTM besides the two very minor comments I left, thanks for taking care of this @brwe !

I left another distinct minor comment but yeah, its great! This'll make highlighting so much less blowy-upy.

brwe · 2015-05-27T14:52:25Z

addressed all comments

javanna · 2015-05-27T14:56:47Z

LGTM

…d contains wildcards When we highlight on fields using wildcards then fields might match that cannot be highlighted by the specified highlighter. The whole search request then failed. Instead, check that the field can be highlighted and ignore the field if it can't. In addition ignore the exception thrown by plain highlighter if a field conatins terms larger than 32766. closes elastic#9881

Wildcard field names in highlighting should only return fields that can be highlighted

javanna reviewed May 27, 2015
View reviewed changes

brwe mentioned this pull request May 27, 2015

Ability to disable fields from default highlighting elastic/kibana#2782

Closed

clintongormley added >enhancement :Search/Highlighting How a query matched a document labels May 27, 2015

clintongormley changed the title ~~highlighting: don't fail search request when name of highlighted fiel…~~ Wildcard field names in highlighting should only return fields that can be highlighted May 27, 2015

clintongormley added the v1.6.0 label May 27, 2015

javanna reviewed May 27, 2015
View reviewed changes

brwe added the >breaking label May 27, 2015

nik9000 reviewed May 27, 2015
View reviewed changes

brwe force-pushed the highlighter-wildcard branch from 0ad1161 to 92eaf98 Compare May 27, 2015 13:26

javanna reviewed May 27, 2015
View reviewed changes

brwe force-pushed the highlighter-wildcard branch from 2d27496 to fe9810a Compare May 27, 2015 14:19

nik9000 reviewed May 27, 2015
View reviewed changes

javanna added the v2.0.0-beta1 label May 27, 2015

brwe force-pushed the highlighter-wildcard branch 2 times, most recently from a79eca8 to edcdf7a Compare May 27, 2015 14:46

brwe force-pushed the highlighter-wildcard branch from edcdf7a to 3761054 Compare May 27, 2015 15:10

brwe added a commit that referenced this pull request May 27, 2015

Merge pull request #11364 from brwe/highlighter-wildcard

ceb0782

Wildcard field names in highlighting should only return fields that can be highlighted

brwe merged commit ceb0782 into elastic:master May 27, 2015

clintongormley mentioned this pull request May 5, 2016

Invalid shift value (xx) in prefixCoded bytes (is encoded value really a geo point?) #17537

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wildcard field names in highlighting should only return fields that can be highlighted #11364

Wildcard field names in highlighting should only return fields that can be highlighted #11364

brwe commented May 27, 2015

javanna May 27, 2015

brwe May 27, 2015

javanna commented May 27, 2015

brwe commented May 27, 2015

javanna May 27, 2015

brwe May 27, 2015

javanna May 27, 2015

nik9000 May 27, 2015

brwe May 27, 2015

javanna commented May 27, 2015

nik9000 commented May 27, 2015

nik9000 May 27, 2015

javanna May 27, 2015

nik9000 May 27, 2015

brwe May 27, 2015

brwe commented May 27, 2015

nik9000 May 27, 2015

javanna May 27, 2015

brwe May 27, 2015

brwe commented May 27, 2015

javanna May 27, 2015

javanna commented May 27, 2015

brwe commented May 27, 2015

nik9000 May 27, 2015

javanna May 27, 2015

nik9000 May 27, 2015

brwe May 27, 2015

javanna commented May 27, 2015

nik9000 commented May 27, 2015

brwe commented May 27, 2015

javanna commented May 27, 2015

Wildcard field names in highlighting should only return fields that can be highlighted #11364

Wildcard field names in highlighting should only return fields that can be highlighted #11364

Conversation

brwe commented May 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented May 27, 2015

brwe commented May 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented May 27, 2015

nik9000 commented May 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brwe commented May 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brwe commented May 27, 2015

Choose a reason for hiding this comment

javanna commented May 27, 2015

brwe commented May 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javanna commented May 27, 2015

nik9000 commented May 27, 2015

brwe commented May 27, 2015

javanna commented May 27, 2015