Fix binary docvalue_fields with padding #70826
Merged
Conversation
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
mayya-sharipova
added
the
:Search/Search
|
Pinging @elastic/es-search (Team:Search) |
mayya-sharipova
added
v8.0.0
v7.13.0
>bug
and removed
Team:Search
mayya-sharipova
force-pushed
the
binary-fields_doc_values
branch
from
2a4599a to
ffbbc0c
Compare
Previously docvalue_fields for binary values with paddings did not output padding. We consider it to be a bug because: 1) es would not be able parse these values 2) output from source filtering and fields API is different and does output padding. This patches fixes this by outputing padding for binary docvalue_fields where it is present. Closes elastic#70244
mayya-sharipova
force-pushed
the
binary-fields_doc_values
branch
from
ffbbc0c to
0f5de93
Compare
nik9000
reviewed
Mar 26, 2021
rest-api-spec/src/main/resources/rest-api-spec/test/search/350_binary_field.yml
Show resolved
Hide resolved
nik9000
reviewed
Mar 26, 2021
server/src/test/java/org/elasticsearch/index/mapper/BinaryFieldMapperTests.java
Outdated
Show resolved
Hide resolved
| if (rarely()) { | ||
| return null; | ||
| } else { | ||
| byte[] value = randomAlphaOfLengthBetween(1, 30).getBytes(StandardCharsets.UTF_8); |
There was a problem hiding this comment.
I think it'd be nice to also try the case where these aren't utf-8 bytes. Just some byte array of pure random stuff.
There was a problem hiding this comment.
Great comment! I guess it makes sense just always use some random bytes, I guess this is what Binary field is intended for – to store random binary values.
There was a problem hiding this comment.
Addressed in 3aa1286
|
Thanks for iterating! I left a request to add one more case. We have a surprising amount of code that things all byte arrays are utf-8 characters. I'm sort of hoping to create a trap for code like that. |
24 tasks
mayya-sharipova
added
the
:Search Foundations/Mapping
mayya-sharipova
removed
the
:Search/Search
mayya-sharipova
added a commit
to mayya-sharipova/elasticsearch
that referenced
this pull request
Mar 29, 2021
Previously docvalue_fields for binary values with paddings did not output padding. We consider it to be a bug because: 1) es would not be able parse these values 2) output from source filtering and fields API is different and does output padding. This patches fixes this by outputing padding for binary docvalue_fields where it is present. Closes elastic#70244 Backport for elastic#70826
mayya-sharipova
added a commit
that referenced
this pull request
Mar 29, 2021
Previously docvalue_fields for binary values with paddings did not output padding. We consider it to be a bug because: 1) es would not be able parse these values 2) output from source filtering and fields API is different and does output padding. This patches fixes this by outputing padding for binary docvalue_fields where it is present. Closes #70244 Backport for #70826
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Previously docvalue_fields for binary values with paddings did not
output padding. We consider it to be a bug because: 1) es would
not be able parse these values 2) output from source filtering
and fields API is different and does output padding.
This patches fixes this by outputing padding for binary
docvalue_fields where it is present.
Closes #70244