Skip to content

Handling integers #245

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

Closed
hierozbosch opened this issue May 22, 2017 · 9 comments
Closed

Handling integers #245

hierozbosch opened this issue May 22, 2017 · 9 comments

Comments

@hierozbosch
Copy link

hierozbosch commented May 22, 2017

The handling of integers is referenced in previous issues (153, 122, 106, etc.). These issues have been closed and point to the Bolt driver methods (InSafeRange, int, etc.) as the resolution. However, using these methods implies hard-coding conversions against a known set of data entities, and doing it on the server. It's a highly inefficient solution: the driver should only convert 64-bit numbers to {high: low:} objects as a last resort. Converting all integers into these objects and then requiring a server to locate and re-convert most of them back into JS numbers is not an ideal solution. I'm using the neo4j-javascript-driver for a web app, and I would be very surprised if serious 64-bit applications considered JS their language of choice. In my case: a.) all my numbers are JS numbers, both in and out of neo4j; b.) I do not want to use my back-end neo4j server and front-line web servers to convert JS integers into objects and then convert those objects back into JS integers; and c.) I do not want to hard-code my server or client scripts to the granularities of a data model. I'm raising this as a new issue: I think the neo4j-JS-driver should have an optional directive to convert 64-bit integers directly to JS numbers and not waste time converting them all into objects. Thanks!

@lutovich
Copy link
Contributor

Hi @hierozbosch,

Thanks for reporting this.

I feel that converting to Integer objects as a last resort might be strange. Some numbers will then be regular javascript numbers and some will be Integers which could be even more confusing. You are right saying that we could have a switch like {useNativeNumbersOnly: boolean} but what should driver do if it receives a 64-bit number from the database? It is possible for multiple applications written in different languages to talk to the same database. Stored numbers could have any number of bits in such case.

All those mentioned issues and this one definitely show that Integer class with {high: ..., log: ...} does not feel very native and convenient. We'll think about improving number handing in upcoming releases.

@hierozbosch
Copy link
Author

Thanks Lutovich, Just by way of background, I've been running on neo4j for 5 years, since v 0.7. We use the graph for supply chain management--relationship tracking, access control, and social media stuff. We deal in smallish numbers--JS dates are probably the only time we exceed the ^32 range, and we don't see a case where we'd get over 2^53 with this app.

I put off the upgrade to neo4j 3.x for over a year due to the breakages, but I'm deep in it now. I had to change a lot of cypher, and almost all the result-handling. That's when the issue with the integers came up.

Looking at the driver sources briefly this a.m., it appears the data stream is coming into the driver with the integers already cut before it hits the 'packables' methods.

I guess what I'd suggest would be an optional flag that a user could inject when making the call to the driver, like: neo4j.run( query, params, JS_NUMBERS) ...

The packer would check that flag; if set, it would just pack and return a JS number. If it hits a big number with the flag set, have it throw an error-- INTEGER_OUT_OF_RANGE.

For some users, like me, that would be enough--not likely I would ever see that error, but if I did I would just remove the flag from that query and build in the number handling on a case-by-case basis.

That might help some users make the upgrade to Bolt easier. I'm already watching for errors on every query, of course, so no change there. I could get up and running without having to go through the entire code base looking for every instance where an integer comes out of the db, as I'm in the process of doing now.

Thanks!

@bryanph
Copy link

bryanph commented Jun 6, 2017

I think a common use case is writing only through the javascript-driver anyway. So any overflows will not occur, because there are no external writes causing integers bigger than 2^53.

You might also just simply return a string with the integer value and probably cause less memory then repeatedly initiating an Integer class. The Twitter API for example, simply provides an "id_str" field to account for overflows. This is why I proposed an option for simply returning string representations instead of instances of Int in #106

@hierozbosch
Copy link
Author

Thanks bryanph, yes, that's my use-case, too: all read/write via javascript-driver; therefore, no overflows above 2^53. I prefer an option on the query just to give me JS numbers--in fact I've already modified the driver to do this (https://stackoverflow.com/questions/44116453), but it would be better to have it supported. I'm not sure I understand the need for a "one-size-fits-all" on this particular issue, when neo4j offers options on many other things. Make it default to 64-bit if most people want that, so it's transparent for them.

@bryanph
Copy link

bryanph commented Jun 29, 2017

@lutovich @hierozbosch I don't mind casting the string to a javascript integer at a later time. In this case, you put the responsibility in the users hands. The user can check if the string represents a legal integer and do the casting explicitly whenever they want to display or use the string as a number. Or the user could use a library of their choice. In my opinion there is no good reason for this library to implement its own Integer class. It simply makes the library more difficult to use.

@bryanph
Copy link

bryanph commented Oct 26, 2017

I forked the repo and added an option "convertToString" to always return strings instead of a neo4j.int object. See bryanph@9f9d644

@lutovich
Copy link
Contributor

Hi @hierozbosch, @bryanph.

Please check #323 which makes it possible to create driver that only exposes native numbers.

@hierozbosch
Copy link
Author

hierozbosch commented Jan 17, 2018 via email

@lutovich
Copy link
Contributor

lutovich commented Feb 2, 2018

#323 is now merged. Change will be part of 1.6.0-alpha01.

@lutovich lutovich closed this as completed Feb 2, 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

No branches or pull requests

3 participants