[FLINK-39115][s3] Support URI Handling Utility - #29127
Conversation
|
@flinkbot run azure |
| } | ||
|
|
||
| public static String extractKey(String s3Uri) { | ||
| String uri = s3Uri.replaceFirst(S3A, S3); |
There was a problem hiding this comment.
nit: now that extractKey(Path) and extractKey(String) live side by side in the same class, they still use two completely different parsing strategies (one via Path/URI, the other via manual indexOf/substring). Was consolidating them into one implementation considered, or is keeping them separate intentional?
| */ | ||
| public static String extractBucket(String s3Uri) { | ||
| String uri = s3Uri.replaceFirst(S3A, S3); | ||
| int bucketStart = uri.indexOf("://") + 3; |
There was a problem hiding this comment.
This can be -1 but not checked
|
|
||
| public static String extractKey(String s3Uri) { | ||
| String uri = s3Uri.replaceFirst(S3A, S3); | ||
| int bucketStart = uri.indexOf("://") + 3; |
There was a problem hiding this comment.
This can be -1 but not checked
| * | ||
| * <p><b>S3 URI Handling:</b> The {@link #extractKey(Path)} and {@link #extractBucketName(Path)} | ||
| * methods expect URIs in the standard {@code s3://bucket/key} format. Other formats like path-style | ||
| * ({@code https://s3.amazonaws.com/bucket/key}) or virtual-hosted-style ({@code | ||
| * https://bucket.s3.amazonaws.com/key}) are not currently supported. |
There was a problem hiding this comment.
Now we're dropping the format expectation and we don't have any enforcement in code too. Maybe we should add and blow up early.
| } | ||
|
|
||
| @Test | ||
| void testIsSupportedLocalSchemeAcceptsNoScheme() { |
There was a problem hiding this comment.
!nit, this test could be folded in parametrised below
| } | ||
|
|
||
| @ParameterizedTest | ||
| @CsvSource({"file:///tmp/foo", "hdfs://bucket/key", "gs://bucket/key"}) |
There was a problem hiding this comment.
Should we also have no schema option test?
| @ParameterizedTest | ||
| @CsvSource({ | ||
| "s3://bucket", | ||
| "s3://bucket/", |
There was a problem hiding this comment.
what would happen for s3://bucket//?
Izeren
left a comment
Izeren
left a comment
There was a problem hiding this comment.
Thank you @Samrat002, LGTM assuming that CI is green and feedback about indexof checks is addressed
github-actions
Bot
added
the
community-reviewed-LGTM
5 participants
What is the purpose of the change
Add S3URIUtils and remove duplicated helpers.
Brief change log
Introduce S3URIUtils.
Verifying this change
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes / no) noDocumentation
Was generative AI tooling used to co-author this PR?
Generated-by: [Tool Name and Version]