Remove extraneous references to 'tokenized' in the mapper code. by jtibshirani · Pull Request #31010 · elastic/elasticsearch

jtibshirani · 2018-05-31T18:09:53Z

These are likely left over from when there were three options for the index mapping (no, analyzed, and not_analyzed).

These are likely left over from when there were three options for the index mapping ('no', 'analyzed', 'not_analyzed').

elasticmachine · 2018-05-31T20:57:14Z

Pinging @elastic/es-search-aggs

cbuescher

The deletion in the BooleanFieldMapper and the FieldMapper make sense to me, the code seems unused. I left two questions though where I might not understand the difference of FieldMapper#tokenized (removed) vs. FieldType#tokenized.

cbuescher · 2018-06-06T11:46:38Z

server/src/main/java/org/elasticsearch/index/mapper/FieldMapper.java

        boolean indexed =  fieldType().indexOptions() != IndexOptions.NONE;
        boolean defaultIndexed = defaultFieldType.indexOptions() != IndexOptions.NONE;
-        if (includeDefaults || indexed != defaultIndexed ||
-            fieldType().tokenized() != defaultFieldType.tokenized()) {


This is another tokenized() method than the one deleted above, no? I don't know enough about the details of how mappings work, but how are they different? How is this removal in the if-clause related to the rest of this change?

cbuescher · 2018-06-06T11:47:44Z

server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java

        boolean indexed =  indexOptions() != IndexOptions.NONE;
        boolean mergeWithIndexed = other.indexOptions() != IndexOptions.NONE;
        // TODO: should be validating if index options go "up" (but "down" is ok)
-        if (indexed != mergeWithIndexed || tokenized() != other.tokenized()) {


Same here, this it the field type tokenized flag, no? Not sure if this matters.

jtibshirani · 2018-06-08T01:21:42Z

@cbuescher thanks for taking a look! They are indeed separate methods, but they concern the same flag. In short, each FieldMapper, which represents a field mapping, produces a MappedFieldType, which corresponds to an actual lucene field type and is used when constructing queries, highlighting, etc. Many methods on FieldMapper.Builder simply write through to the corresponding methods on the underlying MappedFieldType, like the tokenized method that I removed.

Previously when we had the string type (along with the index options analyzed, not_analyzed, and no), a single type of field could be set to tokenized, or not tokenized. My understanding is that now that string is split into keyword and text, this is no longer the case — a field’s type now fully determines whether its tokenized flag will be true or false (it’s always false for BooleanFieldType, always true for TextFieldType, etc.) This means that (1) it does not make sense to have a FieldMapper.Builder#tokenized method, as a field type’s tokenized flag shouldn’t change, and (2) if two fields are of the same type, we don’t need to check whether their ‘tokenized()’ values are the same.

I hope this helps explain the reasoning behind the change. I really appreciate additional sanity checks on these mapping refactors, as it seems like the code is a bit older and non-obvious.

cbuescher

this is no longer the case — a field’s type now fully determines whether its tokenized flag will be true or false

Thanks for the great explanation, I think I understand now. LGTM.

jpountz

LGTM

These are likely left over from when there were three options for the index mapping ('no', 'analyzed', 'not_analyzed'). (cherry picked from commit 2378fa1)

* 6.x: Move default location of dependencies report (#31228) Remove dependencies report task dependencies (#31227) Add recognition of MPL 2.0 (#31226) Fix unknown licenses (#31223) Fully encapsulate LocalCheckpointTracker inside of the engine (#31213) Remove version from license file name for GCS SDK (#31221) Remove DocumentFieldMappers#simpleMatchToFullName. (#31041) [DOCS] Removes 6.3.1 release notes [DOCS] Splits release notes by major version Remove DocumentFieldMappers#smartNameFieldMapper, as it is no longer needed. (#31018) Remove extraneous references to 'tokenized' in the mapper code. (#31010) SQL: Make a single JDBC driver jar (#31012) QA: Fix rolling restart tests some more Allow to trim all ops above a certain seq# with a term lower than X high level REST api: cancel task (#30745) Mute TokenBackwardsCompatibilityIT.testMixedCluster Mute WatchBackwardsCompatibilityIT.testWatcherRestart Enhance license detection for various licenses (#31198) [DOCS] Add note about long-lived idle connections (#30990) Add high-level client methods that accept RequestOptions (#31069) Remove RestGetAllMappingsAction (#31129) Move RestGetSettingsAction to RestToXContentListener (#31101) Move number of language analyzers to analysis-common module (#31143) flush job to ensure all results have been written (#31187)

* master: Move default location of dependencies report (#31228) Remove dependencies report task dependencies (#31227) Add recognition of MPL 2.0 (#31226) Fix unknown licenses (#31223) Remove version from license file name for GCS SDK (#31221) Fully encapsulate LocalCheckpointTracker inside of the engine (#31213) [DOCS] Added 'fail_on_unsupported_field' param to MLT. Closes #28008 (#31160) Add licenses for transport-nio (#31218) Remove DocumentFieldMappers#simpleMatchToFullName. (#31041) Allow to trim all ops above a certain seq# with a term lower than X, post backport fix (#31211) Compliant SAML Response destination check (#31175) Remove DocumentFieldMappers#smartNameFieldMapper, as it is no longer needed. (#31018) Remove extraneous references to 'tokenized' in the mapper code. (#31010) Allow to trim all ops above a certain seq# with a term lower than X (#30176) SQL: Make a single JDBC driver jar (#31012) Enhance license detection for various licenses (#31198) [DOCS] Add note about long-lived idle connections (#30990) Move number of language analyzers to analysis-common module (#31143) Default max concurrent search req. numNodes * 5 (#31171) flush job to ensure all results have been written (#31187)

…ecker * elastic/master: (309 commits) [test] add fix for rare virtualbox error (elastic#31212) Move default location of dependencies report (elastic#31228) Remove dependencies report task dependencies (elastic#31227) Add recognition of MPL 2.0 (elastic#31226) Fix unknown licenses (elastic#31223) Remove version from license file name for GCS SDK (elastic#31221) Fully encapsulate LocalCheckpointTracker inside of the engine (elastic#31213) [DOCS] Added 'fail_on_unsupported_field' param to MLT. Closes elastic#28008 (elastic#31160) Add licenses for transport-nio (elastic#31218) Remove DocumentFieldMappers#simpleMatchToFullName. (elastic#31041) Allow to trim all ops above a certain seq# with a term lower than X, post backport fix (elastic#31211) Compliant SAML Response destination check (elastic#31175) Remove DocumentFieldMappers#smartNameFieldMapper, as it is no longer needed. (elastic#31018) Remove extraneous references to 'tokenized' in the mapper code. (elastic#31010) Allow to trim all ops above a certain seq# with a term lower than X (elastic#30176) SQL: Make a single JDBC driver jar (elastic#31012) Enhance license detection for various licenses (elastic#31198) [DOCS] Add note about long-lived idle connections (elastic#30990) Move number of language analyzers to analysis-common module (elastic#31143) Default max concurrent search req. numNodes * 5 (elastic#31171) ...

Remove extraneous references to 'tokenized' in the mapper code.

e5cf863

These are likely left over from when there were three options for the index mapping ('no', 'analyzed', 'not_analyzed').

jtibshirani added WIP :Search Foundations/Mapping Index mappings, including merging and defining field types v7.0.0 v6.4.0 labels May 31, 2018

jtibshirani added >refactoring review and removed WIP labels May 31, 2018

cbuescher reviewed Jun 6, 2018

View reviewed changes

cbuescher approved these changes Jun 8, 2018

View reviewed changes

jpountz approved these changes Jun 8, 2018

View reviewed changes

jtibshirani merged commit 2378fa1 into elastic:master Jun 8, 2018

jtibshirani deleted the refactor/field-mapper branch June 8, 2018 16:23

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove extraneous references to 'tokenized' in the mapper code.#31010

Remove extraneous references to 'tokenized' in the mapper code.#31010
jtibshirani merged 1 commit intoelastic:masterfrom
jtibshirani:refactor/field-mapper

jtibshirani commented May 31, 2018 •

edited

Loading

Uh oh!

elasticmachine commented May 31, 2018

Uh oh!

cbuescher left a comment

Uh oh!

cbuescher Jun 6, 2018

Uh oh!

cbuescher Jun 6, 2018

Uh oh!

jtibshirani commented Jun 8, 2018

Uh oh!

cbuescher left a comment

Uh oh!

jpountz left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

jtibshirani commented May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented May 31, 2018

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

cbuescher Jun 6, 2018

Choose a reason for hiding this comment

Uh oh!

cbuescher Jun 6, 2018

Choose a reason for hiding this comment

Uh oh!

jtibshirani commented Jun 8, 2018

Uh oh!

cbuescher left a comment

Choose a reason for hiding this comment

Uh oh!

jpountz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jtibshirani commented May 31, 2018 •

edited

Loading