Fix null-safety and contract violations in HadoopPinotFS#18457
Merged
yashmayya merged 2 commits intoMay 11, 2026
Conversation
- visitFileStatus(): guard against null return value from FileSystem.listStatus() before iterating, avoiding NPE on empty or inaccessible directories - isDirectory() / lastModified(): remove RuntimeException wrapping of IOException — both methods are declared in the PinotFS interface to throw IOException, so the checked exception should propagate directly to callers rather than being hidden in a RuntimeException - touch(): use try-with-resources for FSDataOutputStream so the stream is closed even if fos.close() throws, preventing a resource leak - close(): null-check _hadoopFS before calling close() to guard against the case where init() was never called or threw
Contributor
Author
|
@xiangfu0 please review |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18457 +/- ##
============================================
+ Coverage 63.40% 63.66% +0.25%
- Complexity 1679 1684 +5
============================================
Files 3253 3256 +3
Lines 198767 199548 +781
Branches 30791 30986 +195
============================================
+ Hits 126034 127047 +1013
+ Misses 62659 62370 -289
- Partials 10074 10131 +57
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
xiangfu0
reviewed
May 11, 2026
Contributor
xiangfu0
left a comment
xiangfu0
left a comment
There was a problem hiding this comment.
Found one high-signal correctness issue; see inline comment.
…g on null listStatus Silently returning when listStatus() returns null causes delete(uri, false) to see an empty directory listing, skip the non-empty guard, and fall through to _hadoopFS.delete(path, true) — recursively deleting a non-empty directory that should have been preserved. Throw IOException instead so callers fail fast.
Contributor
Author
|
@xiangfu0 recheck please |
yashmayya
approved these changes
May 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four independent issues in
HadoopPinotFS:visitFileStatus()— null return fromlistStatus()FileSystem.listStatus()can returnnullon some Hadoop versions when the path is empty or inaccessible. Iterating over a null array causes NPE. Added an early-return null guard.isDirectory()/lastModified()—RuntimeExceptionwrappingBoth methods catch
IOExceptionand rethrow wrapped inRuntimeException. ThePinotFSinterface declares both asthrows IOException, so the checked exception should propagate directly. Removed the wrapping and addedthrows IOExceptionto the overrides.touch()—FSDataOutputStreamresource leakFSDataOutputStream fos = _hadoopFS.create(path); fos.close();— iffos.close()throws, the stream leaks. Replaced with try-with-resources.close()— NPE wheninit()was never calledIf
init()never ran or threw before assigning_hadoopFS,close()throws NPE. Added a null guard before calling_hadoopFS.close().Test plan
HadoopPinotFSpasslistStatusreturningnullno longer causes NPE inlistFiles/listFilesWithMetadataisDirectoryandlastModifiedpropagateIOExceptionto callers instead ofRuntimeException