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

Simplify handling of keyword field normalizers #42002

Merged
merged 4 commits into from
May 10, 2019

Conversation

romseygeek
Copy link
Contributor

We have a number of places in analysis-handling code where we check
if a field type is a keyword field, and if so then extract the normalizer rather
than pulling the index-time analyzer. However, a keyword normalizer is
really just a special case of an analyzer, so we should be able to simplify this
by setting the normalizer as the index-time analyzer at construction time.

@romseygeek romseygeek self-assigned this May 9, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search

Copy link
Contributor

@mayya-sharipova mayya-sharipova left a comment

Choose a reason for hiding this comment

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

Thanks @romseygeek, the simplification makes sense.
We just need to update the test KeywordFieldMapperTests::testUpdateNormalizer

@romseygeek
Copy link
Contributor Author

The compatibility check now reports mapper [field] has a different [analyzer], mapper [field] has a different [normalizer], which isn't ideal, but there isn't really an easy way to remove the first clause there.

@romseygeek
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/1

@romseygeek romseygeek merged commit 6d95386 into elastic:master May 10, 2019
@romseygeek romseygeek deleted the custom-analyze-api branch May 10, 2019 14:15
romseygeek added a commit that referenced this pull request May 10, 2019
We have a number of places in analysis-handling code where we check
if a field type is a keyword field, and if so then extract the normalizer rather
than pulling the index-time analyzer. However, a keyword normalizer is
really just a special case of an analyzer, so we should be able to simplify this
by setting the normalizer as the index-time analyzer at construction time.
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request May 10, 2019
* elastic/master: (84 commits)
  [ML] adds geo_centroid aggregation support to data frames (elastic#42088)
  Add documentation for calendar/fixed intervals (elastic#41919)
  Remove global checkpoint assertion in peer recovery (elastic#41987)
  Don't create tempdir for cli scripts (elastic#41913)
  Fix debian-8 update (elastic#42056)
  Cleanup plugin bin directories (elastic#41907)
  Prevent order being lost for _nodes API filters (elastic#42045)
  Change IndexAnalyzers default analyzer access (elastic#42011)
  Remove reference to fs.data.spins in docs
  Mute failing AsyncTwoPhaseIndexerTests
  Remove close method in PageCacheRecycler/Recycler (elastic#41917)
  [ML] adding pivot.max_search_page_size option for setting paging size (elastic#41920)
  Docs: Tweak list formatting
  Simplify handling of keyword field normalizers (elastic#42002)
  [ML] properly nesting objects in document source (elastic#41901)
  Remove extra `ms` from log message (elastic#42068)
  Increase the sample space for random inner hits name generator (elastic#42057)
  Recognise direct buffers in heap size docs (elastic#42070)
  shouldRollGeneration should execute under read lock (elastic#41696)
  Wait for active shard after close in mixed cluster (elastic#42029)
  ...
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
We have a number of places in analysis-handling code where we check
if a field type is a keyword field, and if so then extract the normalizer rather
than pulling the index-time analyzer. However, a keyword normalizer is
really just a special case of an analyzer, so we should be able to simplify this
by setting the normalizer as the index-time analyzer at construction time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants