Skip to content

add phone number analysis plugin#609

Merged
dblock merged 1 commit intoopensearch-project:mainfrom
rursprung:add-spec-for-opensearch-phone-plugin
Nov 14, 2024
Merged

add phone number analysis plugin#609
dblock merged 1 commit intoopensearch-project:mainfrom
rursprung:add-spec-for-opensearch-phone-plugin

Conversation

@rursprung
Copy link
Copy Markdown
Contributor

@rursprung rursprung commented Oct 11, 2024 •
edited
Loading

Description

this is part of opensearch-project/OpenSearch#11326. the actual implementation was done opensearch-project/OpenSearch#15915. see the commit message on the PR for further details.

the new test group analysis has been added so that it can later be extended with all other optional language analyzers (which are currently also not covered).

Issues Resolved

n/a

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

dblock
dblock reviewed Oct 11, 2024
View reviewed changes
Comment thread tests/plugins/analysis/plugins/phone.yaml Outdated
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Oct 11, 2024 •
edited
Loading

Changes Analysis

Commit SHA: 780b653
Comparing To SHA: 6bb1fed

API Changes

Summary

└─┬Components
  ├──[➕] schemas (38207:7)
  └─┬_common.analysis___Analyzer
    ├──[➕] oneOf (38513:7)
    └─┬ONEOF
      └──[🔀] $ref (38207:13)❌
Document Element Total Changes Breaking Changes
components 3 1
  • ❌ BREAKING Changes: 1 out of 3
  • Modifications: 1
  • Additions: 2
  • Breaking Modifications: 1

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/11834286056/artifacts/2186592205

API Coverage

Before After Δ
Covered (%) 588 (57.59 %) 588 (57.59 %) 0 (0 %)
Uncovered (%) 433 (42.41 %) 433 (42.41 %) 0 (0 %)
Unknown 42 42 0

@dblock
Copy link
Copy Markdown
Member

dblock commented Oct 11, 2024

Looks great! Needs CHANGELOG, and a passing build (plugin installation likely needs work, ./bin/opensearch-plugin install -b analysis-phonenumber is failing).

dblock
dblock reviewed Oct 11, 2024
View reviewed changes
Comment thread tests/plugins/analysis/plugins/phone.yaml
@rursprung
Copy link
Copy Markdown
Contributor Author

rursprung commented Oct 11, 2024

i've updated the path & added the changelog. i'm keeping this as a draft until this TODO is resolved:

TODO: the plugin is not yet released => installation fails. is there a snapshot somewhere which can be used instead?

acc. to @dblock the plugin should be part of the docker image (but doesn't seem to be as it tries to fetch it from the web)

@dblock
Copy link
Copy Markdown
Member

dblock commented Oct 11, 2024

i've updated the path & added the changelog. i'm keeping this as a draft until this TODO is resolved:

TODO: the plugin is not yet released => installation fails. is there a snapshot somewhere which can be used instead?

acc. to @dblock the plugin should be part of the docker image (but doesn't seem to be as it tries to fetch it from the web)

I can confirm that anlaysis-phone is not in the docker image.

[opensearch@6b69e2640682 ~]$ ls -R -la | grep analysis-
-rw-r--r-- 1 opensearch opensearch  1723718 Sep 12 10:57 lucene-analysis-common-9.11.1.jar
drwxr-xr-x  2 opensearch opensearch 4096 Sep 12 10:57 analysis-common
./modules/analysis-common:
-rw-r--r--  1 opensearch opensearch 199046 Sep 12 10:57 analysis-common-2.17.0.jar
-rw-r--r--  1 opensearch opensearch  34776 Sep 12 10:57 asm-analysis-9.7.jar

@dblock dblock mentioned this pull request Oct 11, 2024
Closed
3 tasks
@rursprung rursprung marked this pull request as ready for review November 7, 2024 09:25
@rursprung rursprung requested a review from dblock November 7, 2024 09:26
@rursprung
Copy link
Copy Markdown
Contributor Author

rursprung commented Nov 7, 2024

since 2.18.0 has now been released the plugin installation should now work and there's no need to use the snapshot build of 2.18.0 anymore. i've removed the snapshot part and pushed it here again (sorry, i don't have docker-compose available locally and thus can't test if this really works; but presuming that the release of the plugin happened then it should work fine)

rursprung
rursprung commented Nov 7, 2024
View reviewed changes
Comment thread tests/plugins/analysis/indices/analyze/analyzer/phone.yaml Outdated
dblock
dblock suggested changes Nov 7, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Almost there.

Comment thread CHANGELOG.md Outdated
Comment thread tests/plugins/analysis/indices/analyze/analyzer/phone.yaml Outdated
@dblock
Copy link
Copy Markdown
Member

dblock commented Nov 11, 2024

I fixed the Vale problem in #660.

@dblock
Copy link
Copy Markdown
Member

dblock commented Nov 12, 2024

I upgraded to 2.18 GA in #665, so this becomes easier.

@dblock dblock mentioned this pull request Nov 13, 2024
Merged
@rursprung
add phone number analysis plugin
780b653 
this is part of opensearch-project/OpenSearch#11326. the actual
implementation was done opensearch-project/OpenSearch#15915. see the
commit message on the PR for further details.

the new test group `analysis` has been added so that it can later be
extended with all other optional language analyzers (which are currently
also not covered).

Signed-off-by: Ralph Ursprung <Ralph.Ursprung@avaloq.com>
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Nov 14, 2024

Spec Test Coverage Analysis

Total Tested
517 349 (67.5 %)

@rursprung
Copy link
Copy Markdown
Contributor Author

rursprung commented Nov 14, 2024

i've now fixed the failure (text values are now quoted) and rebased after #666 => this should now be ready!

dblock
dblock suggested changes Nov 14, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

The CHANGELOG is in the wrong section, otherwise LGTM!

Comment thread CHANGELOG.md
- Added a spec style checker [#620](https://github.com/opensearch-project/opensearch-api-specification/pull/620).
- Added `remote_store` to node `Stats` ([#643](https://github.com/opensearch-project/opensearch-api-specification/pull/643))
- Added `/_cluster/stats/{metric}/nodes/{node_id}` and `/_cluster/stats/{metric}/{index_metric}/nodes/{node_id}` ([#639](https://github.com/opensearch-project/opensearch-api-specification/pull/639))
- Added `PhoneAnalyzer` from `analysis-phonenumber` plugin ([#609](https://github.com/opensearch-project/opensearch-api-specification/pull/609))
Copy link
Copy Markdown
Member

@dblock dblock Nov 14, 2024

Choose a reason for hiding this comment

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

This is in the wrong place, it should be in the unreleased section above. Actually I think the line above also is wrong here and possibly more (please check in this PR).

Copy link
Copy Markdown
Contributor Author

@rursprung rursprung Nov 15, 2024

Choose a reason for hiding this comment

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

oh, sorry, i think i missed that when rebasing! 🫣
thanks for fixing it!

@dblock
Copy link
Copy Markdown
Member

dblock commented Nov 14, 2024 •
edited
Loading

Actually, I'll merge and fix CHANGELOG in a separate PR.

Included in #671.

@dblock dblock merged commit 1c46b69 into opensearch-project:main Nov 14, 2024
@rursprung rursprung deleted the add-spec-for-opensearch-phone-plugin branch November 15, 2024 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@harshavamsi harshavamsi Awaiting requested review from harshavamsi harshavamsi is a code owner

@sachetalva sachetalva Awaiting requested review from sachetalva sachetalva is a code owner

@nhtruong nhtruong Awaiting requested review from nhtruong

@Xtansia Xtansia Awaiting requested review from Xtansia Xtansia is a code owner

@VachaShah VachaShah Awaiting requested review from VachaShah VachaShah is a code owner

@Tokesh Tokesh Awaiting requested review from Tokesh Tokesh is a code owner

@aabeshov aabeshov Awaiting requested review from aabeshov aabeshov is a code owner

1 more reviewer

@dblock dblock dblock approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rursprung @dblock