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

Type coercion for numerical comparison #37

Closed
joeworkman opened this issue Jul 24, 2018 · 7 comments · May be fixed by #38
Closed

Type coercion for numerical comparison #37

joeworkman opened this issue Jul 24, 2018 · 7 comments · May be fixed by #38

Comments

@joeworkman
Copy link

joeworkman commented Jul 24, 2018

If you want to store float values in JSON, you must store them as string. Therefore, to make my life easier, I simply store all numbers as strings in my JSON.

I have not had any issues with this until now. I cannot seem to use the numerical comparisons in the jsql.

Data:

[{"popularity":"100"}, {"popularity":"50"}, {"popularity":"75"}]
[{"price":"5.95"}, {"price":"99.95"}, {"price":"49.95"}]

JSQL:

{popularity: {gt: 50}}
{price: {gt: 50}}

Please let me know if I am missing something, I would not doubt that.

@deitch
Copy link
Owner

deitch commented Jul 25, 2018

If you want to store float values in JSON, you must store them as string

I don't think that is true.

> s = require('searchjs')
> q = [{"popularity":100}, {"popularity":50.2}, {"popularity":75}]
[ { popularity: 100 }, { popularity: 50.2 }, { popularity: 75 } ]
> s.matchArray(q, {popularity: {gt: 50}})
[ { popularity: 100 }, { popularity: 50.2 }, { popularity: 75 } ]
> s.matchArray(q, {popularity: {gt: 52}})
[ { popularity: 100 }, { popularity: 75 } ]

Seems to work just fine.

@joeworkman
Copy link
Author

Yes. You are correct. I recall the root of the issue now. When encoding and decoding JSON in PHP, floats would not properly keep their decimal places. 4.95 would turn into 4.94999999999999999. This obviously is not acceptable. The only solution that I could find was to store numbers as strings.

I submitted a PR that completely resolves this issue. Its been working flawlessly in my tests so far. It just adds a simple isNaN test.

@deitch
Copy link
Owner

deitch commented Jul 27, 2018

I saw the PR. I am a bit torn about it. On the one hand, if it works around an issue with no collateral damage, then should be ok, and isNAN() does a decent job of teasing out strings-that-are-numbers from strings-that-are-not-numbers.

On the other hand, type coercion does defeat the very purpose of having, well, types. Well JS is not a strongly typed language (intentionally, if I recall correctly), it does have types that can be used.

If it were core to JS, I would agree without question (well, without questioning the fix, but would question the whole system that makes it necessary, but that would be JS's fault, not ours :-) . Since it is a problem with PHP, would a better design not be to have a translating clean-up layer?

Or, perhaps, fix the issue at its core? It appears to be a 'serialize_precision' issue. Look at the following:

Fails:

$ php -r 'ini_set('serialize_precision','1'); $price = ["price" => round("45.99", 2)]; echo json_encode($price);'
{"price":5.0e+1}

Succeeds:

$ php -r 'ini_set('serialize_precision','-1'); $price = ["price" => round("45.99", 2)]; echo json_encode($price);'

{"price":45.99}

Can you just use serialize_precision to "reset" back to pre-7.1.1 php setting?

@deitch
Copy link
Owner

deitch commented Jul 27, 2018

Also check the comment I am putting in now on the PR.

@joeworkman
Copy link
Author

Wow. You rock brother! I have never been aware of that ini setting. I will definitely be updating my app with this.

@deitch
Copy link
Owner

deitch commented Jul 28, 2018

Hope it works. :-)

@joeworkman
Copy link
Author

Sorry for the delay. It looks like that serialize_precision is working properly. You can close and my PR if you like...

@deitch deitch closed this as completed Aug 3, 2018
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 a pull request may close this issue.

2 participants