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.

Sign up for GitHub

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

Jump to bottom

Flatten object mappings when subobjects is false #103542

Merged
merged 29 commits into from
Feb 22, 2024
Merged

Flatten object mappings when subobjects is false #103542

merged 29 commits into from
Feb 22, 2024

Conversation

felixbarny
Copy link
Member

@felixbarny felixbarny commented Dec 19, 2023 •
edited
Loading

Closes #99860
Also fixes #103497

@felixbarny felixbarny added >feature :Search Foundations/Mapping Index mappings, including merging and defining field types labels Dec 19, 2023
@felixbarny felixbarny self-assigned this Dec 19, 2023
@elasticsearchmachine elasticsearchmachine added v8.13.0 Team:Search Meta label for search team external-contributor Pull request authored by a developer outside the Elasticsearch team labels Dec 19, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

@elasticsearchmachine
Copy link
Collaborator

Hi @felixbarny, I've created a changelog YAML for you.

@felixbarny felixbarny force-pushed the flatten-mappings-subobjects-false branch from c71feaf to 47131c7 Compare December 19, 2023 08:18
@felixbarny felixbarny requested a review from javanna January 23, 2024 10:58
@siposea siposea added the :StorageEngine/Logs You know, for Logs label Jan 25, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

@felixbarny felixbarny mentioned this pull request Feb 12, 2024
Closed
@elasticsearchmachine
Copy link
Collaborator

Hi @felixbarny, I've updated the changelog YAML for you.

@felixbarny felixbarny mentioned this pull request Feb 13, 2024
Closed
javanna
javanna reviewed Feb 19, 2024
View reviewed changes
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

I did a first pass and left a bunch of comments.

docs/reference/mapping/params/subobjects.asciidoc Outdated

* The <<enabled, `enabled`>> mapping parameter must not be `false`.
* The <<dynamic, `dynamic`>> mapping parameter must not contradict the implicit or explicit value of the parent.
* The <<subobjects, `subobjects`>> mapping parameter mot not be set to `true` explicitly.
Copy link
Member

Choose a reason for hiding this comment

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

when can it happen that subobjects is re-enabled when it's been set to false for a certain branch in the object tree? I thought this is not allowed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Think of a component template (A) that was written with "traditional" object mappings in mind (subobjects is not set, the default value true is used). This PR makes it so that this component template can be used in combination with another component template (B) that sets subobjects to false. However, one limitation is that in component template A, the object mappers must not be defined with subobjects: true. We could also choose to just discard the subobjects mapping parameter but I chose a more conservative approach that would reject such mappings. However, the consequence is that adding subobjects: false to existing mappings may fail.

Copy link
Member

Choose a reason for hiding this comment

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

ok. I see how this makes sense applied to component templates. I got confused because in the docs there is no context around component templates, and subobjects can't normally be changed back to true once set to false, so I was wondering: that is already not valid mappings, why do we need to state that explicitly. I am still uncertain of whether we should specify it or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

I realized this not only applies to component templates but also to mapping updates.

docs/reference/mapping/params/subobjects.asciidoc Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java Show resolved Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java Outdated Show resolved Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java Show resolved Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java Show resolved Hide resolved
server/src/test/java/org/elasticsearch/index/mapper/MapperServiceTests.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/Mapper.java Outdated Show resolved Hide resolved
felixbarny added a commit to felixbarny/elasticsearch that referenced this pull request Feb 20, 2024
@felixbarny
Encapsulate Mapper.Builder#name and make it private
4438240 
This is in preparation to make the field mutable,
which is needed in the context of elastic#103542
@felixbarny felixbarny mentioned this pull request Feb 20, 2024
Merged
felixbarny added a commit that referenced this pull request Feb 20, 2024
@felixbarny
Encapsulate Mapper.Builder#name and make it private (#105648)
5920c91 
This is in preparation to make the field mutable,
which is needed in the context of #103542
javanna
javanna reviewed Feb 20, 2024
View reviewed changes
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

a few more questions and comments, this is almost ready.

server/src/main/java/org/elasticsearch/index/mapper/MapperMergeContext.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java Show resolved Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java Outdated Show resolved Hide resolved
server/src/main/java/org/elasticsearch/index/mapper/ObjectMapper.java Outdated Show resolved Hide resolved
javanna
javanna approved these changes Feb 22, 2024
View reviewed changes
Copy link
Member

@javanna javanna left a comment

Choose a reason for hiding this comment

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

LGTM, great work.

@felixbarny felixbarny merged commit dee0be5 into elastic:main Feb 22, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javanna javanna javanna approved these changes

Assignees

@felixbarny felixbarny

Labels
external-contributor Pull request authored by a developer outside the Elasticsearch team >feature :Search Foundations/Mapping Index mappings, including merging and defining field types :StorageEngine/Logs You know, for Logs Team:Search Meta label for search team Team:StorageEngine v8.14.0
Projects
None yet
Milestone
No milestone
4 participants
@felixbarny @elasticsearchmachine @javanna @siposea