xiangfu0 commented on code in PR #19863:
URL: https://github.com/apache/pinot/pull/19863#discussion_r4278287625
##########
pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java:
##########
@@ -797,6 +799,67 @@ public List<FileMetadata> listFilesWithMetadata(final URI
fileUri, final boolean
return result;
}
+ @Override
+ public boolean supportsListingAfter() {
+ return true;
+ }
+
+ @Override
+ public List<FileMetadata> listFilesWithMetadataAfter(final URI fileUri,
@Nullable final String startAfter,
+ final int maxResults)
+ throws IOException {
+ final List<FileMetadata> result = new ArrayList<>();
+ if (maxResults <= 0) {
+ return result;
+ }
+ final String scheme = fileUri.getScheme();
+ Preconditions.checkArgument(scheme.equals(S3_SCHEME) ||
scheme.equals(S3A_SCHEME));
+ final String pathPrefix = scheme + SCHEME_SEPARATOR + fileUri.getHost() +
DELIMITER;
+ Preconditions.checkArgument(startAfter == null ||
startAfter.startsWith(pathPrefix),
+ "startAfter %s is not under %s", startAfter, pathPrefix);
+ try {
+ final String prefix = normalizeToDirectoryPrefix(fileUri);
+ String continuationToken = null;
+ boolean isDone = false;
+ while (!isDone && result.size() < maxResults) {
+ final ListObjectsV2Request.Builder builder =
ListObjectsV2Request.builder().bucket(fileUri.getHost())
+ .maxKeys(Math.min(maxResults - result.size(), LIST_MAX_KEYS));
+ if (!prefix.equals(DELIMITER)) {
+ builder.prefix(prefix);
+ }
+ if (continuationToken != null) {
+ builder.continuationToken(continuationToken);
+ } else if (startAfter != null) {
+ // StartAfter is ignored once a continuation token is set.
Review Comment:
Good catch handling the "StartAfter is ignored once a continuation token is
set" case — the `testContinuationReplacesStartAfter` test locks this in. This
is the kind of S3 subtlety that's easy to get wrong, so thanks for the explicit
comment and coverage.
##########
pinot-plugins/pinot-file-system/pinot-gcs/src/main/java/org/apache/pinot/plugin/filesystem/GcsPinotFS.java:
##########
@@ -316,6 +313,72 @@ public List<FileMetadata> listFilesWithMetadata(final URI
fileUri, final boolean
return result;
}
+ @Override
+ public boolean supportsListingAfter() {
+ return true;
+ }
+
+ /// GCS lists objects by name and file paths are those names URI-escaped, so
`startAfter` is decoded back to the
+ /// object name it spells.
+ @Override
+ public List<FileMetadata> listFilesWithMetadataAfter(final URI fileUri,
@Nullable final String startAfter,
+ final int maxResults)
+ throws IOException {
+ final List<FileMetadata> result = new ArrayList<>();
+ if (maxResults <= 0) {
+ return result;
+ }
+ final GcsUri gcsFileUri = new GcsUri(fileUri);
+ final String bucketName = gcsFileUri.getBucketName();
+ final String prefix = gcsFileUri.getPrefix();
+ String startName = null;
+ if (startAfter != null) {
+ final URI start = URI.create(startAfter);
+ checkArgument(GcsUri.SCHEME.equals(start.getScheme()) &&
bucketName.equals(start.getAuthority()),
+ "startAfter %s is not in bucket %s", startAfter, bucketName);
+ startName = start.getPath().substring(1);
Review Comment:
`start.getPath().substring(1)` decodes the escaped path back to the raw
object name, which is exactly what `startOffset` wants — nice. One edge case
worth a thought: if a caller ever passes `gs://bucket/` (empty path),
`getPath()` is empty and `substring(1)` throws
`StringIndexOutOfBoundsException`. Probably not a real caller shape, but a
guard or a quick test would make the contract robust.
##########
pinot-plugins/pinot-file-system/pinot-gcs/src/main/java/org/apache/pinot/plugin/filesystem/GcsPinotFS.java:
##########
@@ -316,6 +313,72 @@ public List<FileMetadata> listFilesWithMetadata(final URI
fileUri, final boolean
return result;
}
+ @Override
+ public boolean supportsListingAfter() {
+ return true;
+ }
+
+ /// GCS lists objects by name and file paths are those names URI-escaped, so
`startAfter` is decoded back to the
+ /// object name it spells.
+ @Override
+ public List<FileMetadata> listFilesWithMetadataAfter(final URI fileUri,
@Nullable final String startAfter,
+ final int maxResults)
+ throws IOException {
+ final List<FileMetadata> result = new ArrayList<>();
+ if (maxResults <= 0) {
+ return result;
+ }
+ final GcsUri gcsFileUri = new GcsUri(fileUri);
+ final String bucketName = gcsFileUri.getBucketName();
+ final String prefix = gcsFileUri.getPrefix();
+ String startName = null;
+ if (startAfter != null) {
+ final URI start = URI.create(startAfter);
+ checkArgument(GcsUri.SCHEME.equals(start.getScheme()) &&
bucketName.equals(start.getAuthority()),
+ "startAfter %s is not in bucket %s", startAfter, bucketName);
+ startName = start.getPath().substring(1);
+ }
+ try {
+ final List<Storage.BlobListOption> options = new ArrayList<>();
+ options.add(Storage.BlobListOption.prefix(prefix));
+ options.add(Storage.BlobListOption.pageSize(Math.min(maxResults,
LIST_PAGE_SIZE)));
Review Comment:
Nit: the page size is capped against the original `maxResults`, while the S3
path uses the remaining count (`maxResults - result.size()`). After skipping
the `startOffset` match and directory markers, this can fetch a slightly
larger-than-needed page on continuation. Not a correctness issue since the loop
stops at `maxResults`, but matching S3's `maxResults - result.size()` would
keep the two implementations consistent.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFS.java:
##########
@@ -188,6 +189,27 @@ default List<FileMetadata> listFilesWithMetadata(URI
fileUri, boolean recursive,
return result;
}
+ /// Whether [#listFilesWithMetadataAfter] is supported.
+ default boolean supportsListingAfter() {
+ return false;
+ }
+
+ /// Lists the files (never directories) under `fileUri` recursively, in the
store's key order, starting strictly
+ /// after the key `startAfter` names. That key need not exist, so a caller
can skip a range of keys with one request
+ /// instead of paging through it. Only stores whose listing is sorted and
can start at an arbitrary key implement
+ /// this; see [#supportsListingAfter]. Where file paths escape the key (GCS
URI-escapes object names), path order
Review Comment:
The javadoc is clear about path-order vs key-order differing for GCS —
that's the one caveat a consumer really needs. Could be worth one concrete
sentence that callers computing `startAfter` should derive it from a
previously-returned `getFilePath()` value (not a hand-built key), so the
escaping matches on both sides.
##########
pinot-plugins/pinot-file-system/pinot-s3/src/main/java/org/apache/pinot/plugin/filesystem/S3PinotFS.java:
##########
@@ -797,6 +799,67 @@ public List<FileMetadata> listFilesWithMetadata(final URI
fileUri, final boolean
return result;
}
+ @Override
+ public boolean supportsListingAfter() {
+ return true;
+ }
+
+ @Override
+ public List<FileMetadata> listFilesWithMetadataAfter(final URI fileUri,
@Nullable final String startAfter,
+ final int maxResults)
+ throws IOException {
+ final List<FileMetadata> result = new ArrayList<>();
+ if (maxResults <= 0) {
+ return result;
+ }
+ final String scheme = fileUri.getScheme();
+ Preconditions.checkArgument(scheme.equals(S3_SCHEME) ||
scheme.equals(S3A_SCHEME));
+ final String pathPrefix = scheme + SCHEME_SEPARATOR + fileUri.getHost() +
DELIMITER;
+ Preconditions.checkArgument(startAfter == null ||
startAfter.startsWith(pathPrefix),
Review Comment:
The validation only checks that `startAfter` shares the `s3://bucket/`
prefix, not that it falls under the listing directory prefix. That's fine
functionally (S3's prefix filter still constrains results), but it means a
`startAfter` pointing at a sibling directory silently behaves like a seek
rather than an error. Worth a one-line javadoc note that `startAfter` only
needs to be in the same bucket, not necessarily under `fileUri`.
##########
pinot-spi/src/main/java/org/apache/pinot/spi/filesystem/PinotFS.java:
##########
@@ -188,6 +189,27 @@ default List<FileMetadata> listFilesWithMetadata(URI
fileUri, boolean recursive,
return result;
}
+ /// Whether [#listFilesWithMetadataAfter] is supported.
+ default boolean supportsListingAfter() {
+ return false;
+ }
+
+ /// Lists the files (never directories) under `fileUri` recursively, in the
store's key order, starting strictly
+ /// after the key `startAfter` names. That key need not exist, so a caller
can skip a range of keys with one request
+ /// instead of paging through it. Only stores whose listing is sorted and
can start at an arbitrary key implement
+ /// this; see [#supportsListingAfter]. Where file paths escape the key (GCS
URI-escapes object names), path order
+ /// and key order can differ.
+ ///
+ /// @param fileUri location to list files from
+ /// @param startAfter a path in the form [FileMetadata#getFilePath] returns,
or null to start at the first file
+ /// @param maxResults maximum number of files to return; fewer are returned
only when no files remain
+ /// @return the next files in order, at most `maxResults`
+ /// @throws IOException on IO failure
+ default List<FileMetadata> listFilesWithMetadataAfter(URI fileUri, @Nullable
String startAfter, int maxResults)
Review Comment:
The default throwing `UnsupportedOperationException` with no message is a
bit opaque when it surfaces. Consider a short message (e.g. the class name plus
"does not support listing after a key; check supportsListingAfter()") so a
caller that forgot the capability check gets an actionable error.
--
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]
