Skip UTF8 to UTF16 conversion during document indexing #126492
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
|
Ok, I figured out a way to avoid cloning the file. Basically the problem was that detecting the encoding may possibly consume some bytes from the beginning of the input if there is a BOM present, and the property keeping track of how many bytes had been consumed was internal to Also, looking at the API docs, it seems elasticsearch only supports UTF-8 inputs anyway. So maybe I don't need to check the character encoding? Although I don't know for certain that elasticsearch won't parse json from another source (such as a configuration file or something) that's not UTF-8 encoded, so I'll leave it in |
martijnvg
reviewed
Apr 16, 2025
| /** | ||
| * Class that holds either a UTF-16 String or a UTF-8 BytesRef, and lazily converts between the two. | ||
| */ | ||
| private static class RawString { |
There was a problem hiding this comment.
Possibly, but it would be a bit tough since records are immutable. The lazy conversion works by only setting the bytesValue or stringValue once they've been converted, which wouldn't work if the properties were final.
There was a problem hiding this comment.
Ok, I falsely assumed that making RawString's properties final would be straightforward. In that case turning this class into a record doesn't make sense.
| /** | ||
| * Method that will try to get underlying UTF-8 encoded bytes of the current string token. | ||
| * This is only a best-effort attempt; if there is some reason the bytes cannot be retrieved, this method will return null. | ||
| * Currently, this is only implemented for ascii-only strings that do not contain escaped characters. |
There was a problem hiding this comment.
I suspect we need to support escaped characters? I don't think we have a test for this. Maybe a rest test (yaml or java) that tries to index a escaped json string field using keyword field mapper?
There was a problem hiding this comment.
I'll definitely add a test.
I was thinking that support for escaped characters and/or possibly full unicode (not just ascii) could happen in a follow-up after this is merged. The current implementation does still function with such characters, it just won't be optimized (since this method will return null so the KeywordFieldMapper will fall back to getValueAsString()).
There was a problem hiding this comment.
The current implementation does still function with such characters, it just won't be optimized (since this method will return null so the KeywordFieldMapper will fall back to getValueAsString()).
That makes sense to me.
There was a problem hiding this comment.
++ it makes sense to have it in a follow-up, this PR is already big enough :)
The important thing is to test nothing is broken, and it takes the non-optimized path.
No it was actually 0.27% slower, but that's such a small change I think it's noise. Latency was much lower though. I plan to rerun the benchmarks with logsdb. Hopefully that shows some improvement to throughput. Baseline: Contender: |
|
Ok, ran another benchmark. This time a multi-node logsdb run, similar to the nightlies. It seems that in this run, cumulative indexing time, latency, service time, and indexing throughput all increased. I'm not sure why indexing time, latency, and service time increased. Maybe those values are just noisy? Baseline: Contender: |
jordan-powers
changed the title
jfreden
pushed a commit
to jfreden/elasticsearch
that referenced
this pull request
May 12, 2025
This PR is a precursor to elastic#126492. It does three things: - Move org.elasticsearch.common.text.Text from :server to org.elasticsearch.xcontent.Text in :libs:x-content. - Refactor the Text class to use a java-native ByteBuffer instead of the elasticsearch BytesReference. - Add the XContentString interface, with the Text class implementing that interface.
ldematte
reviewed
May 21, 2025
There was a problem hiding this comment.
@jordan-powers have you added the test on "support" escaped characters? (ensure it will fall back, not error?)
If so, this PR is gtg for me.
|
Yes, those tests exist. |
ldematte
approved these changes
May 22, 2025
jordan-powers
added a commit
that referenced
this pull request
Jun 4, 2025
This PR is a precursor to #126492. It does three things: 1. Move org.elasticsearch.common.text.Text from :server to org.elasticsearch.xcontent.Text in :libs:x-content. 2. Refactor the Text class to use a new EncodedBytes record instead of the elasticsearch BytesReference. 3. Add the XContentString interface, with the Text class implementing that interface. These changes were originally implemented in #127666 and #128316, however they were reverted in #128484 due to problems caused by the mutable nature of java ByteBuffers. This is resolved by instead using a new immutable EncodedBytes record.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Jun 4, 2025
This PR is a precursor to #126492. It does three things: 1. Move org.elasticsearch.common.text.Text from :server to org.elasticsearch.xcontent.Text in :libs:x-content. 2. Refactor the Text class to use a new EncodedBytes record instead of the elasticsearch BytesReference. 3. Add the XContentString interface, with the Text class implementing that interface. These changes were originally implemented in #127666 and #128316, however they were reverted in #128484 due to problems caused by the mutable nature of java ByteBuffers. This is resolved by instead using a new immutable EncodedBytes record. (cherry picked from commit de40ac4) # Conflicts: # server/src/main/java/org/elasticsearch/search/fetch/subphase/highlight/DefaultHighlighter.java # server/src/test/java/org/elasticsearch/common/xcontent/BaseXContentTestCase.java # server/src/test/java/org/elasticsearch/search/fetch/subphase/highlight/HighlightFieldTests.java # test/framework/src/main/java/org/elasticsearch/search/SearchResponseUtils.java
martijnvg
approved these changes
Jun 5, 2025
| } | ||
|
|
||
| boolean indexed = indexValue(context, value); | ||
| if (offsetsFieldName != null && context.isImmediateParentAnArray() && context.canAddIgnoredField()) { | ||
| if (indexed) { | ||
| context.getOffSetContext().recordOffset(offsetsFieldName, value); | ||
| context.getOffSetContext().recordOffset(offsetsFieldName, value.string()); |
There was a problem hiding this comment.
I think there is no need to convert to string here? UTF8Bytes already implements Comparable and the recordOffset method isn't bound to string, the provided value just needs to implement Comparable. This should save another unneeded conversation to java string?
There was a problem hiding this comment.
Makes sense to me.
While looking into this I found an issue with the implementation of UTF8Bytes#compareTo. It was delegating to ByteBuffer#compareTo, which was performing signed comparison. This was incorrect, and it needs to do unsigned comparison to perform correct lexicographical ordering of encoded UTF8 values. So I also fixed that.
| // convert to utf8 only once before feeding postings/dv/stored fields | ||
| final BytesRef binaryValue = new BytesRef(value); | ||
| if (fieldType().normalizer() != Lucene.KEYWORD_ANALYZER) { | ||
| String normalizedString = normalizeValue(fieldType().normalizer(), fullPath(), value.string()); |
There was a problem hiding this comment.
In a follow up we can explore normalizing to not relying on java string as return type here.
💚 Backport successful
|
elasticsearchmachine
pushed a commit
that referenced
this pull request
Jun 6, 2025
…9023) When parsing documents, we receive the document as UTF-8 encoded data which we then parse and convert the fields to java-native UTF-16 encoded Strings. We then convert these strings back to UTF-8 for storage in lucene. This patch skips the redundant conversion, instead passing lucene a direct reference to the received UTF-8 bytes when possible.
This was referenced Jun 6, 2025
ldematte
added a commit
to mosche/elasticsearch
that referenced
this pull request
Jun 10, 2025
commit e6b8a64 Author: Lorenzo Dematte <lorenzo.dematte@elastic.co> Date: Tue Jun 10 12:02:16 2025 +0200 PR comments commit ad0902e Author: Moritz Mack <mmack@apache.org> Date: Fri Jun 6 13:33:37 2025 +0200 Update ReproduceInfoPrinter to correctly print a reproduction for lucene / BC upgrade tests. Relates to ES-12005 commit 78b4168 Author: Alexander Spies <alexander.spies@elastic.co> Date: Fri Jun 6 11:37:53 2025 +0200 ESQL: Throw ISE instead of IAE for illegal block in page (elastic#128960) IAE gets reported as a 400 status code, but that's inappropriate when inconsistent pages are always bugs, and should be reported with a 500. Throw ISE instead. commit 29e68bd Author: Aurélien FOUCRET <aurelien.foucret@gmail.com> Date: Fri Jun 6 11:24:23 2025 +0200 [ES|QL] Fix test releases for telemetry. (elastic#128971) commit 1a76bc2 Author: Bogdan Pintea <bogdan.pintea@elastic.co> Date: Fri Jun 6 11:01:49 2025 +0200 ESQL: Workaround for RLike handling of empty lang pattern (elastic#128895) Lucene's `org.apache.lucene.util.automaton.Operations#getSingleton` fails with an Automaton for a `REGEXP_EMPTY` `RegExp`. This adds a workaround for that, to check the type of automaton before calling into that failing method. Closes elastic#128813 commit e24fd32 Author: Aurélien FOUCRET <aurelien.foucret@gmail.com> Date: Fri Jun 6 10:25:28 2025 +0200 [ES|QL] Enable the completion command as a tech preview feature (elastic#128948) commit 3f03775 Author: Niels Bauman <33722607+nielsbauman@users.noreply.github.com> Date: Fri Jun 6 09:00:24 2025 +0200 Remove non-test usages of `Metadata.Builder#putCustom` (elastic#128801) This removes all non-test usages of ``` Metadata.Builder.putCustom(String type, ProjectCustom custom) ``` And replaces it with appropriate calls to the equivalent method on `ProjectMetadata.Builder`. In most cases this _does not_ make the code project aware, but does reduce the number of deprecated methods in use. commit 330d127 Author: Niels Bauman <33722607+nielsbauman@users.noreply.github.com> Date: Fri Jun 6 08:07:59 2025 +0200 Make utility methods in `IndexLifecycleTransition` project-aware (elastic#128930) Modifies the methods to work with a project scope rather than a cluster scope. This is part of an iterative process to make ILM project-aware. commit 1b5720d Author: Aurélien FOUCRET <aurelien.foucret@gmail.com> Date: Fri Jun 6 07:50:52 2025 +0200 [ES|QL] Fix test releases for LookupJoinTypesIT. (elastic#128985) commit 40cf2d3 Author: Tim Vernum <tim@adjective.org> Date: Fri Jun 6 13:09:31 2025 +1000 Add "extension" attribute validation to IdP SPs (elastic#128805) This extends the change from elastic#128176 to validate the "custom attributes" on a per Service Provider basis. Each Service Provider (whether registered or wildcard based) has a field "attributes.extensions" which is a list of attribute names that may be provided by the caller of "/_idp/saml/init". Service Providers that have not be configured with extension attributes will reject any custom attributes in SAML init. This necessitates a new field in the service provider index (but only if the new `extensions` attribute is set). The template has been updated, but there is no data migration because the `saml-service-provider` index does not exist in any of the environments into which we wish to deploy this change. commit 496fb2d Author: Jordan Powers <jordan.powers@elastic.co> Date: Thu Jun 5 19:50:09 2025 -0700 Skip UTF8 to UTF16 conversion during document indexing (elastic#126492) When parsing documents, we receive the document as UTF-8 encoded data which we then parse and convert the fields to java-native UTF-16 encoded Strings. We then convert these strings back to UTF-8 for storage in lucene. This patch skips the redundant conversion, instead passing lucene a direct reference to the received UTF-8 bytes when possible. commit c34f8b6 Author: Tim Vernum <tim@adjective.org> Date: Fri Jun 6 12:03:25 2025 +1000 Improve cache invalidation in IdP SP cache (elastic#128890) The Identity Provider's Service Provider cache had two issues: 1. It checked for identity based on sequence numbers, but didn't include the `seq_no_primary_term` parameter on searches, which means the sequence would always by `-2` 2. It didn't track whether the index was deleted, which means it could be caching values from an old version of the index This commit fixes both of these issues. In practice neither issue was a problem because there are no deployments that use index-based service providers, however the 2nd issue did cause some challenges for testing. commit 923f029 Author: Nhat Nguyen <nhat.nguyen@elastic.co> Date: Thu Jun 5 18:09:58 2025 -0700 Fix block loader with missing ignored source (elastic#129006) We miss appending null when ignored_source is not available. Our randomized tests already cover this case, but we do not check it when loading fields. I labelled this non-issue for an unreleased bug. Closes elastic#128959 Relates elastic#119546 commit 0f8178a Author: Bogdan Pintea <bogdan.pintea@elastic.co> Date: Fri Jun 6 02:02:24 2025 +0200 ESQL: Forward port 8.19 RegexMatch serialization change version (elastic#128979) Fwd port ESQL_REGEX_MATCH_WITH_CASE_INSENSITIVITY_8_19. Related: elastic#128919. commit ee716f1 Author: Simon Chase <simon.chase@elastic.co> Date: Thu Jun 5 15:20:08 2025 -0700 transport: edit TransportConnectionListener for close exceptions (elastic#129015) The TransportConnectionListener interface has previously included the Transport.Connection being closed and unregistered in its onNodeDisconnected callback. This is not in use, and can be removed as it is also available in the onConnectionClosed callback. It is being replaced with a Nullable exception that caused the close. This is being used in pending work (ES-11448) to differentiate network issues from node restarts. Closes ES-12007 commit aceaf23 Merge: f18f4ee 159c57f Author: elasticsearchmachine <infra-root+elasticsearchmachine@elastic.co> Date: Thu Jun 5 19:27:54 2025 +0000 Merge patch/serverless-fix into main commit 159c57f Author: Rene Groeschke <rene@elastic.co> Date: Tue Jun 3 11:56:27 2025 +0200 Prepare serverless patch including elastic#128784 elastic#128740 (elastic#128807) * Change default for vector.rescoring.directio to false (elastic#128784) On serverless (and potentially elsewhere), direct IO is not available, which can cause BBQ shards to fail to read with org.apache.lucene.CorruptIndexException when this setting is true. * Optimize sparse vector stats collection (elastic#128740) This change improves the performance of sparse vector statistics gathering by using the document count of terms directly, rather than relying on the field name field to compute stats. By avoiding per-term disk/network reads and instead leveraging statistics already loaded into leaf readers at index opening, we expect to significantly reduce overhead. Relates to elastic#128583 --------- Co-authored-by: Dave Pifke <dave.pifke@elastic.co> Co-authored-by: Jim Ferenczi <jim.ferenczi@elastic.co>
valeriy42
pushed a commit
to valeriy42/elasticsearch
that referenced
this pull request
Jun 12, 2025
When parsing documents, we receive the document as UTF-8 encoded data which we then parse and convert the fields to java-native UTF-16 encoded Strings. We then convert these strings back to UTF-8 for storage in lucene. This patch skips the redundant conversion, instead passing lucene a direct reference to the received UTF-8 bytes when possible.
jordan-powers
added a commit
that referenced
this pull request
Jun 12, 2025
Follow-up to #126492 to apply the json parsing optimization to strings containing unicode characters and some backslash-escaped characters. Supporting backslash-escaped strings is tricky as it requires modifying the string. There are two types of modification: some just remove the backslash (e.g. \", \\), and some replace the whole escape sequence with a new character (e.g. \n, \r, \u00e5). In this implementation, the optimization only supports the first case--removing the backslash. This is done by making a copy of the data, skipping the backslash. It should still be more optimized than full String decoding, but it won't be as fast as non-backslashed strings where we can directly reference the input bytes. Relates to #129072.
elasticsearchmachine
pushed a commit
that referenced
this pull request
Jun 12, 2025
…129360) Follow-up to #126492 to apply the json parsing optimization to strings containing unicode characters and some backslash-escaped characters. Supporting backslash-escaped strings is tricky as it requires modifying the string. There are two types of modification: some just remove the backslash (e.g. \", \\), and some replace the whole escape sequence with a new character (e.g. \n, \r, \u00e5). In this implementation, the optimization only supports the first case--removing the backslash. This is done by making a copy of the data, skipping the backslash. It should still be more optimized than full String decoding, but it won't be as fast as non-backslashed strings where we can directly reference the input bytes. Relates to #129072.
jordan-powers
added a commit
that referenced
this pull request
Jun 17, 2025
elasticsearchmachine
pushed a commit
that referenced
this pull request
Jun 18, 2025
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.
Initial implementation of avoiding the UTF8 to UTF16 back to UTF8 conversion that currently happens when parsing JSON.
Currently, the optimization only applies for ASCII-only strings that do not contain escaped characters (f.e.
\").Also, the optimization is currently only implemented for keyword fields.
The core logic to return the underlying bytes is implemented in
ESUTF8StreamJsonParser. The other "ES" versions of Jackson classes are just the glue logic required to make Jackson create instances ofESUTF8StreamJsonParserinstead of the superclassUTF8StreamJsonParser.I ran the prototype in esbench using the logging-indexing challenge on a standard-mode index, and I got these results:
Baseline:
Contender: