-
Notifications
You must be signed in to change notification settings - Fork 25.4k
Adding a merge_type parameter to the ingest simulate API #132210
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
Adding a merge_type parameter to the ingest simulate API #132210
Conversation
Hi @masseyke, I've created a changelog YAML for you. |
Hi @masseyke, I've updated the changelog YAML for you. |
Drive-by bikeshed :D — how about |
Think it's still clear that it refers only to mappings though? I didn't want to imply anything about the way settings or pipelines were merged. |
Pinging @elastic/es-data-management (Team:Data Management) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a new merge_type
parameter to the ingest simulate API to control how mapping overrides are merged with existing mappings. The parameter accepts "index" or "template" values, allowing users to choose between merging mappings as they would be merged into an existing index versus how they would be merged into templates.
Key changes:
- Added
merge_type
parameter toSimulateBulkRequest
constructor and related API endpoints - Updated mapping validation logic to use the appropriate
MapperService.MergeReason
based on the merge type - Added comprehensive test coverage for the new functionality
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
SimulateBulkRequest.java |
Added mappingMergeType field and updated constructor to accept the new parameter |
RestSimulateIngestAction.java |
Added extraction of merge_type parameter from REST request |
TransportSimulateBulkAction.java |
Updated mapping validation to use the appropriate merge reason based on merge type |
TransportVersions.java |
Added new transport version for the feature |
Test files | Updated all test constructors to include the new parameter |
API spec | Added documentation for the new merge_type parameter |
Integration tests | Added test cases to verify the new functionality works correctly |
rest-api-spec/src/main/resources/rest-api-spec/api/simulate.ingest.json
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/action/bulk/TransportSimulateBulkAction.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@@ -357,6 +357,7 @@ static TransportVersion def(int id) { | |||
public static final TransportVersion COMPONENT_TEMPLATE_TRACKING_INFO = def(9_132_0_00); | |||
public static final TransportVersion TO_CHILD_BLOCK_JOIN_QUERY = def(9_133_0_00); | |||
public static final TransportVersion ML_INFERENCE_AI21_COMPLETION_ADDED = def(9_134_0_00); | |||
public static final TransportVersion SIMULATE_INGEST_MAPPING_MERGE_TYPE = def(9_135_0_00); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If i understand correctly, seems we need to update this version number due to the change in the API request? https://github.com/elastic/elasticsearch/blob/21e8bac36e84a73a8c3aa9740d4b92333edca7ef/docs/internal/Versioning.md#transport-protocol
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we have to increment the transport version any time we change serialization.
A bit late to the party, how about |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just some minor comments.
"mapping_addition": { | ||
"properties": { | ||
"a.b": { | ||
"type": "keyword" | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Is it worth testing with mapping in index_template_substitutions
or component_template_substitutions
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it probably is.
@dakrone any strong preference? Otherwise I'll probably just go with what I've currently got (merge_type). |
I don't have a strong preference, I think |
If mapping overrides are given to the ingest simulate API (_ingest/_simulate) in the
mapping_addition
,index_template_substitutions
, orcomponent_template_substitutions
, they are currently merged in with existing mappings using MapperService.MergeReason.MAPPING_UPDATE when doing mapping validation. This simulates merging the mappings the way that they would be merged in to an existing index, rather than the way that they would be merged into templates. This can cause problems like #131608, where the mapping overrides cannot be merged in.This PR adds a new
merge_type
parameter that allows a user to specify exactly how they would like the mappings merged. The options areindex
ortemplate
. By default, they will be merged using theindex
strategy to maintain backwards compatibility.Closes #131608