Skip to content

Store source for fields in objects with dynamic override#108911

Merged
elasticsearchmachine merged 10 commits intoelastic:mainfrom
kkrik-es:fix/synthetic-source/no-dynamic
May 27, 2024
Merged

Store source for fields in objects with dynamic override#108911
elasticsearchmachine merged 10 commits intoelastic:mainfrom
kkrik-es:fix/synthetic-source/no-dynamic

Conversation

@kkrik-es
Copy link
Copy Markdown
Member

Covers setting dynamic to false or runtime.

Related to #106825, #108417

@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

Hi @kkrik-es, I've created a changelog YAML for you.

@kkrik-es
Copy link
Copy Markdown
Member Author

@elasticsearchmachine update branch

@kkrik-es kkrik-es requested review from lkts and martijnvg May 23, 2024 09:14
@kkrik-es kkrik-es marked this pull request as ready for review May 23, 2024 09:14
@elasticsearchmachine
Copy link
Copy Markdown
Collaborator

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


private static void parseArrayDynamic(DocumentParserContext context, String currentFieldName) throws IOException {
ensureNotStrict(context, currentFieldName);
if (context.dynamic() == ObjectMapper.Dynamic.FALSE) {
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I sent #108912 for this change, it's orthogonal.

kkrik-es added 4 commits May 23, 2024 18:42
# Conflicts:
#	server/src/main/java/org/elasticsearch/index/mapper/DocumentParser.java
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.

👍

&& fieldMapper.syntheticSourceMode() == FieldMapper.SyntheticSourceMode.FALLBACK;
if (storeArraySourceEnabled || fieldWithFallbackSyntheticSource || mapper instanceof NestedObjectMapper) {
if (context.canAddIgnoredField()) {
if ((mapper instanceof ObjectMapper objectMapper
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: i do prefer named booleans here

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not a huge fan but happy to entertain your preference.

// field defined as runtime field, don't index anything
// Run-time fields are mapped to this mapper, so it needs to handle storing values for use in synthetic source.
// #parseValue calls this method once the run-time field is created.
if (context.dynamic() == ObjectMapper.Dynamic.RUNTIME && context.canAddIgnoredField()) {
Copy link
Copy Markdown
Contributor

@lkts lkts May 24, 2024

Choose a reason for hiding this comment

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

It is not clear to me which code path leads to this and does not have source already stored at DocumentParser level. E.g. if canAddIgnoredField is true we would have already stored source inside parseObjectDynamic right after creating this mapper, meaning canAddIgnoredField is always false here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Took me a while to capture this.. When a new runtime field is created, we go through the path in parseDynamicValue in the master. Then, in the data node, the forwarded mapping contains the runtime field so this code executes; parseValue calls parseObjectOrField with this mapper, instead of parseDynamicValue.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks.

@kkrik-es kkrik-es added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 27, 2024
@elasticsearchmachine elasticsearchmachine merged commit b1d54e6 into elastic:main May 27, 2024
@kkrik-es kkrik-es deleted the fix/synthetic-source/no-dynamic branch May 27, 2024 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) >enhancement :StorageEngine/Mapping The storage related side of mappings Team:StorageEngine v8.15.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants