Add BreakIteratorBoundaryScanner support by shaie · Pull Request #23248 · elastic/elasticsearch

shaie · 2017-02-19T10:28:28Z

This commit adds a boundary_scanner property to the search highlight
request (defaults to "simple", per current behavior) so the user can
specify "break_iterator" and its type ("sentence", "word", "line" and
"character"). Also adds "boundary_scanner_locale" to define which one
should be used when highlighting the text.

This commit adds a boundary_scanner property to the search highlight request (defaults to "simple", per current behavior) so the user can specify "break_iterator" and its type ("sentence", "word", "line" and "character"). Also adds "boundary_scanner_locale" to define which one should be used when highlighting the text.

elasticmachine · 2017-02-19T10:28:29Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

jimczi

Thanks @shaie
The change looks good, I left some comments regarding the BWC. Also the docs are missing. Can you add a section in the docs explaining how these new options can be used and their limitations ?

jimczi · 2017-02-21T08:46:09Z

.../main/java/org/elasticsearch/search/fetch/subphase/highlight/AbstractHighlighterBuilder.java

        order(in.readOptionalWriteable(Order::readFromStream));
        highlightFilter(in.readOptionalBoolean());
        forceSource(in.readOptionalBoolean());
+        boundaryScannerType(in.readOptionalWriteable(BoundaryScannerType::readFromStream));


Since this PR can go to 5.x can you add a check on the version, like this:

if (in.getVersion().onOrAfter(Version.V_5_4_0_UNRELEASED)) { boundaryScannerType(in.readOptionalWriteable(BoundaryScannerType::readFromStream)); }

This will ensure that a node in 5.4 can safely send request to a 5.3 node.

Nice, I didn't see this pattern in the files I changed, so had no idea :). I will do that in all 4 places you commented on.

jimczi · 2017-02-21T08:46:18Z

.../main/java/org/elasticsearch/search/fetch/subphase/highlight/AbstractHighlighterBuilder.java

        if (in.readBoolean()) {
            boundaryChars(in.readString().toCharArray());
        }
+        boundaryScannerLocale(in.readOptionalString());


jimczi · 2017-02-21T08:46:27Z

.../main/java/org/elasticsearch/search/fetch/subphase/highlight/AbstractHighlighterBuilder.java

        out.writeOptionalWriteable(order);
        out.writeOptionalBoolean(highlightFilter);
        out.writeOptionalBoolean(forceSource);
+        out.writeOptionalWriteable(boundaryScannerType);


and here ...

jimczi · 2017-02-21T08:46:38Z

.../main/java/org/elasticsearch/search/fetch/subphase/highlight/AbstractHighlighterBuilder.java

+        out.writeBoolean(hasBoundaryScannerLocale);
+        if (hasBoundaryScannerLocale) {
+            out.writeString(boundaryScannerLocale.toLanguageTag());
+        }


and the last one ;)

jimczi · 2017-02-21T08:55:48Z

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

    }
+
+    public static enum BoundaryScannerType implements Writeable {
+        SIMPLE, BREAK_ITERATOR;


BREAK_ITERATOR is not reflecting what this type is doing. Maybe just SENTENCE ? Or SENTENCE_BREAK ?

This is actually exactly what it means. As I wrote in the PR comment, I wanted to get initial feedback. Now that I see you're OK with the general approach, I want to add a "break_iterator_type" property which defaults to "SENTENCE" but also supports "WORD", "CHARACTER" and "LINE".

Got it thanks. Though we could avoid creating another option and merge the logic in a single one ? The boundary scanner type could be any of:

simple

sentence

word

The fact that we use a break iterator for sentence and word is just an implementation detail ?
Although "CHARACTER" seems weird, should we really expose it ?

I don't think we have to expose all options, was just thinking "if Lucene supports it ..". But having said that, from an API perspective this may be weird, so I'll take your proposal and add "boundary_scanner" with values "simple", "sentence" and "word". I'll do that.

jimczi · 2017-02-21T08:57:23Z

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

+            if (boundaryScannerType.toUpperCase(Locale.ROOT).equals(BREAK_ITERATOR.name())) {
+                return BoundaryScannerType.BREAK_ITERATOR;
+            }
+            return SIMPLE;


I think it's too lenient, we should fail if the type is unknown.

I followed the same approach as in Order though I agree with you. Just thought this is how ES prefers to do things. I can simplify the method to use valueOf(boundaryScannerType.toUpperCase(Locale.ROOT))

shaie · 2017-02-22T19:27:22Z

@jimczi I believe I've addressed all your comments. Hope I addressed the BW ones as you intended!

shaie · 2017-02-22T20:33:11Z

@jimczi I only now noticed your comment about adding something to the docs. Can you point me to where that is?

jimczi · 2017-02-22T21:47:21Z

Thanks @shaie ! You can check the current docs for the FVH here: https://github.com/elastic/elasticsearch/blob/master/docs/reference/search/request/highlighting.asciidoc#fast-vector-highlighter
I'll review your changes shortly.

shaie · 2017-02-23T13:37:31Z

@jimczi I updated the docs and also renamed SIMPLE to CHARS, which I think makes more sense?

jimczi

Looks great @shaie !
I agree chars for the boundary scanner type makes more sense.
I'll check that the build is ok and that the tests pass.

jimczi · 2017-02-23T14:00:28Z

@elasticmachine please test it

shaie · 2017-02-23T14:56:29Z

@jimczi, the build failed but seems weird exception, something about Git plugin ..

jimczi · 2017-02-23T15:16:53Z

Yes it's just a problem with our CI. I'll test locally :(

shaie · 2017-02-23T16:07:05Z

K thanks @jimczi. Let me know if there are any issues, though I did run some tests locally and they passed (ones I thought are related to the change).

elasticmachine · 2017-02-23T18:12:31Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

jimczi · 2017-02-23T18:18:45Z

.../main/java/org/elasticsearch/search/fetch/subphase/highlight/AbstractHighlighterBuilder.java

+            boolean hasBoundaryScannerLocale = boundaryScannerLocale != null;
+            out.writeBoolean(hasBoundaryScannerLocale);
+            if (hasBoundaryScannerLocale) {
+                out.writeString(boundaryScannerLocale.toLanguageTag());


out.writeOptionalString() since you use in.readOptionalString in the readFrom ?

No, notice that since boundaryScannerLocale is a Locale, I first write a boolean, and then write the String if it's not null. If I wanted to use writeOptionalString I would need to do something like:

out.writeOptionalString(boundaryScannerLocale == null ? null : boundaryScannerLocale.toLanguageTag())

Do you think that's preferred?

Just for reference @jimczi, I borrowed this pattern from the code just above those lines which handles boundaryChars:

boolean hasBounaryChars = boundaryChars != null; out.writeBoolean(hasBounaryChars); if (hasBounaryChars) { out.writeString(String.valueOf(boundaryChars)); }

So before I change the code I wrote, does this change your opinion? not that I mind changing the code, just prefer the code to be consistent with the rest.

I don't mind as long as we use writeString/readString and writeOptionalString/readOptionalString consistently.
So you can maybe just change the readFrom to explicitly use readBoolean.

jimczi · 2017-02-23T18:19:39Z

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

        }
    }
+
+    public static enum BoundaryScannerType implements Writeable {


this makes our code checker unhappy since enum are always static in this context.

Good point!

jimczi

The build fails for me. I've left some comments regarding the failing codes. You can test your change locally with gradle clean check to make sure that the build is ok.

jimczi · 2017-02-23T18:20:27Z

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

+            if (randomBoolean()) {
+                highlightBuilder.boundaryScannerType(randomFrom(BoundaryScannerType.values()));
+            } else {
+                // also test the string setter


this fails the build, the toString is not correctly applied

Right, fixed.

jimczi · 2017-02-23T18:21:18Z

.../src/main/java/org/elasticsearch/search/fetch/subphase/highlight/SearchContextHighlight.java

                }
+                if (fieldOptions.boundaryScannerType == null) {
+                    fieldOptions.boundaryScannerType = globalOptions.boundaryScannerType;
+                }


The merge of the boundaryScannerLocale is missing

Sorry about that. Fixed.

shaie · 2017-02-23T20:11:34Z

I'm running gradle clean check and will push a new commit to the PR once it finishes. Sorry for the noise @jimczi

shaie · 2017-02-23T21:08:41Z

@jimczi I pushed a commit which addresses all your comments and all tests pass. Actually, when running gradle clean check only one test failed: org.elasticsearch.client.transport.NodeDisconnectIT.testNotifyOnDisconnect, but it doesn't look related to this PR and also the failures looked like:

 1> Caused by: org.elasticsearch.transport.NodeNotConnectedException: [node_s0][127.0.0.1:9500] Node not connected
  1>    at org.elasticsearch.transport.TcpTransport.getConnection(TcpTransport.java:594) ~[main/:?]
  1>    at org.elasticsearch.transport.TcpTransport.getConnection(TcpTransport.java:114) ~[main/:?]
  1>    at org.elasticsearch.transport.TransportService.getConnection(TransportService.java:509) ~[main/:?]
  1>    at org.elasticsearch.transport.TransportService.sendRequest(TransportService.java:485) ~[main/:?]
  1>    ... 19 more

I can rerun, but if you want to start the tests too, wanted to let you know.

jimczi · 2017-02-23T22:32:44Z

Thanks @shaie !
I'll merge to 5.x now.

This commit adds a boundary_scanner property to the search highlight request so the user can specify different boundary scanners: * `chars` (default, current behavior) * `word` Use a WordBreakIterator * `sentence` Use a SentenceBreakIterator This commit also adds "boundary_scanner_locale" to define which locale should be used when scanning the text.

shaie · 2017-02-24T08:32:08Z

Thanks @jimczi!

* master: (26 commits) CLI: Fix prompting for yes/no to handle console returning null (elastic#23320) Tests: Fix reproduce line for packagingTest (elastic#23365) Build: Remove extra copies of netty license (elastic#23361) [TEST] Removes timeout based wait_for_active_shards REST test (elastic#23360) [TEST] increase timeout slightly in wait_for_active_shards test to allow for index creation cluster state update to be processed before ensuring the wait times out Handle snapshot repository's missing index.latest Adding equals/hashCode to MainResponse (elastic#23352) Always restore the ThreadContext for operations delayed due to a block (elastic#23349) Add support for named xcontent parsers to high level REST client (elastic#23328) Add unit tests for ParentToChildAggregator (elastic#23305) Fix after last merge with master and apply last comments [INGEST] Lazy load the geoip databases. disable BWC tests for the highlighters, need a new 5.x build to make it work Expose WordDelimiterGraphTokenFilter (elastic#23327) Test that buildCredentials returns correct clazz (elastic#23334) Add BreakIteratorBoundaryScanner support for FVH (elastic#23248) Prioritize listing index-N blobs over index.latest in reading snapshots (elastic#23333) Test: Fix hdfs test fixture setup on windows delete and index tests can share some part of the code Remove createSampleDocument method and use the sync'ed index method ...

jimczi self-requested a review February 20, 2017 11:09

clintongormley added :Search Relevance/Highlighting How a query matched a document >enhancement labels Feb 20, 2017

jimczi requested changes Feb 21, 2017

View reviewed changes

Fix per review comments

4edf12c

Update highlighting.asciidoc and rename SIMPLE to CHARS

54f84a8

jimczi added v5.4.0 v6.0.0-alpha1 labels Feb 23, 2017

jimczi approved these changes Feb 23, 2017

View reviewed changes

jimczi reviewed Feb 23, 2017

View reviewed changes

jimczi requested changes Feb 23, 2017

View reviewed changes

Address more review comments and test failures

563ad72

jimczi approved these changes Feb 23, 2017

View reviewed changes

jimczi merged commit eeac6d2 into elastic:master Feb 23, 2017

shaie deleted the break-iterator-scanner branch February 24, 2017 08:31

Conversation

shaie commented Feb 19, 2017

Uh oh!

elasticmachine commented Feb 19, 2017

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimczi Feb 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shaie commented Feb 22, 2017

Uh oh!

shaie commented Feb 22, 2017

Uh oh!

jimczi commented Feb 22, 2017

Uh oh!

shaie commented Feb 23, 2017

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

jimczi commented Feb 23, 2017

Uh oh!

shaie commented Feb 23, 2017

Uh oh!

jimczi commented Feb 23, 2017

Uh oh!

shaie commented Feb 23, 2017

Uh oh!

elasticmachine commented Feb 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimczi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jimczi Feb 21, 2017 •

edited

Loading

jimczi commented Feb 23, 2017 •

edited

Loading