Add support for Guava ImmutableIntArray / ImmutableDoubleArray (2.22)#157
Conversation
|
LGTM, but have you already signed the CLA? |
ImmutableIntArray / ImmutableDoubleArray
|
Ah yes -- happy to help get this reviewed, but before merging will need CLA (if not already sent earlier; only needs to be done once before the first contribution). It's this: https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf and the usual way is to print, fill & sign, scan/photo, email to |
|
Sure, I can get the CLA signed; but also I realized that |
|
Ok, one immediate problem tho: looks like our CI is still assuming min Guava level of 20, and So: do we want to bump minimum supported Guava to 22 for Jackson 2.18? EDIT: filed #158 |
|
@kilink One possibility: postpone Guava baseline upgrade to Jackson 2.19, merge this in after 2.18.0 released? This gives us more time. |
I'm fine with that. Not really an urgent need for this, I just saw it as a gap. I can continue just using the custom serializers / deserializers directly for now. |
|
Would like to proceed with this, wrt merging into 2.19, but need 2 things:
with that, I'd be happy to merge this! |
|
Now that #158 complete, baseline not a blocker. But looks like this pr is not mergeable as-is... |
|
@cowtowncoder sorry for the delay, I've found a printer finally and signed the CLA. Is this change still needed? If so, I can rebase it. I noticed that 3.x already has support for these types, however the implementations look like they allocate and box unnecessarily, and may have a bug (doesn't write anything for empty arrays). Anyway I am fine with rebasing this PR if you'd still want it in 2.x, or I can just close it. |
|
@kilink I'd be happy to get 2.x (for 2.22) version. And maybe could replace 3.x implementation with improvements you mention? |
bcd0897 to
5aaabd2
Compare
5aaabd2 to
3d937a0
Compare
Add serde support for ImmutableIntArray and ImmutableDoubleArray.
3d937a0 to
299f969
Compare
|
@cowtowncoder okay, I've rebased onto 2.x and updated the PR to point to that. I also added support for |
cowtowncoder
left a comment
There was a problem hiding this comment.
Looks good; couple of small suggestions (I might apply changes too if need be).
...a/src/main/java/com/fasterxml/jackson/datatype/guava/ser/ImmutableDoubleArraySerializer.java
Outdated
Show resolved
Hide resolved
guava/src/main/java/com/fasterxml/jackson/datatype/guava/ser/ImmutableIntArraySerializer.java
Outdated
Show resolved
Hide resolved
ImmutableIntArray / ImmutableDoubleArrayImmutableIntArray / ImmutableDoubleArray (2.22)
|
Ok, I managed to merge this into 3.x as well, although it's bit of hybrid for now. I think I'll try to follow-up here for |
Thanks! Not sure how I missed the long variant... |
Add serde support for ImmutableIntArray and ImmutableDoubleArray.
Note that for simplicity, the deserializers currently delegate to the implementations in PrimitiveArrayDeserializers, passing the deserialize primitive arrays to the respective
copyOffactory methods. The benefit is that all of the various edge cases of reading ints / doubles is handled already in those implementations. The downside is that we make an extra copy of the array when we pass it to thecopyOfmethod, but at least there is no waste in the array sizing in this case.