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

Wouldn't String be a better serialization format for BigInteger and BigDecimal? #7

Closed
christian-lechner opened this issue Apr 18, 2018 · 4 comments
Assignees
Labels
Milestone

Comments

@christian-lechner
Copy link

Hi!

I saw that BigInteger is serialized as long and BigDecimal as double. But they can easily overflow long and double (that's why they exist ;)). Wouldn't it be better to serialize both as String?

Greetings,
Christian

@mvollmary
Copy link

Hi @christian-lechner,

Yes that's right. I am not 100% satisfied with the current solution. At the time of the initial implementation I was guided by Jackson, which also serialize BigInteger and BigDecimal as numbers.

On the one hand, this is not really a big deal because you can just register a custom VPackSerializer when you need to serialize them as String, on the other hand we might want to offer a more convenient way to change the serialized type for BigInteger and BigDecimal.

A quick search on google shows what possibilities you have with Jackson.

  • converting the type in the getter (no option for us because we're not using setter/getter for serialization)
  • use a custom serializer by registering a module (which we can already do)
  • use a custom serializer by using annotation JsonSerialize (providing that kind of annotation is already on my todo list)

I am open to suggestions. Should we change the default to String or just document how the default behavior is and how to easily change it with a custom VPackSerializer.

What we can do in any case is to make the deserializer for BigInteger and BigDecimal compatible with both number and String.

best
Mark

@christian-lechner
Copy link
Author

christian-lechner commented Apr 18, 2018

That's not an easy question. But considering that the reason why BigInteger and BigDecimal exist is that they can store much larger and more accurate numbers, then we have to ask if it makes sense to reduce them to long and double. Doesn't this make the use of both senseless?

What we can do in any case is to make the deserializer for BigInteger and BigDecimal compatible with both number and String.

I totally agree with that.

@mvollmary
Copy link

My first point against this change was that you can't work properly in AQL with this anymore, but then I realized that you can type cast it in AQL with TO_NUMBER(value) but then you will have data loss. But that same argument is true when we save them as long and double :-)

So I think, from the point of avoiding data loss, we should serialize it as String 👍

With the change on the deserializers I also see no real pain point with backward compatibility.

@mvollmary mvollmary self-assigned this Apr 19, 2018
@mvollmary mvollmary added the bug label Apr 19, 2018
@mvollmary mvollmary added this to the 1.1.0 milestone Apr 19, 2018
@mvollmary
Copy link

fixen in 1.1.0

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

No branches or pull requests

2 participants