Bug fix, last optimised value in ESUTF8StreamJsonParser kept old value.

[gmarouli]

In #134790 we introduced a bug that was caught by our tests.;

The problem manifested itself when a multi field mapper would call getText() which would set _tokenIncomplete to false after the getOptimisedText(). This would evaluate the condition _currToken == JsonToken.VALUE_STRING && _tokenIncomplete && stringEnd > 0 to false and the lastOptimisedValue would not be reset.

We changed the code to always reset the lastOptimisedValue when a next*() method is called.

Furthermore, we introduced a randomised unit test that creates two XContentParsers and runs one as a baseline using none of the optimised code and the other one is accessing both optimised and non optimised in a random pattern. This test was able to catch both #134770 & #135256.

Fixes #135256

[elasticsearchmachine]

Hi @gmarouli, I've created a changelog YAML for you.

[elasticsearchmachine]

Pinging @elastic/es-core-infra (Team:Core/Infra)

[martijnvg]

Thanks Mary!

I think you also need to undo a part #135172 did: https://github.com/elastic/elasticsearch/pull/135172/files#diff-8ef1741dbe67a9d21e8154754d17238802a9b21344f997067befe4f618f985feR145 ?

I think it would be great if we can get a unit test that reproduces the issue. For example a json document that we know caused the logsdb randomized tests to fail. If we need a DotExpandingXContentParser to reproduce that should be ok. We should reproduce a the bug, which is that an incorrect value is returned for a specific field.

I think the unit test coverage needs to be improved. Maybe we should create a unit test that parses a random json document. First with a parser that never uses the uff-8 parsing optimization and secondly with a parser that uses the utf-8 parsing optimization. Both parsers should produce the exact same result. (I think we can let both parsers something like a map of maps). In order to make it more evil, we can randomly invoke text() or optimizedText() multiple times.

[elasticsearchmachine]

Hi @gmarouli, I've updated the changelog YAML for you.

[gmarouli]

Changed to non-issue to remove the changelog.

[martijnvg]

This looks good @gmarouli! I left a few comments around testing.

Maybe also add a yaml test similar to reproduction in #135256?

[gmarouli]

Thanks for the feedback @martijnvg , it is much simpler now and I learned a few extra things. I extended the existing test to capture both bugs we encountered.

[martijnvg]

Left one testing suggestion, otherwise LGTM.

[justzh]

Ok. First, you guys are making way too many changes in the span of 4 - 5 days. Second of all...

[justzh]

#132306

[gmarouli]

Ok. First, you guys are making way too many changes in the span of 4 - 5 days. Second of all...

Hi @justzh , indeed we have been very active on this part of the code, happy to walk you through this.

• Recently, we discovered a bug in the optimised path.
• I tried to fix it in Bug fix: Facilitate second retrieval of the same value (#134770) #134790, but unfortunately, I caused a different bug that was caught by our test 3 days later.
• @martijnvg disabled the optimised path to ensure the optimised code will not be released (Disable utf-8 parsing optimization #135172).
• This PR fixes the bug, enhances our testing and enabled the optimised path again.

We are trying to ensure this will be in 9.2 so our users will not "miss" the optimisation's benefits. This is why we have been so active. Does it make sense?

If you have any feedback on our approach on handling this bug, I am very happy to hear.

[gmarouli]

#132306

This is unrelated.

[elasticsearchmachine]

💔 Backport failed

Status	Branch	Result
❌	8.19	Commit could not be cherrypicked due to conflicts
❌	9.1	Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 135184