Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix regression with parsing of scalar value documents #90

Merged

Conversation

tobil4sk
Copy link
Contributor

This fixes a regression from 0f609dd where lua-simdjson switched to the ondemand parser. The ondemand::document type refuses to be convert to ondemand::value if the document is a single scalar value, e.g. null, 10, hello or true.

It would give this error:

SCALAR_DOCUMENT_AS_VALUE: A JSON document made of a scalar (number, Boolean, null or string) is treated as a value. Use get_bool(), get_double(), etc. on the document instead.

I've also added relevant test cases.

With the ondemand parser, the document to value conversion fails with
the error SCALAR_DOCUMENT_AS_VALUE if the document is a single null,
string, bool, or number value.
Since we are now using a template type, this is no longer inferable by
an IDE. Adding explicit types also reduces the number of `.value()`
conversions required.
@tobil4sk
Copy link
Contributor Author

This solution seems like a bit of a hack, but it seems to be the way to have the least code changes in luasimdjson.cpp.

@FourierTransformer
Copy link
Owner

@tobil4sk How could it be less of a hack? My C++ isn't the best. I'm willing to dig in (or at least create an issue for the future) if you had any suggestions.

@tobil4sk
Copy link
Contributor Author

I'm not sure there is another way of doing this at the moment, unless we just copy the function implementation to have one for documents and one for values, however, that is worse than using the template.

I've tried to look a bit into what other bindings are trying to do, though not many have switched to the ondemand parser yet. cysimdjson has a PR open which has duplicate implementations for document and value: https://github.com/TeskaLabs/cysimdjson/pull/13/files#diff-083a29ee978c1780e06e7653801bd953dfb9e92ce0ee95155534b51428f355a9.

This might be something that could be improved on simdjson's side, so maybe it is more useful to open an issue there.

@FourierTransformer
Copy link
Owner

Ahh I see. Well, I'll get this merged for now. I'll probably also update to 3.10.1 before cutting a new release.

@FourierTransformer FourierTransformer merged commit 789d08a into FourierTransformer:master Sep 16, 2024
23 checks passed
@tobil4sk
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants