Skip to content

add nori_number token filter in analysis-nori#53583

Merged
jimczi merged 5 commits intoelastic:masterfrom
danmuzi:add-nori-number-filter
Mar 23, 2020
Merged

add nori_number token filter in analysis-nori#53583
jimczi merged 5 commits intoelastic:masterfrom
danmuzi:add-nori-number-filter

Conversation

@danmuzi
Copy link
Copy Markdown
Contributor

@danmuzi danmuzi commented Mar 15, 2020

The KoreanNumberFilter has included in Nori after Lucene 8.2.0. (LUCENE-8812)
However, it isn't supported now in Nori Analysis plugin. (Kuromoji supports kuromoji_number)
It seems to be missing(#30397) because KoreanNumberFilter didn't exist at Lucene 7.4.0 that supports Nori first time.
This PR is for that.

@jrodewig jrodewig added :Search Relevance/Analysis How text is split into tokens >docs General docs changes labels Mar 16, 2020
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Mar 16, 2020

Pinging @elastic/es-search (:Search/Analysis)

@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Mar 16, 2020

Pinging @elastic/es-docs (>docs)

@jimczi jimczi added >feature v7.7.0 v8.0.0 and removed >docs General docs changes labels Mar 16, 2020
jimczi
jimczi reviewed Mar 17, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

Thanks for adding this filter danmuzi ! I left one comment regarding the discard_punctuation option of the tokenizer that was added to handle the number filter correctly.

@danmuzi
Copy link
Copy Markdown
Contributor Author

danmuzi commented Mar 19, 2020

Thanks for your review, @jimczi 👍
I added some commits reflecting your comments.
But I'm not sure it's right to include the discard_punctuation option in this PR.
Because this PR is for nori_number token filter.
So I submitted commits separately.
If you think creating a new PR for discard_punctuation is better, I will write a new PR for that.
If so, I'll remove the discard_punctuation commit in this PR and rebase to master branch after discard_punctuation PR is submitted.
What do you think about this?

jimczi
jimczi reviewed Mar 19, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

I left two minor comments but the change looks good to me.

But I'm not sure it's right to include the discard_punctuation option in this PR.
Because this PR is for nori_number token filter.

I think it's ok since the discard_punctuation option was added specifically for the number token filter. Let's add both in the same pr, thanks for separating the commits though.

docs/plugins/analysis-nori.asciidoc Outdated
Copy link
Copy Markdown
Contributor

@jimczi jimczi Mar 19, 2020

Choose a reason for hiding this comment

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

Maybe add add a NOTE: to emphasize this part ?

Copy link
Copy Markdown
Contributor Author

@danmuzi danmuzi Mar 20, 2020

Choose a reason for hiding this comment

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

Done!

@danmuzi
Copy link
Copy Markdown
Contributor Author

danmuzi commented Mar 20, 2020

Thanks Jim.
I added a commit based on your review.
It's about adding NOTE in asciidoc and test cases for discard_punctuation.
Please check it!

@jimczi
Copy link
Copy Markdown
Contributor

jimczi commented Mar 23, 2020

@elasticmachine ok to test

@danmuzi danmuzi force-pushed the add-nori-number-filter branch from 3864137 to 1a4367e Compare March 23, 2020 17:08
@danmuzi
Copy link
Copy Markdown
Contributor Author

danmuzi commented Mar 23, 2020

I'm not sure why elasticsearch-ci/2 and elasticsearch-ci/bwc and elasticsearch-ci/default-distro are failed.
It doesn't seem related but I found an indentation problem in this PR.
And I rebased this patch to the latest master branch commit.

@danmuzi
Copy link
Copy Markdown
Contributor Author

danmuzi commented Mar 23, 2020

Because of the my rebase mistake, the previous Jenkins build history has been lost in this conversation.
So I attach the failure builds before the rebase.
elasticsearch-ci/2 : https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request-2/19332/
elasticsearch-ci/bwc : https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request-bwc/19021/
elasticsearch-ci/default-distro : https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request+default-distro/18876/

After the rebase, all Jenkins builds passed.

jimczi
jimczi approved these changes Mar 23, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @danmuzi !

@danmuzi
Copy link
Copy Markdown
Contributor Author

danmuzi commented Mar 23, 2020

Thanks for your kind reviews! @jimczi

@jimczi jimczi merged commit 8d4ff29 into elastic:master Mar 23, 2020
jimczi pushed a commit that referenced this pull request Mar 23, 2020
@danmuzi @jimczi
Add nori_number token filter in analysis-nori (#53583)
bc2289c 
This change adds the `nori_number` token filter.
It also adds a `discard_punctuation` option in nori_tokenizer that should be used in conjunction with the new filter.
@codebrain codebrain mentioned this pull request Apr 1, 2020
Closed
38 tasks
@codebrain codebrain mentioned this pull request Apr 15, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jimczi jimczi jimczi approved these changes

Assignees

No one assigned

Labels

>feature :Search Relevance/Analysis How text is split into tokens v7.7.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@danmuzi @elasticmachine @jimczi @jakelandis @jrodewig