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

Fix kuromoji default stoptags #26600

Merged
merged 2 commits into from Sep 15, 2017

Conversation

Projects
None yet
6 participants
@avdv
Copy link
Contributor

commented Sep 12, 2017

Fixes #26519

avdv added some commits Sep 12, 2017

[TEST] Fix parameter order to `assertThat` call
The order was reversed, as the expected value was given for the actual value and
vice versa. This led to a confusing assertion error message:

```
FAILURE 0.04s J1 | KuromojiAnalysisTests.testPartOfSpeechFilter <<< FAILURES!
   > Throwable #1: java.lang.AssertionError: expected different term at index 1
   > Expected: "が"
   >      but: was "おいしい"
```

when the string "が" was actually not expected.
Use default stop-tags for Kuromoji part-of-speech filter
* add new test which checks that part-of-speech tokens are removed when
  using the kuromoji_part_of_speech filter

* initialize the default stop-tags in `KuromojiPartOfSpeechFilterFactory` if
  the `stoptags` are not given in the config
@elasticmachine

This comment has been minimized.

Copy link
Collaborator

commented Sep 12, 2017

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?

@cbuescher

This comment has been minimized.

Copy link
Member

commented Sep 12, 2017

@elasticmachine test this please

@cbuescher

This comment has been minimized.

Copy link
Member

commented Sep 14, 2017

@johtani looks like you might be the right person to look at this. Please un-assign yourself if not.

@johtani
Copy link
Member

left a comment

LGTM
@cbuescher This PR is bug fix so we can cherry-pick it to 6.x, right?

@cbuescher

This comment has been minimized.

Copy link
Member

commented Sep 15, 2017

@johtani if it is a bug fix an doesn't change existing behaviour I'd even pick it to 6.0. Looks low risk to me, do you agree?

@johtani

This comment has been minimized.

Copy link
Member

commented Sep 15, 2017

@cbuescher I agree with you.

@cbuescher

This comment has been minimized.

Copy link
Member

commented Sep 15, 2017

@avdv thanks a lot for this fix, I will merge this to master and the current 6.x branches

@cbuescher cbuescher merged commit 7184cf8 into elastic:master Sep 15, 2017

2 checks passed

CLA Commit author has signed the CLA
Details
elasticsearch-ci Build finished.
Details

cbuescher added a commit that referenced this pull request Sep 15, 2017

Fix kuromoji default stoptags (#26600)
Initialize the default stop-tags in `KuromojiPartOfSpeechFilterFactory` if the
`stoptags` are not given in the config. Also adding a test which checks that 
part-of-speech tokens are removed when using the kuromoji_part_of_speech 
filter.

cbuescher added a commit that referenced this pull request Sep 15, 2017

Fix kuromoji default stoptags (#26600)
Initialize the default stop-tags in `KuromojiPartOfSpeechFilterFactory` if the
`stoptags` are not given in the config. Also adding a test which checks that 
part-of-speech tokens are removed when using the kuromoji_part_of_speech 
filter.
@avdv

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2017

thank for @cbuescher and @johtani! any chance to get this into 5.6.x, too?

@cbuescher

This comment has been minimized.

Copy link
Member

commented Sep 15, 2017

@avdv you are right, since this is a bugfix, I think we should also merge to the 5.6 branch. Will do so. Not sure though in which of the next minor releases it is going to end up.

cbuescher added a commit that referenced this pull request Sep 15, 2017

Fix kuromoji default stoptags (#26600)
Initialize the default stop-tags in `KuromojiPartOfSpeechFilterFactory` if the
`stoptags` are not given in the config. Also adding a test which checks that 
part-of-speech tokens are removed when using the kuromoji_part_of_speech 
filter.

@cbuescher cbuescher added the v5.6.2 label Sep 15, 2017

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Sep 16, 2017

Merge branch 'master' into track-global-checkpoints
* master:
  fix testSniffNodes to use the new error message
  Add check for invalid index in WildcardExpressionResolver (elastic#26409)
  Docs: Use single-node discovery.type for dev example
  Filter unsupported relation for range query builder (elastic#26620)
  Fix kuromoji default stoptags (elastic#26600)
  [Docs] Add description for missing fields in Reindex/Update/Delete By Query (elastic#26618)
  [Docs] Update ingest.asciidoc (elastic#26599)
  Better message text for ResponseException
  [DOCS] Remove edit link from ML node
  enable bwc testing
  fix StartRecoveryRequestTests.testSerialization
  Add bad_request to the rest-api-spec catch params (elastic#26539)
  Introduce a History UUID as a requirement for ops based recovery  (elastic#26577)
  Add missing catch arguments to the rest api spec (elastic#26536)

@lcawl lcawl removed the v6.1.0 label Dec 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.