Skip to content

Move mapper merging logic to Mapper.Builder#142754

Merged
romseygeek merged 33 commits intoelastic:mainfrom
romseygeek:mapper/builder-merge
Feb 23, 2026
Merged

Move mapper merging logic to Mapper.Builder#142754
romseygeek merged 33 commits intoelastic:mainfrom
romseygeek:mapper/builder-merge

Conversation

@romseygeek
Copy link
Copy Markdown
Contributor

This is the first step for #142753

It's a pretty large and complex PR, which unfortunately is tricky to break down into smaller pieces. Here's a summary of what it contains:

  • a new MappingBuilder object that holds Builders for a RootObjectMapper and various MetadataFieldMappers. This handles all merging, meaning that the merge methods on Mapping and Mapper are removed
  • MapperService takes input merges as CompressedXContent rather than parsed Mapping objects
  • MappingParser returns a MappingBuilder rather than a Mapping
  • DocumentParser switches to use Builders to build dynamic mappings
  • The various Builder classes for each field type are updated to add a contentType() method, to be used for error messages in merging.
  • MetadataFieldMappers now all have Builders
  • ObjectMapper merging is all done on the Builder, which is mostly just moving code around but adds some nice simplifications as well.

@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Member

@martijnvg martijnvg left a comment

Choose a reason for hiding this comment

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

Thanks @romseygeek, this on its own is a good refactoring. Looking forward when all the pieces of #142753 are in.

// and it should not be reassigned below.
// For now it is still possible to set `_source.mode` so this is correct.
boolean isSourceSynthetic = SourceFieldMapper.isSynthetic(mappingParserContext.getIndexSettings());
boolean isDataStream = false;
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.

nice to see this now being better handled in MappingBuilder

@romseygeek
Copy link
Copy Markdown
Contributor Author

CI is failing due to an unrelated failure that is failing on a bunch of other PRs as well, so this is good to merge.

@romseygeek romseygeek merged commit d977ef9 into elastic:main Feb 23, 2026
33 of 35 checks passed
sidosera pushed a commit to sidosera/elasticsearch that referenced this pull request Feb 24, 2026
This is the first step for elastic#142753

It's a pretty large and complex PR, which unfortunately is tricky to break down into smaller pieces. Here's a summary of what it contains:

- A new MappingBuilder object that holds Builders for a RootObjectMapper and various MetadataFieldMappers. This handles all merging, meaning that the merge methods on Mapping and Mapper are removed
- MapperService takes input merges as CompressedXContent rather than parsed Mapping objects
- MappingParser returns a MappingBuilder rather than a Mapping
- DocumentParser switches to use Builders to build dynamic mappings
- The various Builder classes for each field type are updated to add a contentType() method, to be used for error messages in merging.
- MetadataFieldMappers now all have Builders
- ObjectMapper merging is all done on the Builder, which is mostly just moving code around but adds some nice simplifications as well.
mayya-sharipova added a commit to mayya-sharipova/elasticsearch that referenced this pull request Mar 2, 2026
PR elastic#130540 introduced a bug where indexing a
document with a dotted field name (e.g. "my_vectors.vector1") matched
by a dynamic template for dense_vector or rank_vectors without
explicit dims would fail with:
IllegalStateException: Missing intermediate object.

The bug only manifests when dims are not set in the mapping or
template, triggering the code path that dynamically determines
dimensions from the first indexed document.

The root cause is that when DenseVectorFieldMapper and
RankVectorsFieldMapper dynamically determine dims, they rebuild the
mapper using the document parser's content path. For dynamically
mapped fields with dotted names, the content path includes the field
name itself rather than just the parent object path, producing a
duplicated full path in the rebuilt mapper.

This was fixed on main/9.4 by PR elastic#142754, which refactored dynamic
mapper tracking to use Mapper.Builder objects. This commit provides a
targeted fix for 9.3.x by deriving the correct parent path from the
existing mapper's fullPath instead of relying on the content path.

The same fix is applied to both DenseVectorFieldMapper and
RankVectorsFieldMapper, which shared the same bug.
mayya-sharipova added a commit that referenced this pull request Mar 3, 2026
PR #130540 introduced a bug where indexing a
document with a dotted field name (e.g. "my_vectors.vector1") matched
by a dynamic template for dense_vector or rank_vectors without
explicit dims would fail with:
IllegalStateException: Missing intermediate object.

The bug only manifests when dims are not set in the mapping or
template, triggering the code path that dynamically determines
dimensions from the first indexed document.

The root cause is that when DenseVectorFieldMapper and
RankVectorsFieldMapper dynamically determine dims, they rebuild the
mapper using the document parser's content path. For dynamically
mapped fields with dotted names, the content path includes the field
name itself rather than just the parent object path, producing a
duplicated full path in the rebuilt mapper.

This was fixed on main/9.4 by PR #142754, which refactored dynamic
mapper tracking to use Mapper.Builder objects. This commit provides a
targeted fix for 9.3.x by deriving the correct parent path from the
existing mapper's fullPath instead of relying on the content path.

The same fix is applied to both DenseVectorFieldMapper and
RankVectorsFieldMapper, which shared the same bug.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants