Merge pull request #15376 from cbuescher/highlight-refactoring-searchSource

Use HighlightBuilder in SearchSourceBuilder

core/src/main/java/org/elasticsearch/search/SearchService.java

@@ -83,12 +83,14 @@ import org.elasticsearch.search.fetch.fielddata.FieldDataFieldsContext;
 import org.elasticsearch.search.fetch.fielddata.FieldDataFieldsContext.FieldDataField;
 import org.elasticsearch.search.fetch.fielddata.FieldDataFieldsFetchSubPhase;
 import org.elasticsearch.search.fetch.script.ScriptFieldsContext.ScriptField;
+import org.elasticsearch.search.highlight.HighlightBuilder;
 import org.elasticsearch.search.internal.*;
 import org.elasticsearch.search.internal.SearchContext.Lifetime;
 import org.elasticsearch.search.query.*;
 import org.elasticsearch.search.warmer.IndexWarmersMetaData;
 import org.elasticsearch.threadpool.ThreadPool;
 
+import java.io.IOException;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.concurrent.CountDownLatch;
@@ -656,7 +658,7 @@ public class SearchService extends AbstractLifecycleComponent<SearchService> imp
         }
     }
 
-    private void parseSource(SearchContext context, SearchSourceBuilder source) throws SearchParseException {
+    private void parseSource(SearchContext context, SearchSourceBuilder source) throws SearchContextException {
         // nothing to parse...
         if (source == null) {
             return;
@@ -807,19 +809,11 @@ public class SearchService extends AbstractLifecycleComponent<SearchService> imp
             fieldDataFieldsContext.setHitExecutionNeeded(true);
         }
         if (source.highlighter() != null) {
-            XContentParser highlighterParser = null;
+            HighlightBuilder highlightBuilder = source.highlighter();
             try {
-                highlighterParser = XContentFactory.xContent(source.highlighter()).createParser(source.highlighter());
+                context.highlight(highlightBuilder.build(context.indexShard().getQueryShardContext()));
-                this.elementParsers.get("highlight").parse(highlighterParser, context);
+            } catch (IOException e) {
-            } catch (Exception e) {
+                throw new SearchContextException(context, "failed to create SearchContextHighlighter", e);
-                String sSource = "_na_";
-                try {
-                    sSource = source.toString();
-                } catch (Throwable e1) {
-                    // ignore
-                }
-                XContentLocation location = highlighterParser != null ? highlighterParser.getTokenLocation() : null;
-                throw new SearchParseException(context, "failed to parse suggest source [" + sSource + "]", location, e);
             }
         }
         if (source.innerHits() != null) {

core/src/main/java/org/elasticsearch/search/aggregations/metrics/tophits/TopHitsBuilder.java

@@ -19,7 +19,6 @@
 package org.elasticsearch.search.aggregations.metrics.tophits;
 
 import org.elasticsearch.common.Nullable;
-import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.script.Script;
 import org.elasticsearch.search.aggregations.AbstractAggregationBuilder;
@@ -194,7 +193,7 @@ public class TopHitsBuilder extends AbstractAggregationBuilder {
         return sourceBuilder;
     }
 
-    public BytesReference highlighter() {
+    public HighlightBuilder highlighter() {
         return sourceBuilder().highlighter();
     }
 

core/src/main/java/org/elasticsearch/search/builder/SearchSourceBuilder.java

@@ -144,7 +144,7 @@ public final class SearchSourceBuilder extends ToXContentToBytes implements Writ
 
     private List<BytesReference> aggregations;
 
-    private BytesReference highlightBuilder;
+    private HighlightBuilder highlightBuilder;
 
     private BytesReference suggestBuilder;
 
@@ -405,22 +405,14 @@ public final class SearchSourceBuilder extends ToXContentToBytes implements Writ
      * Adds highlight to perform as part of the search.
      */
     public SearchSourceBuilder highlighter(HighlightBuilder highlightBuilder) {
-        try {
+        this.highlightBuilder = highlightBuilder;
-            XContentBuilder builder = XContentFactory.jsonBuilder();
+        return this;
-            builder.startObject();
-            highlightBuilder.innerXContent(builder);
-            builder.endObject();
-            this.highlightBuilder = builder.bytes();
-            return this;
-        } catch (IOException e) {
-            throw new RuntimeException(e);
-        }
     }
 
     /**
-     * Gets the bytes representing the hightlighter builder for this request.
+     * Gets the hightlighter builder for this request.
      */
-    public BytesReference highlighter() {
+    public HighlightBuilder highlighter() {
         return highlightBuilder;
     }
 
@@ -813,8 +805,7 @@ public final class SearchSourceBuilder extends ToXContentToBytes implements Writ
                     }
                     builder.aggregations = aggregations;
                 } else if (context.parseFieldMatcher().match(currentFieldName, HIGHLIGHT_FIELD)) {
-                    XContentBuilder xContentBuilder = XContentFactory.jsonBuilder().copyCurrentStructure(parser);
+                    builder.highlightBuilder = HighlightBuilder.PROTOTYPE.fromXContent(context);
-                    builder.highlightBuilder = xContentBuilder.bytes();
                 } else if (context.parseFieldMatcher().match(currentFieldName, INNER_HITS_FIELD)) {
                     XContentBuilder xContentBuilder = XContentFactory.jsonBuilder().copyCurrentStructure(parser);
                     builder.innerHitsBuilder = xContentBuilder.bytes();
@@ -1012,10 +1003,7 @@ public final class SearchSourceBuilder extends ToXContentToBytes implements Writ
         }
 
         if (highlightBuilder != null) {
-            builder.field(HIGHLIGHT_FIELD.getPreferredName());
+            this.highlightBuilder.toXContent(builder, params);
-            XContentParser parser = XContentFactory.xContent(XContentType.JSON).createParser(highlightBuilder);
-            parser.nextToken();
-            builder.copyCurrentStructure(parser);
         }
 
         if (innerHitsBuilder != null) {
@@ -1158,7 +1146,7 @@ public final class SearchSourceBuilder extends ToXContentToBytes implements Writ
         }
         builder.from = in.readVInt();
         if (in.readBoolean()) {
-            builder.highlightBuilder = in.readBytesReference();
+            builder.highlightBuilder = HighlightBuilder.PROTOTYPE.readFrom(in);
         }
         boolean hasIndexBoost = in.readBoolean();
         if (hasIndexBoost) {
@@ -1259,7 +1247,7 @@ public final class SearchSourceBuilder extends ToXContentToBytes implements Writ
         boolean hasHighlightBuilder = highlightBuilder != null;
         out.writeBoolean(hasHighlightBuilder);
         if (hasHighlightBuilder) {
-            out.writeBytesReference(highlightBuilder);
+            highlightBuilder.writeTo(out);
         }
         boolean hasIndexBoost = indexBoost != null;
         out.writeBoolean(hasIndexBoost);

core/src/main/java/org/elasticsearch/search/fetch/innerhits/InnerHitsBuilder.java

@@ -20,7 +20,6 @@
 package org.elasticsearch.search.fetch.innerhits;
 
 import org.elasticsearch.common.Nullable;
-import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.xcontent.ToXContent;
 import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.index.query.QueryBuilder;
@@ -266,7 +265,7 @@ public class InnerHitsBuilder implements ToXContent {
             return this;
         }
 
-        public BytesReference highlighter() {
+        public HighlightBuilder highlighter() {
             return sourceBuilder().highlighter();
         }
 

core/src/main/java/org/elasticsearch/search/highlight/AbstractHighlighterBuilder.java

@@ -29,6 +29,8 @@ import org.elasticsearch.common.xcontent.XContentBuilder;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.index.query.QueryBuilder;
 import org.elasticsearch.index.query.QueryParseContext;
+import org.elasticsearch.search.highlight.HighlightBuilder.Order;
+
 import java.io.IOException;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -78,7 +80,7 @@ public abstract class AbstractHighlighterBuilder<HB extends AbstractHighlighterB
 
     protected QueryBuilder<?> highlightQuery;
 
-    protected String order;
+    protected Order order;
 
     protected Boolean highlightFilter;
 
@@ -217,18 +219,26 @@ public abstract class AbstractHighlighterBuilder<HB extends AbstractHighlighterB
     /**
      * The order of fragments per field. By default, ordered by the order in the
      * highlighted text. Can be <tt>score</tt>, which then it will be ordered
-     * by score of the fragments.
+     * by score of the fragments, or <tt>none</TT>.
+     */
+    public HB order(String order) {
+        return order(Order.fromString(order));
+    }
+
+    /**
+     * By default, fragments of a field are ordered by the order in the highlighted text.
+     * If set to {@link Order#SCORE}, this changes order to score of the fragments.
      */
     @SuppressWarnings("unchecked")
-    public HB order(String order) {
+    public HB order(Order scoreOrdered) {
-        this.order = order;
+        this.order = scoreOrdered;
         return (HB) this;
     }
 
     /**
-     * @return the value set by {@link #order(String)}
+     * @return the value set by {@link #order(Order)}
      */
-    public String order() {
+    public Order order() {
         return this.order;
     }
 
@@ -395,7 +405,7 @@ public abstract class AbstractHighlighterBuilder<HB extends AbstractHighlighterB
             builder.field(HIGHLIGHT_QUERY_FIELD.getPreferredName(), highlightQuery);
         }
         if (order != null) {
-            builder.field(ORDER_FIELD.getPreferredName(), order);
+            builder.field(ORDER_FIELD.getPreferredName(), order.toString());
         }
         if (highlightFilter != null) {
             builder.field(HIGHLIGHT_FILTER_FIELD.getPreferredName(), highlightFilter);
@@ -458,7 +468,7 @@ public abstract class AbstractHighlighterBuilder<HB extends AbstractHighlighterB
                 }
             } else if (token.isValue()) {
                 if (parseContext.parseFieldMatcher().match(currentFieldName, ORDER_FIELD)) {
-                    highlightBuilder.order(parser.text());
+                    highlightBuilder.order(Order.fromString(parser.text()));
                 } else if (parseContext.parseFieldMatcher().match(currentFieldName, HIGHLIGHT_FILTER_FIELD)) {
                     highlightBuilder.highlightFilter(parser.booleanValue());
                 } else if (parseContext.parseFieldMatcher().match(currentFieldName, FRAGMENT_SIZE_FIELD)) {
@@ -578,7 +588,9 @@ public abstract class AbstractHighlighterBuilder<HB extends AbstractHighlighterB
         if (in.readBoolean()) {
             highlightQuery(in.readQuery());
         }
-        order(in.readOptionalString());
+        if (in.readBoolean()) {
+            order(Order.PROTOTYPE.readFrom(in));
+        }
         highlightFilter(in.readOptionalBoolean());
         forceSource(in.readOptionalBoolean());
         boundaryMaxScan(in.readOptionalVInt());
@@ -609,7 +621,11 @@ public abstract class AbstractHighlighterBuilder<HB extends AbstractHighlighterB
         if (hasQuery) {
             out.writeQuery(highlightQuery);
         }
-        out.writeOptionalString(order);
+        boolean hasSetOrder = order != null;
+        out.writeBoolean(hasSetOrder);
+        if (hasSetOrder) {
+            order.writeTo(out);
+        }
         out.writeOptionalBoolean(highlightFilter);
         out.writeOptionalBoolean(forceSource);
         out.writeOptionalVInt(boundaryMaxScan);

core/src/main/java/org/elasticsearch/search/highlight/HighlightBuilder.java

@@ -44,6 +44,7 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Locale;
 import java.util.Objects;
 import java.util.Set;
 
@@ -308,9 +309,15 @@ public class HighlightBuilder extends AbstractHighlighterBuilder<HighlightBuilde
      */
     @SuppressWarnings({ "rawtypes", "unchecked" })
     private static void transferOptions(AbstractHighlighterBuilder highlighterBuilder, SearchContextHighlight.FieldOptions.Builder targetOptionsBuilder, QueryShardContext context) throws IOException {
-        targetOptionsBuilder.preTags(highlighterBuilder.preTags);
+        if (highlighterBuilder.preTags != null) {
-        targetOptionsBuilder.postTags(highlighterBuilder.postTags);
+            targetOptionsBuilder.preTags(highlighterBuilder.preTags);
-        targetOptionsBuilder.scoreOrdered("score".equals(highlighterBuilder.order));
+        }
+        if (highlighterBuilder.postTags != null) {
+            targetOptionsBuilder.postTags(highlighterBuilder.postTags);
+        }
+        if (highlighterBuilder.order != null) {
+            targetOptionsBuilder.scoreOrdered(highlighterBuilder.order == Order.SCORE);
+        }
         if (highlighterBuilder.highlightFilter != null) {
             targetOptionsBuilder.highlightFilter(highlighterBuilder.highlightFilter);
         }
@@ -326,9 +333,15 @@ public class HighlightBuilder extends AbstractHighlighterBuilder<HighlightBuilde
         if (highlighterBuilder.boundaryMaxScan != null) {
             targetOptionsBuilder.boundaryMaxScan(highlighterBuilder.boundaryMaxScan);
         }
-        targetOptionsBuilder.boundaryChars(convertCharArray(highlighterBuilder.boundaryChars));
+        if (highlighterBuilder.boundaryChars != null) {
-        targetOptionsBuilder.highlighterType(highlighterBuilder.highlighterType);
+            targetOptionsBuilder.boundaryChars(convertCharArray(highlighterBuilder.boundaryChars));
-        targetOptionsBuilder.fragmenter(highlighterBuilder.fragmenter);
+        }
+        if (highlighterBuilder.highlighterType != null) {
+            targetOptionsBuilder.highlighterType(highlighterBuilder.highlighterType);
+        }
+        if (highlighterBuilder.fragmenter != null) {
+            targetOptionsBuilder.fragmenter(highlighterBuilder.fragmenter);
+        }
         if (highlighterBuilder.noMatchSize != null) {
             targetOptionsBuilder.noMatchSize(highlighterBuilder.noMatchSize);
         }
@@ -338,7 +351,9 @@ public class HighlightBuilder extends AbstractHighlighterBuilder<HighlightBuilde
         if (highlighterBuilder.phraseLimit != null) {
             targetOptionsBuilder.phraseLimit(highlighterBuilder.phraseLimit);
         }
-        targetOptionsBuilder.options(highlighterBuilder.options);
+        if (highlighterBuilder.options != null) {
+            targetOptionsBuilder.options(highlighterBuilder.options);
+        }
         if (highlighterBuilder.highlightQuery != null) {
             targetOptionsBuilder.highlightQuery(highlighterBuilder.highlightQuery.toQuery(context));
         }
@@ -545,4 +560,36 @@ public class HighlightBuilder extends AbstractHighlighterBuilder<HighlightBuilde
             writeOptionsTo(out);
         }
     }
+
+    public enum Order implements Writeable<Order> {
+        NONE, SCORE;
+
+        static Order PROTOTYPE = NONE;
+
+        @Override
+        public Order readFrom(StreamInput in) throws IOException {
+            int ordinal = in.readVInt();
+            if (ordinal < 0 || ordinal >= values().length) {
+                throw new IOException("Unknown Order ordinal [" + ordinal + "]");
+            }
+            return values()[ordinal];
+        }
+
+        @Override
+        public void writeTo(StreamOutput out) throws IOException {
+            out.writeVInt(this.ordinal());
+        }
+
+        public static Order fromString(String order) {
+            if (order.toUpperCase(Locale.ROOT).equals(SCORE.name())) {
+                return Order.SCORE;
+            }
+            return NONE;
+        }
+
+        @Override
+        public String toString() {
+            return name().toLowerCase(Locale.ROOT);
+        }
+    }
 }

core/src/test/java/org/elasticsearch/search/builder/SearchSourceBuilderTests.java

@@ -34,7 +34,11 @@ import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.settings.SettingsFilter;
 import org.elasticsearch.common.settings.SettingsModule;
 import org.elasticsearch.common.unit.TimeValue;
-import org.elasticsearch.common.xcontent.*;
+import org.elasticsearch.common.xcontent.ToXContent;
+import org.elasticsearch.common.xcontent.XContentBuilder;
+import org.elasticsearch.common.xcontent.XContentFactory;
+import org.elasticsearch.common.xcontent.XContentParser;
+import org.elasticsearch.common.xcontent.XContentType;
 import org.elasticsearch.index.query.AbstractQueryTestCase;
 import org.elasticsearch.index.query.EmptyQueryBuilder;
 import org.elasticsearch.index.query.QueryBuilders;
@@ -47,7 +51,7 @@ import org.elasticsearch.search.aggregations.AggregationBuilders;
 import org.elasticsearch.search.fetch.innerhits.InnerHitsBuilder;
 import org.elasticsearch.search.fetch.innerhits.InnerHitsBuilder.InnerHit;
 import org.elasticsearch.search.fetch.source.FetchSourceContext;
-import org.elasticsearch.search.highlight.HighlightBuilder;
+import org.elasticsearch.search.highlight.HighlightBuilderTests;
 import org.elasticsearch.search.rescore.RescoreBuilder;
 import org.elasticsearch.search.sort.SortBuilders;
 import org.elasticsearch.search.sort.SortOrder;
@@ -251,8 +255,7 @@ public class SearchSourceBuilderTests extends ESTestCase {
             }
         }
         if (randomBoolean()) {
-            // NORELEASE need a random highlight builder method
+            builder.highlighter(HighlightBuilderTests.randomHighlighterBuilder());
-            builder.highlighter(new HighlightBuilder().field(randomAsciiOfLengthBetween(5, 20)));
         }
         if (randomBoolean()) {
             // NORELEASE need a random suggest builder method

core/src/test/java/org/elasticsearch/search/highlight/HighlightBuilderTests.java

@@ -53,6 +53,7 @@ import org.elasticsearch.index.query.TermQueryBuilder;
 import org.elasticsearch.index.query.TermQueryParser;
 import org.elasticsearch.indices.query.IndicesQueriesRegistry;
 import org.elasticsearch.search.highlight.HighlightBuilder.Field;
+import org.elasticsearch.search.highlight.HighlightBuilder.Order;
 import org.elasticsearch.search.highlight.SearchContextHighlight.FieldOptions;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.IndexSettingsModule;
@@ -148,7 +149,6 @@ public class HighlightBuilderTests extends ESTestCase {
         context.parseFieldMatcher(new ParseFieldMatcher(Settings.EMPTY));
         for (int runs = 0; runs < NUMBER_OF_TESTBUILDERS; runs++) {
             HighlightBuilder highlightBuilder = randomHighlighterBuilder();
-            System.out.println(highlightBuilder);
             XContentBuilder builder = XContentFactory.contentBuilder(randomFrom(XContentType.values()));
             if (randomBoolean()) {
                 builder.prettyPrint();
@@ -416,6 +416,30 @@ public class HighlightBuilderTests extends ESTestCase {
         System.out.println(Math.log(1/(double)(1+1)) + 1.0);
     }
 
+    /**
+     * test ordinals of {@link Order}, since serialization depends on it
+     */
+    public void testValidOrderOrdinals() {
+        assertThat(Order.NONE.ordinal(), equalTo(0));
+        assertThat(Order.SCORE.ordinal(), equalTo(1));
+    }
+
+    public void testOrderSerialization() throws Exception {
+        try (BytesStreamOutput out = new BytesStreamOutput()) {
+            Order.NONE.writeTo(out);
+            try (StreamInput in = StreamInput.wrap(out.bytes())) {
+                assertThat(in.readVInt(), equalTo(0));
+            }
+        }
+
+        try (BytesStreamOutput out = new BytesStreamOutput()) {
+            Order.SCORE.writeTo(out);
+            try (StreamInput in = StreamInput.wrap(out.bytes())) {
+                assertThat(in.readVInt(), equalTo(1));
+            }
+        }
+    }
+
     protected static XContentBuilder toXContent(HighlightBuilder highlight, XContentType contentType) throws IOException {
         XContentBuilder builder = XContentFactory.contentBuilder(contentType);
         if (randomBoolean()) {
@@ -426,9 +450,9 @@ public class HighlightBuilderTests extends ESTestCase {
     }
 
     /**
-     * create random shape that is put under test
+     * create random highlight builder that is put under test
      */
-    private static HighlightBuilder randomHighlighterBuilder() {
+    public static HighlightBuilder randomHighlighterBuilder() {
         HighlightBuilder testHighlighter = new HighlightBuilder();
         setRandomCommonOptions(testHighlighter);
         testHighlighter.useExplicitFieldOrder(randomBoolean());
@@ -487,7 +511,12 @@ public class HighlightBuilderTests extends ESTestCase {
             highlightBuilder.highlightQuery(highlightQuery);
         }
         if (randomBoolean()) {
-            highlightBuilder.order(randomAsciiOfLengthBetween(1, 10));
+            if (randomBoolean()) {
+                highlightBuilder.order(randomFrom(Order.values()));
+            } else {
+                // also test the string setter
+                highlightBuilder.order(randomFrom(Order.values()).toString());
+            }
         }
         if (randomBoolean()) {
             highlightBuilder.highlightFilter(randomBoolean());
@@ -556,7 +585,11 @@ public class HighlightBuilderTests extends ESTestCase {
             highlightBuilder.highlightQuery(new TermQueryBuilder(randomAsciiOfLengthBetween(11, 20), randomAsciiOfLengthBetween(11, 20)));
             break;
         case 8:
-            highlightBuilder.order(randomAsciiOfLengthBetween(11, 20));
+            if (highlightBuilder.order() == Order.NONE) {
+                highlightBuilder.order(Order.SCORE);
+            } else {
+                highlightBuilder.order(Order.NONE);
+            }
             break;
         case 9:
             highlightBuilder.highlightFilter(toggleOrSet(highlightBuilder.highlightFilter()));