search

FasterXML / jackson-dataformats-binary

Uber-project for standard Jackson binary format backends: avro, cbor, ion, protobuf, smile
Apache License 2.0
314 stars 136 forks source link

Add support to (de)serialize Ion timestamps with java.time classes #208

Closed mcliedtke closed 4 years ago

mcliedtke commented 4 years ago

Adding support for some java 8 java.time classes to (de)serialize to Ion's timestamp type.

Starting with just the following since it seemed pretty clear cut that there could be support:

I was less sure about how to handle java.time.LocalDate and java.time.LocalDateTime so they have been omitted.

Serialization is fairly straightforward, each class is pretty easily transformed into an Ion timestamp. The caveat is that the transformation for a java.time.ZonedDateTime will be slightly lossy since an Ion timestamp cannot represent the timezone, just the offset. In this case the local offset is used.

Deserialization is also mostly straight forward. In the event that the Ion timestamp's time local offset is unknown (-00:00), then the default timezone is used from the context.

(De)serialization to/from an Ion int or Ion decimal is added as well though I haven't tried to add support to/from an Ion string.

Some of the code could be refined with lambdas and function references in the future if this module is moved to java 8.

cowtowncoder commented 4 years ago

Sounds reasonable; I assume you know what you are doing :) Since I think we have CCLA for Amazon, including you that should be fine.

Couple of questions:

  1. Ion module is still nominally JDK7 compatible before this change, I think, so to merge it in, module should be considered JDK8-only. I think this should mean that fix goes in 2.12.0, as 2.11.0 was already released? If so, I think it'd make sense to rebase this against 2.12 branch
  2. It would probably be good to add usage information for this module (probably on ion/README.md?)
  3. Since you have contributed changes and know your way around, would you like to become the maintainer of the Ion format? I could give you bit more access, add information on README (and other places where we note owners of various modules), and would know to include you for code reviews for PRs others submit
mcliedtke commented 4 years ago
  1. Yeah I think the module is currently JDK7 compatible. Would a JDK7 consumer be able to consume these changes safely by just avoiding the IonJavaTimeModule and other classes? But in general I agree with your point, it would be better to just make this entire module require JDK8 and that would be better to target for 2.12. What's the timeline look like for 2.12? 2.11 was just released so I presume maybe later this year?
  2. Yes, I can make some updates to the README.md
  3. I would be excited to get more involved and take on some responsibilities. I am up for being a maintainer but not so sure about being the maintainer :). I am passionate about Ion and Jackson but won't claim to know Jackson thoroughly. Would I still be able to delegate/defer to you in certain areas? I probably should also check internally to see if there is anything I need to be aware of.
cowtowncoder commented 4 years ago

So, on 2.12: I am hoping to make this release "quick one" similar to 2.11, but it is probably still couple of months out. If I had to guess, first RC by end of August, release by end of Sep or early Oct.

On being maintainer: I would still handle release process, help merge changes from databind and so on so I would not just dump maintenance on you. It'd be more that I would rely on you to validate Ion-related parts; I could help from overall Jackson design aspects. This is much easier here than with, say, Scala and Kotlin modules where my expertise is much more limited.

mcliedtke commented 4 years ago

Opened a new PR with the changes against the 2.12 branch: https://github.com/FasterXML/jackson-dataformats-binary/pull/213

I am working out what permsions I need for being a maintainer and will reach out once I hear more, this is still something I would be very interested in.