[SPARK-57504][CORE] Avoid a potential NPE in IndexShuffleBlockResolver.getChecksums when the checksum file cannot be opened

[LuciferYang]

What changes were proposed in this pull request?

IndexShuffleBlockResolver.getChecksums opens the checksum file with var in: DataInputStream = null inside a try and closes it in finally { in.close() }. The method already catches IOException/EOFException to return null, so it is meant to degrade gracefully when the file cannot be read. But the finally is not null-safe: if the failure originates in the stream constructor (before in is assigned), it dereferences null and throws a NullPointerException that masks the original error.

The two sibling methods in the same file already handle constructor failure correctly: checkIndexAndDataFile constructs the stream in its own try/catch, and getMergedBlockData uses Utils.tryWithResource. This change makes getChecksums consistent by using Utils.tryWithResource, which evaluates the constructor before its own try, so a constructor IOException reaches the existing catch and the method returns null as intended.

Why are the changes needed?

getChecksums has a latent NullPointerException: the non-null-safe finally { in.close() } would mask the real error if opening the checksum file fails at construction time, which is inconsistent with the rest of the file. No common code path is known to trigger it today; this is a robustness and consistency improvement that removes the latent NPE and aligns the method with its siblings.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Added a unit test in IndexShuffleBlockResolverSuite that forces the stream constructor to fail (a checksum file whose exists() returns true but which cannot be opened) and asserts getChecksums returns null instead of throwing. The test fails on the old code (NPE) and passes with the fix.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 4.8)

[uros-b]

Minor note: there is one subtle difference from the old code. The original finally { in.close() } would propagate an exception thrown by close() out of the method; the new closeQuietly swallows it. Just pointing it out for awareness, so we can discuss if we're sure that we want to proceed like this.

[LuciferYang]

Good catch. Suppressing it should be fine here: getChecksums reads all the checksums before close(), so a close() failure doesn't affect the result, and the single caller uses it best-effort (reuse, or regenerate on null). It also matches getMergedBlockData in this file, which already reads via Utils.tryWithResource (i.e. closeQuietly).

[uros-b]

Left just one note to discuss, otherwise LGTM - thank you @LuciferYang!

[dongjoon-hyun]

+1, LGTM. Thank you, @LuciferYang .

[mridulm]

Isnt this not equivalent to if (null != in) in.close() in finally ?
The new formulation is better IMO - and I am fine with suppressing exception with close

[LuciferYang]

Isnt this not equivalent to if (null != in) in.close() in finally ? The new formulation is better IMO - and I am fine with suppressing exception with close

equivalent except that it also suppresses close() exceptions

[LuciferYang]

If there are no further comments, I'll merge this later.