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

Mapping: Return _boost and _analyzer in the GET field mapping API #7589

Closed
wants to merge 3 commits into from

Conversation

brwe
Copy link
Contributor

@brwe brwe commented Sep 4, 2014

...so

_boost and _analyzer mapping can now be retrieved by their default name
or by given name ("path" for _analyzer, "name" for _boost)
Both still appear with their default name when just retrieving the mapping.
(see SimpleGetFieldMappingTests#testGet_boostAnd_analyzer and
SimpleGetMappingTests#testGet_boostAnd_analyzer)

The index_name handling is removed from _boost. This never worked anyway,
see this test: brwe@3645004

Change in behavior:

_analyzer was never a field mapper. When defining an analyzer
in a document, the field (_analyzer or custom name) was indexed
with default string field properties. These could be overwritten by
defining an explicit mapping for this field, for example:

PUT testidx/doc/_mapping
{
  "_analyzer": {
    "path": "custom_analyzer"
  },
  "properties": {
    "custom_analyzer": {
      "type": "string",
      "store": true
    }
  }
}

Now, this explicit mapping will be ignored completely, instead
one can only set the "index" option in the definition of _analyzer
Every other option will be ignored.

Reason for this change:
The documentation says

"By default, the _analyzer field is indexed, it can be disabled by settings index to no in the mapping."

This was not true - the setting was ignored. There was a test
for the explicit definition of the mapping (AnalyzerMapperTests#testAnalyzerMappingExplicit)
but this functionallity was never documented so I assume it is not in use.

closes #7237

Things that worry me:

I made it work, but am unsure if this is too hacky. I just made use of the fact that four different names are used for mappers (name, indexName, indexNameClean and full name) and set the name at the fitting place. However, there is plans for deprecating indexName (#6677) so I am unsure how long this solution will have any worth.

In addition the overwriting of the properties mapping relies on the fact that the order in which mappings are parsed is never changed.

Also, I wonder if the change in behavior for _analyzer qualifies as "breaking change".

… also

_boost and _analyzer mapping can now be retrieved by their default name
or by given name ("path" for _analyzer, "name" for _boost)
Both still appear with their default name when just retrieving the mapping.
(see SimpleGetFieldMappingTests#testGet_boostAnd_analyzer and
     SimpleGetMappingTests#testGet_boostAnd_analyzer)

The index_name handling is removed from _boost. This never worked anyway,
see this test: 3645004

Change in behavior:

_analyzer was never a field mapper. When defining an analyzer
in a document, the field (_analyzer or custom name) was indexed
with default string field properties. These could be overwritten by
defining an explicit mapping for this field, for example:

```
PUT testidx/doc/_mapping
{
  "_analyzer": {
    "path": "custom_analyzer"
  },
  "properties": {
    "custom_analyzer": {
      "type": "string",
      "store": true
    }
  }
}
```
Now, this explicit mapping will be ignored completely, instead
one can only set the "index" option in the definition of _analyzer
Every other option will be ignored.

Reason for this change:
The documentation says

"By default, the _analyzer field is indexed, it can be disabled by settings index to no in the mapping."

This was not true - the setting was ignored. There was a test
for the explicit definition of the mapping (AnalyzerMapperTests#testAnalyzerMappingExplicit)
but this functionallity was never documented so I assume it is not in use.

closes elastic#7237
@brwe brwe added review and removed review labels Sep 4, 2014
@brwe
Copy link
Contributor Author

brwe commented Sep 5, 2014

Better wait with the reviews. I think this needs a little more work.

@brwe brwe added the review label Sep 5, 2014
@brwe
Copy link
Contributor Author

brwe commented Sep 5, 2014

removed some unneeded code, should be ok now.

@clintongormley clintongormley changed the title get field mapping: return _boost and _analyzer by their default names al... Mapping: Return _boost and _analyzer in the GET field mapping API Sep 8, 2014
@rjernst
Copy link
Member

rjernst commented Sep 15, 2014

Why do we have _analyzer at all? This seems very strange that a document could define which analyzer it will use, since this could introduce discrepancies between what is indexed and what is queried? Can we just get rid of _analyzer? Or deprecate (and change the documentation that to indicate that it is always indexed), and remove in master?

@clintongormley
Copy link
Contributor

@rjernst i agree, i definitely want to deprecate it. but this PR is just about making the existing functionality consistent.

@rjernst
Copy link
Member

rjernst commented Sep 24, 2014

But this is technically adding new functionality (which was previously documented, but unimplemented). Could we instead:

  1. Remove _analyzer from master
  2. Update documentation for 1.x to say:
  • _analyzer is always indexed
  • It is deprecated, will be removed in 2.0, and you should not use it.

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.

Mapping: Get field mapping does not return _analyzer and _boost fields
4 participants