-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support buckets containing underscore #263
Conversation
@@ -73,7 +73,7 @@ PolarisStorageIntegration<T> getStorageIntegrationForConfig( | |||
HttpTransportFactory.class, NetHttpTransport::new)); | |||
} catch (IOException e) { | |||
throw new RuntimeException( | |||
"Error initializing default google credentials" + e.getMessage()); | |||
"Error initializing default google credentials. " + e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not related to this fix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
} | ||
|
||
@ParameterizedTest | ||
@ValueSource(strings = {"s3", "gcs", "azure"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: do you think here values should be inline with iceberg supported scheme ? something like
s3, s3a, s3n, gs, abfs, abfss
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the purposes of this PR, these need to match the schemes listed here
Thanks @eric-maynard and thanks @nk1506 for review |
Description
Currently, we are using
URI.getHost
to extract the bucket from paths, which can returnnull
unexpectedly when the bucket contains an underscore. This seems to be a known issue.This PR introduces a thin new utility for extracting the path and adds a test for it.
Fixes #262
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Added a new suite,
StorageUtilTest
.Checklist:
Please delete options that are not relevant.