Skip to content

AWS, GCS: Fix issue with Kryo and empty immutable collections#13216

Merged
amogh-jahagirdar merged 1 commit into
apache:mainfrom
nastra:kryo-and-immutable-empty-collections
Jun 5, 2025
Merged

AWS, GCS: Fix issue with Kryo and empty immutable collections#13216
amogh-jahagirdar merged 1 commit into
apache:mainfrom
nastra:kryo-and-immutable-empty-collections

Conversation

@nastra

@nastra nastra commented Jun 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

While looking at #13174 I've noticed an issue with Kryo and empty storage credentials.
I've added some tests that reproduce the original issue with Kryo, where the empty immutable collection can't be serded and fails when it is accessed with something like

Cannot read the array length because "this.array" is null
java.lang.NullPointerException: Cannot read the array length because "this.array" is null
	at org.apache.iceberg.relocated.com.google.common.collect.RegularImmutableList.size(RegularImmutableList.java:47)
	at org.apache.iceberg.relocated.com.google.common.collect.Lists.equalsImpl(Lists.java:1039)
	at org.apache.iceberg.relocated.com.google.common.collect.ImmutableList.equals(ImmutableList.java:708)
	at org.assertj.core.internal.StandardComparisonStrategy.areEqual(StandardComparisonStrategy.java:108)
	at org.assertj.core.internal.Objects.areEqual(Objects.java:228)
	at org.assertj.core.internal.Objects.assertEqual(Objects.java:219)
	at org.assertj.core.api.AbstractAssert.isEqualTo(AbstractAssert.java:374)
	at org.assertj.core.api.AbstractIterableAssert.isEqualTo(AbstractIterableAssert.java:4065)
	at org.assertj.core.api.AbstractListAssert.isEqualTo(AbstractListAssert.java:276)
	at org.assertj.core.api.ListAssert.isEqualTo(ListAssert.java:100)
	at org.apache.iceberg.gcp.gcs.GCSFileIOTest.fileIOWithPrefixedStorageClientWithoutCredentialsKryoSerialization(GCSFileIOTest.java:442)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

This applies to the storageCredentials in GCSFileIO / S3FileIO / ResolvingFileIO

@nastra nastra force-pushed the kryo-and-immutable-empty-collections branch from bfba154 to c06303c Compare June 2, 2025 12:00
@nastra nastra force-pushed the kryo-and-immutable-empty-collections branch from c06303c to 530300b Compare June 2, 2025 12:08
@nastra nastra changed the title GCS, S3: Fix issue with Kryo and empty immutable collections AWS, GCS: Fix issue with Kryo and empty immutable collections Jun 2, 2025
talatuyarer
talatuyarer reviewed Jun 2, 2025
View reviewed changes
Comment thread gcp/src/main/java/org/apache/iceberg/gcp/gcs/GCSFileIO.java
@nastra nastra force-pushed the kryo-and-immutable-empty-collections branch from 530300b to 6bcd1af Compare June 3, 2025 07:35
talatuyarer
talatuyarer approved these changes Jun 3, 2025
View reviewed changes

@talatuyarer talatuyarer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

nandorKollar
nandorKollar reviewed Jun 3, 2025
View reviewed changes
Comment thread aws/src/integration/java/org/apache/iceberg/aws/s3/TestS3FileIO.java
});

// make sure credentials can be accessed after kryo serde
ResolvingFileIO resolvingIO = TestHelpers.KryoHelpers.roundTripSerialize(resolvingFileIO);

@nandorKollar nandorKollar Jun 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the assertions look the same for both Kryo and Java serialisation, would it make sense to use a 'mini-parametrisation', something like a for loop:

for serialisation in {Kryo, Java}
roundTripSerialize with serialisation
perform common assertion logic

@nastra nastra Jun 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes that might make sense. We do have the same issue across the codebase so I've opened #13237 to address this

amogh-jahagirdar
amogh-jahagirdar approved these changes Jun 5, 2025
View reviewed changes

@amogh-jahagirdar amogh-jahagirdar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @nastra , and thank you @talatuyarer @nandorKollar for reviewing. this LGTM

@amogh-jahagirdar amogh-jahagirdar merged commit d3ebea5 into apache:main Jun 5, 2025
42 checks passed
@nastra nastra deleted the kryo-and-immutable-empty-collections branch June 6, 2025 05:34
devendra-nr pushed a commit to devendra-nr/iceberg that referenced this pull request Dec 8, 2025
@nastra @devendra-nr
AWS, GCS: Fix issue with Kryo and empty immutable collections for sto…
5c8dedb 
…rage credential (apache#13216)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amogh-jahagirdar amogh-jahagirdar amogh-jahagirdar approved these changes
@danielcweeks danielcweeks Awaiting requested review from danielcweeks
+2 more reviewers
@talatuyarer talatuyarer talatuyarer approved these changes
@nandorKollar nandorKollar nandorKollar approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

AWS core GCP

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@nastra @talatuyarer @nandorKollar @amogh-jahagirdar