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

Raw mapping merge fix for properties field #108867

Merged

Conversation

eyalkoren
Copy link
Contributor

Fixes #108866

@eyalkoren eyalkoren added >bug :Search Foundations/Mapping Index mappings, including merging and defining field types v8.14.1 labels May 21, 2024
@eyalkoren eyalkoren self-assigned this May 21, 2024
@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label May 21, 2024
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine
Copy link
Collaborator

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

eyalkoren added 3 commits May 22, 2024 08:40
…perties-field' into raw-mapping-merge-fix-for-properties-field
Copy link
Member

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

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

LGTM

@eyalkoren
Copy link
Contributor Author

cc @chiarch84

@chiarachiarelli
Copy link

Thanks a lot for following this. Sorry for not commenting earlier but I do not have access to my pc for some time to test It.

@cbuescher cbuescher self-requested a review May 22, 2024 12:30
@cbuescher cbuescher self-assigned this May 22, 2024
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

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

I took a deeper look at the custom merge logic in RawFieldMappingMerge and agree that passing on "null" in the recursion when the parent is a "properties" field should not affect any further merging logic down the line.
LGTM for that reason.

@eyalkoren
Copy link
Contributor Author

eyalkoren commented May 27, 2024

I took a deeper look at the custom merge logic in RawFieldMappingMerge and agree that passing on "null" in the recursion when the parent is a "properties" field should not affect any further merging logic down the line.

Thanks for verifying 🙏
Maybe another way to put it is that XContentHelper.merge() provides tree-merge functionality and the optional parent argument can be used to add context for the merge if necessary. When merging mapping subtrees of a field, we only need the plain merge functionality, so this additional context is not required.
Therefore, there is already a merge API to do just that, so I just merged a tiny change to use it instead. I think it makes it a bit cleaner. I am asking for a re-review just as sanity check.

@eyalkoren eyalkoren requested a review from cbuescher May 27, 2024 06:56
Copy link
Member

@felixbarny felixbarny left a comment

Choose a reason for hiding this comment

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

Just a nit on a comment, otherwise LGTM

@felixbarny
Copy link
Member

Tip: you can apply the auto-backport-and-merge label so that the PR automatically gets back ported to 8.14.1 after it is merged.

@eyalkoren eyalkoren merged commit 92dc76e into elastic:main May 28, 2024
14 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

The backport operation could not be completed due to the following error:

An unhandled error occurred. Please consult the logs

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 108867

@eyalkoren
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.14

Questions ?

Please refer to the Backport tool documentation

eyalkoren added a commit to eyalkoren/elasticsearch that referenced this pull request May 29, 2024
elasticsearchmachine pushed a commit that referenced this pull request May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport pending >bug :Search Foundations/Mapping Index mappings, including merging and defining field types Team:Search Meta label for search team v8.14.1 v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure to merge mappings of field named properties
5 participants