Skip to content

Fix synthetic source issue with deeply nested ignored source fields #121715

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.

Sign up for GitHub

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

Jump to bottom
Merged
lkts merged 2 commits into from
Feb 5, 2025
Merged

Fix synthetic source issue with deeply nested ignored source fields #121715

lkts merged 2 commits into from
Feb 5, 2025

Conversation

@lkts
Copy link
Contributor

@lkts lkts commented Feb 4, 2025 •
edited
Loading

This is an accidental discovery from new block loader tests.

Randomized tests will eventually catch this but it is rare. It requires:

  1. Deep-ish object hierarchy
  2. No other synthetic field loaders being present in children of the last object. E.g. in this case if the field had doc values, it would not reproduce.

The problem is that ignoredValuesPresent signals when any children have ignored values. Existing this.ignoredValues != null check is weaker and only signals when immediate children have ignored values. This causes an erroneous decision that there are no ignored values in the hierarchy.

@lkts lkts added >bug auto-backport Automatically create backport pull requests when merged :StorageEngine/Mapping The storage related side of mappings v9.0.0 v8.17.2 v8.16.4 v8.18.1 v8.19.0 labels Feb 4, 2025
@lkts lkts requested review from kkrik-es and martijnvg February 4, 2025 22:03
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Feb 4, 2025

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

@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Feb 4, 2025

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

jordan-powers
jordan-powers approved these changes Feb 4, 2025
View reviewed changes
Copy link
Contributor

@jordan-powers jordan-powers left a comment

Choose a reason for hiding this comment

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

Good catch, LGTM

@lkts lkts merged commit 2ad64e1 into elastic:main Feb 5, 2025
17 checks passed
@lkts lkts deleted the synthetic_source_deep_fix branch February 5, 2025 16:22
@lkts lkts mentioned this pull request Feb 5, 2025
Merged
lkts added a commit to lkts/elasticsearch that referenced this pull request Feb 5, 2025
@lkts
Fix synthetic source issue with deeply nested ignored source fields (e…
6d4a69c 
…lastic#121715)

* Fix synthetic source issue with deeply nested ignored source fields

* Update docs/changelog/121715.yaml
This was referenced Feb 5, 2025
Merged
Merged
Merged
Merged
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Feb 5, 2025

💚 Backport successful

Status Branch Result
✅ 9.0
✅ 8.17
✅ 8.16
✅ 8.18
✅ 8.x

lkts added a commit to lkts/elasticsearch that referenced this pull request Feb 5, 2025
@lkts
Fix synthetic source issue with deeply nested ignored source fields (e…
895a738 
…lastic#121715)

* Fix synthetic source issue with deeply nested ignored source fields

* Update docs/changelog/121715.yaml
lkts added a commit to lkts/elasticsearch that referenced this pull request Feb 5, 2025
@lkts
Fix synthetic source issue with deeply nested ignored source fields (e…
627e898 
…lastic#121715)

* Fix synthetic source issue with deeply nested ignored source fields

* Update docs/changelog/121715.yaml
lkts added a commit to lkts/elasticsearch that referenced this pull request Feb 5, 2025
@lkts
Fix synthetic source issue with deeply nested ignored source fields (e…
a70d4ee 
…lastic#121715)

* Fix synthetic source issue with deeply nested ignored source fields

* Update docs/changelog/121715.yaml
lkts added a commit to lkts/elasticsearch that referenced this pull request Feb 5, 2025
@lkts
Fix synthetic source issue with deeply nested ignored source fields (e…
0b8125a 
…lastic#121715)

* Fix synthetic source issue with deeply nested ignored source fields

* Update docs/changelog/121715.yaml
elasticsearchmachine pushed a commit that referenced this pull request Feb 5, 2025
@lkts
Fix synthetic source issue with deeply nested ignored source fields (#…
c9f0d93 
…121715) (#121792)

* Fix synthetic source issue with deeply nested ignored source fields

* Update docs/changelog/121715.yaml
elasticsearchmachine pushed a commit that referenced this pull request Feb 5, 2025
@lkts
Fix synthetic source issue with deeply nested ignored source fields (#…
a8eb81a 
…121715) (#121788)

* Fix synthetic source issue with deeply nested ignored source fields

* Update docs/changelog/121715.yaml
elasticsearchmachine pushed a commit that referenced this pull request Feb 5, 2025
@lkts
Fix synthetic source issue with deeply nested ignored source fields (#…
3854eb2 
…121715) (#121791)

* Fix synthetic source issue with deeply nested ignored source fields

* Update docs/changelog/121715.yaml
elasticsearchmachine pushed a commit that referenced this pull request Feb 5, 2025
@lkts
Fix synthetic source issue with deeply nested ignored source fields (#…
747663d 
…121715) (#121789)

* Fix synthetic source issue with deeply nested ignored source fields

* Update docs/changelog/121715.yaml
elasticsearchmachine pushed a commit that referenced this pull request Feb 5, 2025
@lkts
[8.16] Fix synthetic source issue with deeply nested ignored source f…
28635f0 
…ields (#121715) (#121790)

* Fix synthetic source issue with deeply nested ignored source fields (#121715)

* Fix synthetic source issue with deeply nested ignored source fields

* Update docs/changelog/121715.yaml

* fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jordan-powers jordan-powers jordan-powers approved these changes

@kkrik-es kkrik-es kkrik-es approved these changes

@martijnvg martijnvg Awaiting requested review from martijnvg

Assignees

No one assigned

Labels

auto-backport Automatically create backport pull requests when merged >bug :StorageEngine/Mapping The storage related side of mappings Team:StorageEngine v8.16.4 v8.17.2 v8.18.1 v8.19.0 v9.0.0 v9.1.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@lkts @elasticsearchmachine @jordan-powers @kkrik-es