Skip to content

[ML] Fix debug build for raw JSON splicing#2384

Merged
droberts195 merged 1 commit intoelastic:mainfrom
droberts195:fix_debug_build
Aug 4, 2022
Merged

[ML] Fix debug build for raw JSON splicing#2384
droberts195 merged 1 commit intoelastic:mainfrom
droberts195:fix_debug_build

Conversation

@droberts195
Copy link
Copy Markdown

In #2376 the results writer for inference was changed
to splice a preformatted cached section of the final result
document into an outer wrapper that varies per request.

The approach used worked when assertions were disabled but
tripped assertions in RapidJSON due to its internal counters
thinking that the spliced portion was a single value instead
of a key and value together.

This PR fixes the code when assertions are enabled by
taking advantage of the fact that the internal value
count of the RapidJSON writer is accessible to derived
classes, so we can add a method that adds a key and
value together and increments the counter twice.

In elastic#2376 the results writer for inference was changed
to splice a preformatted cached section of the final result
document into an outer wrapper that varies per request.

The approach used worked when assertions were disabled but
tripped assertions in RapidJSON due to its internal counters
thinking that the spliced portion was a single value instead
of a key and value together.

This PR fixes the code when assertions are enabled by
taking advantage of the fact that the internal value
count of the RapidJSON writer is accessible to derived
classes, so we can add a method that adds a key and
value together and increments the counter twice.
Copy link
Copy Markdown
Contributor

@dimitris-athanasiou dimitris-athanasiou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@droberts195 droberts195 merged commit 6c2aedd into elastic:main Aug 4, 2022
@droberts195 droberts195 deleted the fix_debug_build branch August 4, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants