[Remove] Type from nested fields using new metadata field mapper #3004

nknize · 2022-04-20T19:00:20Z

types support is removed yet nested documents use the _type field to store the
path for nested documents. A new _nested_path metadata field mapper is added to
take the place of the _type field in order to remove the type dependency in
nested documents. BWC is handled in the new field mapper to ensure compatibility
with older versions.

relates #1940
relates #2989

opensearch-ci-bot · 2022-04-20T19:35:38Z

✅ Gradle Check success dd6b10427af69f2f02101fc5ca0b002f4253f3a4
Log 4646

Reports 4646

server/src/main/java/org/opensearch/indices/mapper/MapperRegistry.java

reta · 2022-04-20T20:19:28Z

server/src/main/java/org/opensearch/index/mapper/NestedPathFieldMapper.java

+public class NestedPathFieldMapper extends MetadataFieldMapper {
+    // OpenSearch version 2.0 removed types; this name is used for bwc
+    public static final String LEGACY_NAME = "_type";
+    public static final String NAME = "_nested_path";


Just an opinion, _nested_path looks too verbose, may be just _path?

I ended up sticking with _nested_path because it's more descriptive when analyzing lucene segments (which has no object->field relationship). The fieldType is easily identifiable as the path for a nested field when just reading the field name.

reta · 2022-04-20T20:25:22Z

server/src/main/java/org/opensearch/index/mapper/ObjectMapper.java

-        this.nestedTypePathAsString = "__" + fullPath;
-        this.nestedTypePathAsBytes = new BytesRef(nestedTypePathAsString);
-        this.nestedTypeFilter = new TermQuery(new Term(TypeFieldMapper.NAME, nestedTypePathAsBytes));
+        Version version = Version.indexCreated(settings);


This is a bit risky call: when fixing one of the Joda conversions, realized that settings may not hold the index created version all the time [1], may be worth checking here as well.

[1] https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/index/mapper/Mapper.java#L258

Woah! Did you find this to be true in production or could it have just been a test that didn't set the Version? I don't think we can have a case where an index is created without a version, otherwise rolling upgrades would fail?

It is this one https://github.com/opensearch-project/OpenSearch/pull/2094/files, to clarify, it is not caused by absence of the index created but prefabricated index settings which may not include it.

Just getting back to this. The testDefaultConfig I had in there would've failed since it uses prefabricated empty settings but it was updating w/ different mappings so I added an explicit test based on #1921 to update with the same mappings and Empty settings. There are no issues; we may revisit if this rears it's head again.

server/src/main/java/org/opensearch/index/mapper/MapperService.java

CEHENKLE · 2022-04-21T01:06:13Z

Hey @nknize Just double checking -- I do not think you meant for this to get pulled into 2.0.0-rc1. If anything, we would fix it between 2.0.0-rc1 and 2.0.0, right?

nknize · 2022-04-21T01:08:31Z

between 2.0.0-rc1 and 2.0.0, right?

Correct. We just need to get it in for 2.0.0 along with TypeFieldMapper removal.

opensearch-ci-bot · 2022-04-21T03:43:21Z

✅ Gradle Check success 75767ae2fb4fcc747e871cdb83a67d57210cc8c9
Log 4664

Reports 4664

opensearch-ci-bot · 2022-04-21T04:15:21Z

✅ Gradle Check success 04319d2dad7d4bcbab3a1a16c3a654309d8f4b19
Log 4669

Reports 4669

types support is removed yet nested documents use the _type field to store the path for nested documents. A new _nested_path metadata field mapper is added to take the place of the _type field in order to remove the type dependency in nested documents. BWC is handled in the new field mapper to ensure compatibility with older versions. Signed-off-by: Nicholas Walter Knize <[email protected]>

Signed-off-by: Nicholas Walter Knize <[email protected]>

opensearch-ci-bot · 2022-04-25T17:11:25Z

✅ Gradle Check success bf6aecc
Log 4784

Reports 4784

dreamer-89

Thank you @nknize for picking up this tricky change. LGTM!

nknize · 2022-04-26T18:56:29Z

I'm going to give this until COB today and will merge if there are no objections. We need to get this in.

* [Remove] Type from nested fields using new metadata field mapper types support is removed yet nested documents use the _type field to store the path for nested documents. A new _nested_path metadata field mapper is added to take the place of the _type field in order to remove the type dependency in nested documents. BWC is handled in the new field mapper to ensure compatibility with older versions. Signed-off-by: Nicholas Walter Knize <[email protected]> * pr fixes Signed-off-by: Nicholas Walter Knize <[email protected]> * add test to merge same mapping with empty index settings Signed-off-by: Nicholas Walter Knize <[email protected]> (cherry picked from commit 6b641d2)

opensearch-trigger-bot · 2022-04-26T20:20:58Z

The backport to 2.0 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.0 2.0
# Navigate to the new working tree
cd .worktrees/backport-2.0
# Create a new branch
git switch --create backport/backport-3004-to-2.0
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 6b641d2fd29b9542b94f419fc083847fa5bdf55b
# Push it to GitHub
git push --set-upstream origin backport/backport-3004-to-2.0
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.0

Then, create a pull request where the base branch is 2.0 and the compare/head branch is backport/backport-3004-to-2.0.

* [Remove] Type from nested fields using new metadata field mapper types support is removed yet nested documents use the _type field to store the path for nested documents. A new _nested_path metadata field mapper is added to take the place of the _type field in order to remove the type dependency in nested documents. BWC is handled in the new field mapper to ensure compatibility with older versions. Signed-off-by: Nicholas Walter Knize <[email protected]> (cherry picked from commit 6b641d2)

…nsearch-project#3004) * [Remove] Type from nested fields using new metadata field mapper types support is removed yet nested documents use the _type field to store the path for nested documents. A new _nested_path metadata field mapper is added to take the place of the _type field in order to remove the type dependency in nested documents. BWC is handled in the new field mapper to ensure compatibility with older versions. Signed-off-by: Nicholas Walter Knize <[email protected]>

…) (#3085) * [Remove] Type from nested fields using new metadata field mapper types support is removed yet nested documents use the _type field to store the path for nested documents. A new _nested_path metadata field mapper is added to take the place of the _type field in order to remove the type dependency in nested documents. BWC is handled in the new field mapper to ensure compatibility with older versions. Signed-off-by: Nicholas Walter Knize <[email protected]> (cherry picked from commit 6b641d2) Co-authored-by: Nick Knize <[email protected]>

…eld mapper (#3097) * [Remove] Type from nested fields using new metadata field mapper (#3004) * [Remove] Type from nested fields using new metadata field mapper types support is removed yet nested documents use the _type field to store the path for nested documents. A new _nested_path metadata field mapper is added to take the place of the _type field in order to remove the type dependency in nested documents. BWC is handled in the new field mapper to ensure compatibility with older versions. Signed-off-by: Nicholas Walter Knize <[email protected]> * spotless apply Signed-off-by: Nicholas Walter Knize <[email protected]> * add back Nested field mapper to IndicesModuleTests Signed-off-by: Nicholas Walter Knize <[email protected]>

nknize added non-issue bugs / unexpected behaviors that end up non issues; audit trail simple changes that aren't issues backwards-compatibility v3.0.0 Issues and PRs related to version 3.0.0 backport 2.x Backport to 2.x branch backport 2.0 Backport to 2.0 branch labels Apr 20, 2022

nknize requested review from a team and reta as code owners April 20, 2022 19:00

reta reviewed Apr 20, 2022

View reviewed changes

server/src/main/java/org/opensearch/indices/mapper/MapperRegistry.java Outdated Show resolved Hide resolved

reta reviewed Apr 20, 2022

View reviewed changes

dreamer-89 reviewed Apr 20, 2022

View reviewed changes

server/src/main/java/org/opensearch/index/mapper/MapperService.java Outdated Show resolved Hide resolved

dreamer-89 mentioned this pull request Apr 20, 2022

[Type Removal] Remove TypeFieldMapper usage, remove support of _type in searches and from LeafFieldsLookup #3016

Merged

5 tasks

nknize force-pushed the types/nestedPathFieldMapper branch 2 times, most recently from 75767ae to 04319d2 Compare April 21, 2022 03:24

This was referenced Apr 21, 2022

[BUG] _type value in field of exists query returns all records #2989

Closed

Remove type from nested documents; where types used to store nested path information #2357

Closed

nknize added 3 commits April 25, 2022 11:15

pr fixes

478a61e

Signed-off-by: Nicholas Walter Knize <[email protected]>

add test to merge same mapping with empty index settings

bf6aecc

Signed-off-by: Nicholas Walter Knize <[email protected]>

nknize force-pushed the types/nestedPathFieldMapper branch from 04319d2 to bf6aecc Compare April 25, 2022 16:40

dreamer-89 approved these changes Apr 25, 2022

View reviewed changes

dblock requested review from andrross and kartg April 25, 2022 18:52

dblock approved these changes Apr 26, 2022

View reviewed changes

dblock merged commit 6b641d2 into opensearch-project:main Apr 26, 2022

opensearch-trigger-bot bot mentioned this pull request Apr 26, 2022

[Backport 2.x] [Remove] Type from nested fields using new metadata field mapper #3085

Merged

nknize mentioned this pull request Apr 27, 2022

[Backport 2.0] [Remove] Type from nested fields using new metadata field mapper #3097

Merged

martin-gaievski mentioned this pull request Jun 28, 2022

Adding 'dense_vector' field type #3659

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Remove] Type from nested fields using new metadata field mapper #3004

[Remove] Type from nested fields using new metadata field mapper #3004

nknize commented Apr 20, 2022 •

edited

Loading

opensearch-ci-bot commented Apr 20, 2022

reta Apr 20, 2022

nknize Apr 21, 2022

reta Apr 21, 2022

reta Apr 20, 2022

nknize Apr 21, 2022 •

edited

Loading

reta Apr 21, 2022 •

edited

Loading

nknize Apr 25, 2022

CEHENKLE commented Apr 21, 2022

nknize commented Apr 21, 2022

opensearch-ci-bot commented Apr 21, 2022

opensearch-ci-bot commented Apr 21, 2022

opensearch-ci-bot commented Apr 25, 2022

dreamer-89 left a comment

nknize commented Apr 26, 2022

opensearch-trigger-bot bot commented Apr 26, 2022

[Remove] Type from nested fields using new metadata field mapper #3004

[Remove] Type from nested fields using new metadata field mapper #3004

Conversation

nknize commented Apr 20, 2022 • edited Loading

opensearch-ci-bot commented Apr 20, 2022

reta Apr 20, 2022

Choose a reason for hiding this comment

nknize Apr 21, 2022

Choose a reason for hiding this comment

reta Apr 21, 2022

Choose a reason for hiding this comment

reta Apr 20, 2022

Choose a reason for hiding this comment

nknize Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

reta Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

nknize Apr 25, 2022

Choose a reason for hiding this comment

CEHENKLE commented Apr 21, 2022

nknize commented Apr 21, 2022

opensearch-ci-bot commented Apr 21, 2022

opensearch-ci-bot commented Apr 21, 2022

opensearch-ci-bot commented Apr 25, 2022

dreamer-89 left a comment

Choose a reason for hiding this comment

nknize commented Apr 26, 2022

opensearch-trigger-bot bot commented Apr 26, 2022

nknize commented Apr 20, 2022 •

edited

Loading

nknize Apr 21, 2022 •

edited

Loading

reta Apr 21, 2022 •

edited

Loading