This is an automated email from the ASF dual-hosted git repository.
noob-se7en pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/pinot.git
The following commit(s) were added to refs/heads/master by this push:
new e534830ee30 Fix null-safety and PinotFS contract violations in
ADLSGen2PinotFS (#18454)
e534830ee30 is described below
commit e534830ee30b297d41d57fe09c29b4dc20c1164b
Author: Akanksha kedia <[email protected]>
AuthorDate: Wed Jun 10 21:24:27 2026 +0530
Fix null-safety and PinotFS contract violations in ADLSGen2PinotFS (#18454)
* Fix null-safety and PinotFS contract violations in ADLSGen2PinotFS
- isDirectory(): guard against null metadata map before accessing
IS_DIRECTORY_KEY
- lastModified(): return 0L for 404 (missing file) and null OffsetDateTime,
matching PinotFS contract
- touch(): create empty file when it does not exist, instead of propagating
a 404 exception
- open(): throw FileNotFoundException for 404, matching PinotFS contract
- copySrcToDst(): throw FileNotFoundException when source is missing (404)
- listFilesWithMetadata() (both overloads): null-check getLastModified()
before calling toInstant()
to avoid NullPointerException when the timestamp is absent
* Fix touch() to scope 404 catch only to getProperties(), not
setHttpHeaders()
The previous implementation wrapped both getProperties() and
setHttpHeaders() in the
same inner try-catch, causing e.getStatusCode() to be called on any
exception thrown by
setHttpHeaders(). This triggered an unexpected mock interaction in
ADLSGen2PinotFSTest
which verifies no more interactions with the exception mock in tearDown.
Restructure so the inner catch covers only getProperties(): a 404 there
creates the file
and returns immediately; any other status re-throws as IOException.
setHttpHeaders() is
called outside the inner try-catch, so its exceptions propagate directly to
the outer
catch without calling getStatusCode().
* Add unit tests for null-safety branches in ADLSGen2PinotFS
Per noob-se7en's review, covers the 7 new branches added in the null-safety
fix:
- open() 404 DataLakeStorageException → FileNotFoundException
- lastModified() 404 DataLakeStorageException → returns 0L
- lastModified() null OffsetDateTime → returns 0L
- isDirectory() null metadata map → returns false
- listFilesWithMetadata() null getLastModified() → lastModifiedTime = 0L
- touch() 404 on getProperties() → calls createFile() to create the file
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
---------
Co-authored-by: Claude Sonnet 4.6 <[email protected]>
---
.../pinot/plugin/filesystem/ADLSGen2PinotFS.java | 54 +++++++++---
.../filesystem/test/ADLSGen2PinotFSTest.java | 98 ++++++++++++++++++++++
2 files changed, 141 insertions(+), 11 deletions(-)
diff --git
a/pinot-plugins/pinot-file-system/pinot-adls/src/main/java/org/apache/pinot/plugin/filesystem/ADLSGen2PinotFS.java
b/pinot-plugins/pinot-file-system/pinot-adls/src/main/java/org/apache/pinot/plugin/filesystem/ADLSGen2PinotFS.java
index 3c4606d492a..642ae7e06c3 100644
---
a/pinot-plugins/pinot-file-system/pinot-adls/src/main/java/org/apache/pinot/plugin/filesystem/ADLSGen2PinotFS.java
+++
b/pinot-plugins/pinot-file-system/pinot-adls/src/main/java/org/apache/pinot/plugin/filesystem/ADLSGen2PinotFS.java
@@ -44,6 +44,7 @@ import com.google.common.base.Preconditions;
import java.io.ByteArrayInputStream;
import java.io.File;
import java.io.FileInputStream;
+import java.io.FileNotFoundException;
import java.io.FileOutputStream;
import java.io.IOException;
import java.io.InputStream;
@@ -495,9 +496,11 @@ public class ADLSGen2PinotFS extends BasePinotFS {
}
final String filePath =
AzurePinotFSUtil.convertAzureStylePathToUriStylePath(item.getName());
if (pathFilter.test(filePath)) {
+ OffsetDateTime lastModified = item.getLastModified();
+ long lastModifiedTime = lastModified != null ?
lastModified.toInstant().toEpochMilli() : 0L;
result.add(new FileMetadata.Builder()
.setFilePath(filePath)
-
.setLastModifiedTime(item.getLastModified().toInstant().toEpochMilli())
+ .setLastModifiedTime(lastModifiedTime)
.setLength(item.getContentLength())
.setIsDirectory(false)
.build());
@@ -525,8 +528,10 @@ public class ADLSGen2PinotFS extends BasePinotFS {
private static FileMetadata getFileMetadata(PathItem file) {
String path =
AzurePinotFSUtil.convertAzureStylePathToUriStylePath(file.getName());
+ OffsetDateTime lastModified = file.getLastModified();
+ long lastModifiedTime = lastModified != null ?
lastModified.toInstant().toEpochMilli() : 0L;
return new FileMetadata.Builder().setFilePath(path)
-
.setLastModifiedTime(file.getLastModified().toInstant().toEpochMilli()).setLength(file.getContentLength())
+
.setLastModifiedTime(lastModifiedTime).setLength(file.getContentLength())
.setIsDirectory(file.isDirectory()).build();
}
@@ -609,7 +614,7 @@ public class ADLSGen2PinotFS extends BasePinotFS {
// TODO: need to find the other ways to check the directory if it
becomes available. listFiles API returns
// PathInfo, which includes "isDirectory" field; however, there's no API
available for fetching PathInfo directly
// from target uri.
- return Boolean.valueOf(metadata.get(IS_DIRECTORY_KEY));
+ return metadata != null &&
Boolean.parseBoolean(metadata.get(IS_DIRECTORY_KEY));
} catch (DataLakeStorageException e) {
throw new IOException("Failed while checking isDirectory for : " + uri,
e);
}
@@ -627,8 +632,11 @@ public class ADLSGen2PinotFS extends BasePinotFS {
try {
PathProperties pathProperties = getPathProperties(uri);
OffsetDateTime offsetDateTime = pathProperties.getLastModified();
- return offsetDateTime.toInstant().toEpochMilli();
+ return offsetDateTime != null ?
offsetDateTime.toInstant().toEpochMilli() : 0L;
} catch (DataLakeStorageException e) {
+ if (e.getStatusCode() == NOT_FOUND_STATUS_CODE) {
+ return 0L;
+ }
throw new IOException("Failed while checking lastModified time for : " +
uri, e);
}
}
@@ -651,7 +659,17 @@ public class ADLSGen2PinotFS extends BasePinotFS {
try {
DataLakeFileClient fileClient =
_fileSystemClient.getFileClient(AzurePinotFSUtil.convertUriToAzureStylePath(uri));
- PathProperties pathProperties = fileClient.getProperties();
+ PathProperties pathProperties;
+ try {
+ pathProperties = fileClient.getProperties();
+ } catch (DataLakeStorageException e) {
+ if (e.getStatusCode() == NOT_FOUND_STATUS_CODE) {
+ // File does not exist — create an empty file to satisfy the PinotFS
touch() contract
+
_fileSystemClient.createFile(AzurePinotFSUtil.convertUriToAzureStylePath(uri));
+ return true;
+ }
+ throw new IOException(e);
+ }
fileClient.setHttpHeaders(getPathHttpHeaders(pathProperties));
return true;
} catch (DataLakeStorageException e) {
@@ -668,16 +686,30 @@ public class ADLSGen2PinotFS extends BasePinotFS {
@Override
public InputStream open(URI uri)
throws IOException {
- return
_fileSystemClient.getFileClient(AzurePinotFSUtil.convertUriToAzureStylePath(uri)).openInputStream()
- .getInputStream();
+ try {
+ return
_fileSystemClient.getFileClient(AzurePinotFSUtil.convertUriToAzureStylePath(uri)).openInputStream()
+ .getInputStream();
+ } catch (DataLakeStorageException e) {
+ if (e.getStatusCode() == NOT_FOUND_STATUS_CODE) {
+ throw new FileNotFoundException("File not found: " + uri);
+ }
+ throw new IOException(e);
+ }
}
private boolean copySrcToDst(URI srcUri, URI dstUri)
throws IOException {
- PathProperties pathProperties =
-
_fileSystemClient.getFileClient(AzurePinotFSUtil.convertUriToAzureStylePath(srcUri)).getProperties();
- try (InputStream inputStream = open(srcUri)) {
- return copyInputStreamToDst(inputStream, dstUri,
pathProperties.getContentMd5());
+ try {
+ PathProperties pathProperties =
+
_fileSystemClient.getFileClient(AzurePinotFSUtil.convertUriToAzureStylePath(srcUri)).getProperties();
+ try (InputStream inputStream = open(srcUri)) {
+ return copyInputStreamToDst(inputStream, dstUri,
pathProperties.getContentMd5());
+ }
+ } catch (DataLakeStorageException e) {
+ if (e.getStatusCode() == NOT_FOUND_STATUS_CODE) {
+ throw new FileNotFoundException("Source file not found: " + srcUri);
+ }
+ throw new IOException(e);
}
}
diff --git
a/pinot-plugins/pinot-file-system/pinot-adls/src/test/java/org/apache/pinot/plugin/filesystem/test/ADLSGen2PinotFSTest.java
b/pinot-plugins/pinot-file-system/pinot-adls/src/test/java/org/apache/pinot/plugin/filesystem/test/ADLSGen2PinotFSTest.java
index c7823006520..4b7b66bddd8 100644
---
a/pinot-plugins/pinot-file-system/pinot-adls/src/test/java/org/apache/pinot/plugin/filesystem/test/ADLSGen2PinotFSTest.java
+++
b/pinot-plugins/pinot-file-system/pinot-adls/src/test/java/org/apache/pinot/plugin/filesystem/test/ADLSGen2PinotFSTest.java
@@ -31,6 +31,7 @@ import com.azure.storage.file.datalake.models.PathItem;
import com.azure.storage.file.datalake.models.PathProperties;
import java.io.ByteArrayInputStream;
import java.io.File;
+import java.io.FileNotFoundException;
import java.io.IOException;
import java.io.InputStream;
import java.net.URI;
@@ -606,6 +607,103 @@ public class ADLSGen2PinotFSTest {
}
}
+ @Test
+ public void testOpenFileNotFound() {
+
when(_mockFileSystemClient.getFileClient(any())).thenReturn(_mockFileClient);
+
when(_mockFileClient.openInputStream()).thenThrow(_mockDataLakeStorageException);
+ when(_mockDataLakeStorageException.getStatusCode()).thenReturn(404);
+
+ expectThrows(FileNotFoundException.class, () ->
_adlsGen2PinotFsUnderTest.open(_mockURI));
+
+ verify(_mockFileSystemClient).getFileClient(any());
+ verify(_mockFileClient).openInputStream();
+ verify(_mockDataLakeStorageException).getStatusCode();
+ }
+
+ @Test
+ public void testLastModifiedFileNotFound()
+ throws IOException {
+
when(_mockFileSystemClient.getDirectoryClient(any())).thenReturn(_mockDirectoryClient);
+
when(_mockDirectoryClient.getProperties()).thenThrow(_mockDataLakeStorageException);
+ when(_mockDataLakeStorageException.getStatusCode()).thenReturn(404);
+
+ long actual = _adlsGen2PinotFsUnderTest.lastModified(_mockURI);
+ assertEquals(actual, 0L);
+
+ verify(_mockFileSystemClient).getDirectoryClient(any());
+ verify(_mockDirectoryClient).getProperties();
+ verify(_mockDataLakeStorageException).getStatusCode();
+ }
+
+ @Test
+ public void testLastModifiedNullDateTime()
+ throws IOException {
+
when(_mockFileSystemClient.getDirectoryClient(any())).thenReturn(_mockDirectoryClient);
+ when(_mockDirectoryClient.getProperties()).thenReturn(_mockPathProperties);
+ when(_mockPathProperties.getLastModified()).thenReturn(null);
+
+ long actual = _adlsGen2PinotFsUnderTest.lastModified(_mockURI);
+ assertEquals(actual, 0L);
+
+ verify(_mockFileSystemClient).getDirectoryClient(any());
+ verify(_mockDirectoryClient).getProperties();
+ verify(_mockPathProperties).getLastModified();
+ }
+
+ @Test
+ public void testIsDirectoryNullMetadata()
+ throws IOException {
+
when(_mockFileSystemClient.getDirectoryClient(any())).thenReturn(_mockDirectoryClient);
+ when(_mockDirectoryClient.getProperties()).thenReturn(_mockPathProperties);
+ when(_mockPathProperties.getMetadata()).thenReturn(null);
+
+ boolean actual = _adlsGen2PinotFsUnderTest.isDirectory(_mockURI);
+ assertFalse(actual);
+
+ verify(_mockFileSystemClient).getDirectoryClient(any());
+ verify(_mockDirectoryClient).getProperties();
+ verify(_mockPathProperties).getMetadata();
+ }
+
+ @Test
+ public void testListFilesWithMetadataNullLastModified()
+ throws IOException {
+ when(_mockFileSystemClient.listPaths(any(),
any())).thenReturn(_mockPagedIterable);
+ when(_mockPagedIterable.stream()).thenReturn(Stream.of(_mockPathItem));
+ when(_mockPathItem.getName()).thenReturn("foo");
+ when(_mockPathItem.isDirectory()).thenReturn(false);
+ when(_mockPathItem.getContentLength()).thenReturn(1024L);
+ when(_mockPathItem.getLastModified()).thenReturn(null);
+
+ List<FileMetadata> actual =
_adlsGen2PinotFsUnderTest.listFilesWithMetadata(_mockURI, true);
+ FileMetadata fm = actual.get(0);
+ assertEquals(fm.getFilePath(), "/foo");
+ assertEquals(fm.getLastModifiedTime(), 0L);
+
+ verify(_mockFileSystemClient).listPaths(any(), any());
+ verify(_mockPagedIterable).stream();
+ verify(_mockPathItem).getName();
+ verify(_mockPathItem).isDirectory();
+ verify(_mockPathItem).getContentLength();
+ verify(_mockPathItem).getLastModified();
+ }
+
+ @Test
+ public void testTouchFileNotFound()
+ throws IOException {
+
when(_mockFileSystemClient.getFileClient(any())).thenReturn(_mockFileClient);
+
when(_mockFileClient.getProperties()).thenThrow(_mockDataLakeStorageException);
+ when(_mockDataLakeStorageException.getStatusCode()).thenReturn(404);
+
+ boolean actual = _adlsGen2PinotFsUnderTest.touch(_mockURI);
+ assertTrue(actual);
+
+ verify(_mockFileSystemClient).getFileClient(any());
+ verify(_mockFileClient).getProperties();
+ verify(_mockDataLakeStorageException).getStatusCode();
+ verify(_mockFileSystemClient).createFile(any());
+ }
+
@Test
public void testCopyToLocalFileExistingDirectory() throws Exception {
// Create a temporary directory for the test
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
