Core: Add UTs for SerializationUtil class.

[pbajpai21]

Adds unit test coverage for SerializationUtil. Covers the byte and base64 serialize/deserialize round trips, null handling on both deserialize paths, and MIME line-wrapping in the base64 encoding.

[pbajpai21]

@ebyhr @szehon-ho Please review the PR when you have time. Thank you.

[ebyhr]

The test coverage for objects extending HadoopConfigurable looks missing. Is it intentional?

[pbajpai21]

Thank you @ebyhr for pointing this out. I have added the UTs to cover this and also covered error-handling paths.
Kindly review again. Thank you.

[pbajpai21]

@ebyhr I have applied your suggestions in the code, kindly review the PR and If looks good to you, please approve.
Thank you.

[laskoviymishka]

Thanks for adding these — direct coverage for SerializationUtil is a genuine gap (the only production callers are in mr, and nothing in core tested it directly), so having it is a small win.

Most of the file round-trips through the JDK's own serialization and base64, which is fine but low-risk. The part I'd actually want tightened is the two HadoopConfigurable tests, since that branch is the one piece of logic unique to this class — and as written both pass without exercising it. serializeToBytesAppliesCustomConfSerializerToHadoopConfigurable returns the default SerializableConfiguration from its serializer, so it can't tell "our function's result was serialized" from "the util built its own and called the lambda incidentally"; and hadoopConfigurableRoundTripPreservesConfiguration round-trips fine even if the instanceof HadoopConfigurable branch were deleted, since the fixture wraps the conf in its constructor regardless. I left inline suggestions on both.

The rest is minor — cause-based exception assertions over hasMessage, \r\n vs \n, the (Object) casts, and the Test-prefixed fixture name. None of those block.

[laskoviymishka]

The serializer here returns a plain SerializableConfiguration, which is exactly what the fixture already uses — so this passes even if serializeToBytes dropped our function's result and built its own. The two isTrue() flags only prove the lambda ran, not that its output was the thing serialized.

I'd have the serializer return a distinctive supplier (one that yields a Configuration carrying a marker key), round-trip the bytes, and assert getConf() comes back with the marker. That's the actual serializeConfWith contract that Spark and Flink substitute their own serializers into.

[laskoviymishka]

The fixture constructor already wraps the conf in SerializableConfiguration, so this round-trip succeeds whether or not SerializationUtil ever enters the instanceof HadoopConfigurable branch — delete that branch and the test is still green. Since this is the only test of the default HadoopConfigurable path, I'd start the fixture from a non-serializable conf holder (a transient Configuration, or a supplier that throws NotSerializableException until serializeConfWith runs), or assert serializeConfWithInvoked after the single-arg call, so it fails when that branch regresses.

[laskoviymishka]

hasMessage pins the human-readable string but never checks the cause, so a reword breaks the test while an actual regression in what gets wrapped slips through. I'd add .hasCauseInstanceOf(NotSerializableException.class) here (and StreamCorruptedException in deserializeFromBytesWrapsIOException) — strictly more informative; keep the message assertion too if you like.

[laskoviymishka]

contains("\n") works since the MIME separator is CRLF, but it's vague about what we're actually pinning. I'd tighten it to the real wrapping behavior:

Suggested change

	assertThat(encoded).contains("\n");
	assertThat(encoded).contains("\r\n");

[laskoviymishka]

Small thing — the (Object) cast is only here to settle generic inference. Pulling the result into a local reads cleaner (same for the base64 null test):

Suggested change

	assertThat((Object) SerializationUtil.deserializeFromBytes(null)).isNull();
	Object result = SerializationUtil.deserializeFromBytes(null);
	assertThat(result).isNull();

[laskoviymishka]

The Test prefix on this nested helper can read as a test class to discovery tooling and reviewers. Iceberg usually names these fixtures without the leading Test — I'd call it something like HadoopConfigurableFixture.

[developer-rpai]

Meaningful round-trip assertions (not tautological), the prior reviewer's HadoopConfigurable concern was genuinely addressed, CI is fully green, no flakiness. The findings below are nits/questions; none block.

1. Nit: Base64.getMimeEncoder() emits CRLF line breaks, so contains("\r\n") would pin the actual MIME behavior more precisely than contains("\n").

2. One error path looks uncovered: deserializeFromBase64 with malformed input throws a raw IllegalArgumentException from the MIME decoder, while the byte-path failures are wrapped in UncheckedIOException. Is that asymmetry intentional? If so, a test documenting it would lock the contract in.

3. Minor: null is covered on both deserialize paths but not on serializeToBytes(null) (which writes and reads back null cleanly). Worth a one-liner for symmetry, or a note if intentionally omitted.

4. Nit: in serializeToBytesAppliesCustomConfSerializerToHadoopConfigurable, the single-threaded invocation flag works fine as a one-element array, but AtomicBoolean is the more idiomatic choice.