Store keyword fields that trip ignore_above in binary doc values#139093
Merged
Kubik42 merged 8 commits intoelastic:mainfrom Dec 10, 2025
Merged
Store keyword fields that trip ignore_above in binary doc values#139093Kubik42 merged 8 commits intoelastic:mainfrom
Kubik42 merged 8 commits intoelastic:mainfrom
Conversation
Kubik42
added
Team:StorageEngine
:StorageEngine/Mapping
Kubik42
force-pushed
the
binary-doc-values-index-versions-check
branch
3 times, most recently
from
f13ce69 to
0f210d9
Compare
Collaborator
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
martijnvg
reviewed
Dec 5, 2025
...olling-upgrade/src/javaRestTest/java/org/elasticsearch/upgrades/KeywordRollingUpgradeIT.java
Outdated
Show resolved
Hide resolved
...olling-upgrade/src/javaRestTest/java/org/elasticsearch/upgrades/KeywordRollingUpgradeIT.java
Outdated
Show resolved
Hide resolved
| allValues.addAll(FIELD_VALUES_2); | ||
| search(allValues); | ||
| } else { | ||
| // skip - the cluster is upgrading, we don't really need to do anything here |
Member
There was a problem hiding this comment.
maybe also index and search after all nodes have been upgraded?
Contributor
Author
There was a problem hiding this comment.
Isn't that what the else if (isUpgradedCluster()) block is responsible for? In this test method:
- when the cluster is in the old cluster state (nothing has been upgraded), I create an index, index some documents, then run a search query
- when the cluster is in upgraded state (all nodes have been upgraded), I index some more documents, then run another search query and expect to find results from before and post the upgrade
- when the cluster is in mixed state (some nodes have been upgraded), I don't do anything
…lues (elastic#137483)" (elastic#138858) This reverts commit c6cdc01.
# Conflicts: # x-pack/plugin/logsdb/qa/legacy-rolling-upgrade/src/javaRestTest/java/org/elasticsearch/upgrades/KeywordRollingUpgradeIT.java
Kubik42
force-pushed
the
binary-doc-values-index-versions-check
branch
from
dc8ee86 to
19be8df
Compare
Kubik42
force-pushed
the
binary-doc-values-index-versions-check
branch
from
19be8df to
35281b0
Compare
martijnvg
approved these changes
Dec 9, 2025
Member
martijnvg
left a comment
martijnvg
left a comment
There was a problem hiding this comment.
I think there is some reuse possible with MultiValuedBinaryDocValuesField and BinaryDocValuesSyntheticFieldLoaderLayer with the high cardinality keyword field PR. I think that PR can reuse what this PR adds when this gets merged.
LGTM
| * A custom implementation of {@link org.apache.lucene.index.BinaryDocValues} that uses a {@link Set} to maintain a collection of unique | ||
| * binary doc values for fields with multiple values per document. | ||
| */ | ||
| private static final class MultiValuedBinaryDocValuesField extends CustomDocValuesField { |
Member
There was a problem hiding this comment.
Would be good if this implementation can be stream lined in the keyword high cardinality PR, so that we can endup just having one implementation.
Contributor
There was a problem hiding this comment.
I did merge the two implementations back before this was reverted, so probably something similar to that?
...ng-upgrade/src/javaRestTest/java/org/elasticsearch/xpack/logsdb/KeywordRollingUpgradeIT.java
Show resolved
Hide resolved
jordan-powers
added a commit
that referenced
this pull request
Dec 18, 2025
In #139093 we updated the NativeArrayIntegrationTestCase to extract the single value out of arrays of length 1. This prevents the creation of the .offsets field (as it's not need for single-valued fields), which then fails the later assertion in the test which checks for the presence of this .offsets field. This PR fixes the test failures by reverting the logic to collapse the arrays, instead maintaining single-valued arrays as-is.
jordan-powers
added a commit
to jordan-powers/elasticsearch
that referenced
this pull request
Jan 29, 2026
In elastic#139093 we updated the NativeArrayIntegrationTestCase to extract the single value out of arrays of length 1. This prevents the creation of the .offsets field (as it's not need for single-valued fields), which then fails the later assertion in the test which checks for the presence of this .offsets field. This PR fixes the test failures by reverting the logic to collapse the arrays, instead maintaining single-valued arrays as-is. (cherry picked from commit 5f24987) # Conflicts: # muted-tests.yml
jordan-powers
added a commit
that referenced
this pull request
Jan 30, 2026
In #139093 we updated the NativeArrayIntegrationTestCase to extract the single value out of arrays of length 1. This prevents the creation of the .offsets field (as it's not need for single-valued fields), which then fails the later assertion in the test which checks for the presence of this .offsets field. This PR fixes the test failures by reverting the logic to collapse the arrays, instead maintaining single-valued arrays as-is.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reverted the revert.
This PR contains the following additional changes:
KeywordRollingUpgradeIT- a BWC test containing a combination of values that do and do not exceed ignore_above