Jackie-Jiang commented on code in PR #19250:
URL: https://github.com/apache/pinot/pull/19250#discussion_r3778408266
##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/LocalPinotFS.java:
##########
@@ -115,7 +115,8 @@ public String[] listFiles(URI fileUri, boolean recursive)
throws IOException {
File file = toFile(fileUri);
if (!recursive) {
- return Arrays.stream(file.list()).map(s -> new File(file,
s)).map(File::getAbsolutePath).toArray(String[]::new);
+ return Arrays.stream(listFileNames(file)).map(s -> new File(file,
s)).map(File::getAbsolutePath)
+ .toArray(String[]::new);
Review Comment:
Thanks — the observation is accurate: `Files.walk()` accepts a regular file
as its root and the `!s.equals(file.toPath())` filter drops that sole entry, so
recursive listing of a file returns empty while the non-recursive path now
throws. Same for `listFilesWithMetadata`. I am leaving it as is:
- The case behind this fix is already consistent across both modes. For a
path that does not exist, `Files.walk` throws `NoSuchFileException`, so
recursive and non-recursive both throw — the new test asserts that for
`recursive=true` too. The remaining gap is only "path is a regular file", which
no caller in the retention path can reach.
- There is no cross-implementation convention to align with. Listing a
regular file currently gives three different answers: `HadoopPinotFS` returns
the file itself (`listStatus` on a file returns that file), `S3PinotFS` returns
empty (its own key is filtered out in `visitFiles`), and `LocalPinotFS` returns
empty when recursive. Adding a directory guard here would make `LocalPinotFS`
the only implementation that throws.
- Recursive `listFiles` is called by the ingestion job runners and task
generators on configured `inputDirURI` / `outputDirURI` values
(`SegmentGenerationAndPushTaskGenerator`, the batch push runners,
`SegmentUploaderDefault`). A config pointing at a single file yields "no files"
today; the guard would turn that into a thrown `IOException` — a user-facing
behavior change beyond fixing the NPE.
If the asymmetry is worth closing, the question is what "list a regular
file" should mean across all `PinotFS` implementations, which is an SPI-level
decision better made in its own change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
