Skip to content

Store keyword fields that trip ignore_above in binary doc values #137483

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
merged 8 commits into from
Nov 25, 2025
Merged

Store keyword fields that trip ignore_above in binary doc values #137483

merged 8 commits into from
Nov 25, 2025

Conversation

@Kubik42
Copy link
Contributor

@Kubik42 Kubik42 commented Oct 31, 2025 •
edited
Loading

@Kubik42 Kubik42 force-pushed the store-keyword-fields-in-binary-doc-values-ignore-above branch 3 times, most recently from 2a64367 to 8bf42f4 Compare November 5, 2025 01:00
@Kubik42 Kubik42 force-pushed the store-keyword-fields-in-binary-doc-values-ignore-above branch 2 times, most recently from 980e6c9 to ffc2498 Compare November 5, 2025 05:01
@Kubik42 Kubik42 marked this pull request as ready for review November 5, 2025 06:40
@elasticsearchmachine
Copy link
Collaborator

elasticsearchmachine commented Nov 5, 2025

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

@Kubik42
Copy link
Contributor Author

Kubik42 commented Nov 5, 2025 •
edited
Loading

The test is failing bc its expecting duplicate values, but with this change we're now deduplicating multi-valued fields in the same document. This is done for better storage efficiency.

We can either change the test or keep the duplicates.

martijnvg
martijnvg reviewed Nov 6, 2025
View reviewed changes
Copy link
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.

Looks good left a few comments.

@Kubik42 Kubik42 requested review from a team as code owners November 10, 2025 17:00
@Kubik42 Kubik42 force-pushed the store-keyword-fields-in-binary-doc-values-ignore-above branch from b074b90 to 3d83988 Compare November 10, 2025 17:01
@Kubik42 Kubik42 removed request for a team November 10, 2025 17:01
@Kubik42 Kubik42 force-pushed the store-keyword-fields-in-binary-doc-values-ignore-above branch from 3d83988 to a1d1d2a Compare November 10, 2025 17:02
martijnvg
martijnvg approved these changes Nov 11, 2025
View reviewed changes
Copy link
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.

Let's wait for compression of binary doc values to have been merged and its feature flag removed, otherwise LGTM

@parkertimmins parkertimmins merged commit 6783135 into elastic:main Nov 25, 2025
34 checks passed
ncordon pushed a commit to ncordon/elasticsearch that referenced this pull request Nov 26, 2025
@Kubik42 @ncordon
Store keyword fields that trip ignore_above in binary doc values (ela…
b7160da 
…stic#137483)

* Store keyword fields that trip ignore_above in binary doc values

* Addressed feedback

* Moved ignore values doc value field fetcher inside of existing fetcher function
jordan-powers
jordan-powers reviewed Dec 1, 2025
View reviewed changes
server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java
b.utf8Value(ref.bytes, ref.offset, ref.length);
}
});
layers.add(new BinaryDocValuesSyntheticFieldLoaderLayer(fieldName));
Copy link
Contributor

@jordan-powers jordan-powers Dec 1, 2025

Choose a reason for hiding this comment

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

Will this break BWC? Because old indices will have their ignored values stored in a stored field, and now the mapper can no longer load those values?

jordan-powers
jordan-powers reviewed Dec 1, 2025
View reviewed changes
server/src/main/java/org/elasticsearch/index/mapper/KeywordFieldMapper.java
Comment on lines -1156 to +1166
context.doc().add(new StoredField(fieldName, bytesRef));

// store the value in a binary doc values field, create one if it doesn't exist
MultiValuedBinaryDocValuesField field = (MultiValuedBinaryDocValuesField) context.doc().getByKey(fieldName);
if (field == null) {
field = new MultiValuedBinaryDocValuesField(fieldName);
context.doc().addWithKey(fieldName, field);
}
field.add(bytesRef);
Copy link
Contributor

@jordan-powers jordan-powers Dec 1, 2025

Choose a reason for hiding this comment

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

Another BWC issue - old indices will have conflicting fieldinfos for <name>._original since we're switching from using a stored field to binary doc values.

martijnvg added a commit to martijnvg/elasticsearch that referenced this pull request Dec 2, 2025
@martijnvg
Revert "Store keyword fields that trip ignore_above in binary doc val…
847fe51 
…ues (elastic#137483)"

This reverts commit 6783135.
martijnvg added a commit that referenced this pull request Dec 2, 2025
@martijnvg
Revert "Store keyword fields that trip ignore_above in binary doc val…
c6cdc01 
…ues (#137483)" (#138858)

This reverts commit 6783135.

Closes #138857
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 left review comments

@martijnvg martijnvg martijnvg approved these changes

Assignees

No one assigned

Labels

>enhancement >non-issue :StorageEngine/Mapping The storage related side of mappings Team:StorageEngine v9.3.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Kubik42 @elasticsearchmachine @martijnvg @jordan-powers @parkertimmins