jackson-datatypes-collections icon indicating copy to clipboard operation
jackson-datatypes-collections copied to clipboard

Add support for Guava `ImmutableIntArray` / `ImmutableDoubleArray`

Open kilink opened this issue 1 year ago • 6 comments

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 copyOf factory 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 the copyOf method, but at least there is no waste in the array sizing in this case.

kilink avatar Aug 22 '24 06:08 kilink

LGTM, but have you already signed the CLA?

yawkat avatar Aug 27 '24 08:08 yawkat

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 cla at fasterxml dot com. ONce this is done I can proceed with merging (assuming review goes fine etc).

cowtowncoder avatar Aug 27 '24 15:08 cowtowncoder

Sure, I can get the CLA signed; but also I realized that ImmutableIntArray / ImmutableDoubleArray were added in Guava 22.0, so this change cannot really be merged anyway if the minimum supported Guava version is still 20. For some reason I thought these classes had been around much longer.

kilink avatar Aug 27 '24 15:08 kilink

Ok, one immediate problem tho: looks like our CI is still assuming min Guava level of 20, and ImmutableIntArray was added in Guava 22 (as per Javadocs). Note that this is different from default dependency indicated, Guava 25.

So: do we want to bump minimum supported Guava to 22 for Jackson 2.18? I don't have strong opinion here, but will need to file separate issue to increase that, if agreed.

EDIT: filed #158

cowtowncoder avatar Aug 27 '24 15:08 cowtowncoder

@kilink One possibility: postpone Guava baseline upgrade to Jackson 2.19, merge this in after 2.18.0 released? This gives us more time.

cowtowncoder avatar Aug 27 '24 22:08 cowtowncoder

@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.

kilink avatar Aug 28 '24 00:08 kilink

Would like to proceed with this, wrt merging into 2.19, but need 2 things:

  1. PR needs to re-based (or re-created against 2.19)
  2. CLA

with that, I'd be happy to merge this!

cowtowncoder avatar Nov 05 '24 01:11 cowtowncoder

Now that #158 complete, baseline not a blocker. But looks like this pr is not mergeable as-is...

cowtowncoder avatar Jul 18 '25 02:07 cowtowncoder