Use optimized text in match_only_text fields#129371
Merged
jordan-powers merged 8 commits intoelastic:mainfrom Jun 17, 2025
Merged
Use optimized text in match_only_text fields#129371jordan-powers merged 8 commits intoelastic:mainfrom
jordan-powers merged 8 commits intoelastic:mainfrom
Conversation
jordan-powers
added
>non-issue
auto-backport
Collaborator
|
Pinging @elastic/es-storage-engine (Team:StorageEngine) |
jordan-powers
commented
Jun 12, 2025
| /** | ||
| * Reader that decodes UTF-8 formatted bytes into chars. | ||
| */ | ||
| public class UTF8DecodingReader extends Reader { |
Contributor
Author
There was a problem hiding this comment.
This is basically equivalent to:
final var reader = new InputStreamReader(new ByteArrayInputStream(utfBytes.bytes(), utfBytes.offset(), utfBytes.length()), StandardCharsets.UTF_8);But according to the microbenchmarks, using the InputStreamReader/ByteArrayInputStream is really slow, and was actually slower than the original string-based implementation.
Contributor
Author
There was a problem hiding this comment.
Here are the results from the microbenchmark:
Benchmark (nDocs) Mode Cnt Score Error Units
OptimizedTextBenchmark.indexDocuments (baseline) 1048576 avgt 5 581.242 ± 3.050 ms/op
OptimizedTextBenchmark.indexDocuments (UTF8DecodingReader) 1048576 avgt 5 544.477 ± 4.961 ms/op
OptimizedTextBenchmark.indexDocuments (InputStreamReader) 1048576 avgt 5 852.380 ± 6.238 ms/op
Member
There was a problem hiding this comment.
Good catch. Yes, this implementation makes a lot of sense then. Did you also run this PR against elastic/logs (enterprise) benchmark?
Contributor
Author
There was a problem hiding this comment.
I haven't yet, but I plan to today
Member
There was a problem hiding this comment.
Maybe make this a final class?
jordan-powers
changed the title
7 tasks
martijnvg
reviewed
Jun 13, 2025
Member
martijnvg
left a comment
martijnvg
left a comment
There was a problem hiding this comment.
Looks good. I left a few questions.
| /** | ||
| * Reader that decodes UTF-8 formatted bytes into chars. | ||
| */ | ||
| public class UTF8DecodingReader extends Reader { |
Member
There was a problem hiding this comment.
Good catch. Yes, this implementation makes a lot of sense then. Did you also run this PR against elastic/logs (enterprise) benchmark?
...per-extras/src/main/java/org/elasticsearch/index/mapper/extras/MatchOnlyTextFieldMapper.java
Show resolved
Hide resolved
...per-extras/src/main/java/org/elasticsearch/index/mapper/extras/SourceConfirmedTextQuery.java
Show resolved
Hide resolved
martijnvg
approved these changes
Jun 16, 2025
...per-extras/src/main/java/org/elasticsearch/index/mapper/extras/MatchOnlyTextFieldMapper.java
Show resolved
Hide resolved
| /** | ||
| * Reader that decodes UTF-8 formatted bytes into chars. | ||
| */ | ||
| public class UTF8DecodingReader extends Reader { |
Member
There was a problem hiding this comment.
Maybe make this a final class?
Member
|
Looks like serverless check failed, because of: Which killed the search node and many tests failed because of this. I don't think it is related to this change. |
Contributor
Author
|
Ok, I benchmarked this with the In light of that, I'm not sure if we want to continue forward with this change. |
Member
Given that even median indexing throughput varies, I think that this is within noise. I don't think this change makes things slower. I suspect we don't see a gain here, given that that are many other moving parts here. The micro benchmark suggested a minor improvement. I think not having to create java strings during indexing for match only fields is a beneficial. Additionally block loaders for match only text fields can work with bytes refs instead of strings, which I think is beneficial as well.
So I think we can move forward with the change and merge it in. |
Contributor
Author
|
Ok, I'll merge it and keep an eye on the nightlies. We can always revert it if we see something we don't like. |
Collaborator
💚 Backport successful
|
elasticsearchmachine
pushed a commit
that referenced
this pull request
Jun 18, 2025
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
3 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.
Follow-up to #126492 to use the json parsing optimizations for
match_only_textfields.Relates to #129072.