Skip to content

Fix propagation of dynamic mapping parameter when applying copy_to - #121109

Merged
lkts merged 13 commits into
elastic:mainfrom
lkts:fix_113049
Jan 30, 2025
Merged

Fix propagation of dynamic mapping parameter when applying copy_to#121109
lkts merged 13 commits into
elastic:mainfrom
lkts:fix_113049

Conversation

@lkts

@lkts lkts commented Jan 28, 2025

Copy link
Copy Markdown
Contributor

Closes #113049.

@lkts lkts added >bug :Search Foundations/Mapping Index mappings, including merging and defining field types auto-backport Automatically create backport pull requests when merged :StorageEngine/Mapping The storage related side of mappings v8.18.0 labels Jan 28, 2025
@elasticsearchmachine elasticsearchmachine added Team:StorageEngine v9.0.0 Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch labels Jan 28, 2025
@elasticsearchmachine

elasticsearchmachine commented Jan 28, 2025

Copy link
Copy Markdown
Collaborator

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

@elasticsearchmachine

elasticsearchmachine commented Jan 28, 2025

Copy link
Copy Markdown
Collaborator

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

@elasticsearchmachine

elasticsearchmachine commented Jan 28, 2025

Copy link
Copy Markdown
Collaborator

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

lkts
lkts commented Jan 28, 2025
View reviewed changes
Comment thread server/src/main/java/org/elasticsearch/index/mapper/DocumentParserContext.java

// Used to create a copy_to context.
// It is important to reset `dynamic` here since it is possible that we copy into a completely different object.
private Wrapper(RootObjectMapper root, DocumentParserContext in) {

@lkts lkts Jan 28, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Note that the call site didn't change here, this works because createCopyToContext is the only place that passes a root maper to Wrapper constructor. I think this is fragile but i don't have anything better.

@kkrik-es kkrik-es Jan 30, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shall we create a factory method like createWithRootDynamic and use it explicitly?

@lkts lkts Jan 30, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's an optioin but you still need this constructor to call super.

@jordan-powers jordan-powers mentioned this pull request Jan 29, 2025
Closed
@lkts

lkts commented Jan 29, 2025

Copy link
Copy Markdown
Contributor Author

Closes #120384.

@lkts
lkts enabled auto-merge (squash) January 30, 2025 16:23
@lkts

lkts commented Jan 30, 2025

Copy link
Copy Markdown
Contributor Author

CI failure is #121335.

@lkts
lkts merged commit 1225b07 into elastic:main Jan 30, 2025
@elasticsearchmachine

elasticsearchmachine commented Jan 30, 2025

Copy link
Copy Markdown
Collaborator

💔 Backport failed

Status Branch Result
❌ 8.x Commit could not be cherrypicked due to conflicts

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

saikatsarkar056 pushed a commit to saikatsarkar056/elasticsearch that referenced this pull request Jan 31, 2025
@lkts @saikatsarkar056
Fix propagation of dynamic mapping parameter when applying copy_to (e…
5205392 
…lastic#121109)
@lkts lkts mentioned this pull request Jan 31, 2025
Merged
@lkts

lkts commented Jan 31, 2025

Copy link
Copy Markdown
Contributor Author

💚 All backports created successfully

Status Branch Result
✅ 8.x

Questions ?

Please refer to the Backport tool documentation

@lkts
lkts deleted the fix_113049 branch January 31, 2025 01:30
elasticsearchmachine pushed a commit that referenced this pull request Jan 31, 2025
@lkts
Fix propagation of dynamic mapping parameter when applying copy_to (#…
7ff4f3e 
…121109) (#121357)

(cherry picked from commit 1225b07)

# Conflicts:
#	rest-api-spec/build.gradle
bpintea pushed a commit to bpintea/elasticsearch that referenced this pull request Jan 31, 2025
@lkts @bpintea
Fix propagation of dynamic mapping parameter when applying copy_to (e…
2919025 
…lastic#121109)
@tlrx

tlrx commented Jan 31, 2025

Copy link
Copy Markdown
Member

I suspect this change needs to be backported to the 9.0 branch as well: some BWC tests that upgrade a cluster from 8.19.0 to 9.0 are now failing due to the missing required features [mapper.copy_to.dynamic_handling].

See #121362 or #121365 for example.

@kkrik-es kkrik-es mentioned this pull request Jan 31, 2025
Merged
@kkrik-es

kkrik-es commented Jan 31, 2025

Copy link
Copy Markdown
Member

💚 All backports created successfully

Status Branch Result
✅ 9.0

Questions ?

Please refer to the Backport tool documentation

kkrik-es pushed a commit to kkrik-es/elasticsearch that referenced this pull request Jan 31, 2025
@lkts
Fix propagation of dynamic mapping parameter when applying copy_to (e…
53548d9 
…lastic#121109)

(cherry picked from commit 1225b07)

# Conflicts:
#	rest-api-spec/build.gradle
@kkrik-es

kkrik-es commented Jan 31, 2025

Copy link
Copy Markdown
Member

I suspect this change needs to be backported to the 9.0 branch as well: some BWC tests that upgrade a cluster from 8.19.0 to 9.0 are now failing due to the missing required features [mapper.copy_to.dynamic_handling].

See #121362 or #121365 for example.

Created the backport with auto-merge.

@tlrx tlrx mentioned this pull request Jan 31, 2025
Closed
kkrik-es added a commit that referenced this pull request Jan 31, 2025
@kkrik-es @lkts
Fix propagation of dynamic mapping parameter when applying copy_to (#…
21710e7 
…121109) (#121386)

(cherry picked from commit 1225b07)

# Conflicts:
#	rest-api-spec/build.gradle

Co-authored-by: Oleksandr Kolomiiets <oleksandr.kolomiiets@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kkrik-es kkrik-es kkrik-es approved these changes
@martijnvg martijnvg Awaiting requested review from martijnvg
@jordan-powers jordan-powers Awaiting requested review from jordan-powers

Assignees

No one assigned

Labels

auto-backport Automatically create backport pull requests when merged backport pending >bug :Search Foundations/Mapping Index mappings, including merging and defining field types :StorageEngine/Mapping The storage related side of mappings Team:Search Foundations Meta label for the Search Foundations team in Elasticsearch Team:StorageEngine v8.19.0 v9.1.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

copy_to logic fails when used to copy value from object with dynamic: "strict" to a dynamically mapped field

4 participants

@lkts @elasticsearchmachine @tlrx @kkrik-es