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

Float aware mini #102

Closed
wants to merge 3 commits into from
Closed

Float aware mini #102

wants to merge 3 commits into from

Conversation

edgarsi
Copy link

@edgarsi edgarsi commented Nov 22, 2022

simdjson minify drops the trailing '.0' from floats, which is fine by JSON spec,
but matters in practice. For example, Elasticsearch dynamic field type detection
is affected. In general, Python distinguishes between int and float, so various
type guarantees may fail. The dump/load cycle should not convert types for a few
byte gain. Let users explicitly convert types, if they need to.

This modifies minify, so it does not drop the '.0'.

Note: simdjson started dropping '.0' with d0821adf0e7934f27a8eb5c2fe9b8254e4.

@edgarsi edgarsi closed this Nov 22, 2022
@edgarsi edgarsi reopened this Nov 22, 2022
@edgarsi
Copy link
Author

edgarsi commented Nov 22, 2022

Patching the library makes #80 no longer applicable.

@TkTech
Copy link
Owner

TkTech commented Nov 22, 2022

The d0821adf0e7934f27a8eb5c2fe9b8254e4's change seems odd. It was an intentional change by Lemire, but it's a lossy change. We might want to change (or at least make this configurable) upstream. @lemire?

@lemire
Copy link
Contributor

lemire commented Nov 22, 2022

See simdjson/simdjson#1921

@TkTech
Copy link
Owner

TkTech commented Nov 23, 2022

@edgarsi since it looks like this will get changed upstream, want to change this PR to your doc fixes and new tests? I'll update the bundled simdjson when fix is committed.

@edgarsi
Copy link
Author

edgarsi commented Nov 23, 2022

Let's see if it really gets changed upstream.

@lemire
Copy link
Contributor

lemire commented Nov 23, 2022

@edgarsi It will get changed. We just want to make sure that all tests are fine.

@lemire
Copy link
Contributor

lemire commented Nov 23, 2022

You could upgrade to 3.0.1.

@edgarsi
Copy link
Author

edgarsi commented Nov 23, 2022

See now. Btw, the change in this project, and in simdjson, feels like a breaking change. It's in questionable territory at least.

@TkTech
Copy link
Owner

TkTech commented Sep 3, 2023

Done in #110. As it may change behavior, it's behind a major version bump.

@TkTech TkTech closed this Sep 3, 2023
@TkTech TkTech mentioned this pull request Sep 3, 2023
4 tasks
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.

None yet

3 participants