[#5221] feat(python-client): Support OSS for fileset python client #5225
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Please rebase this PR. |
|
@xloya |
yuqi1129
changed the title
jerryshao
added
branch-0.7
yuqi1129
added
branch-0.7
xloya
reviewed
Oct 24, 2024
|
|
||
| @unittest.skip("This test require oss service account") | ||
| class TestGvfsWithOSS(TestGvfsWithHDFS): | ||
| # Before running this test, please set the make sure aws-bundle-x.jar has been |
There was a problem hiding this comment.
aws-bundle-x.jar -> aliyun-bundle-x.jar
| @@ -567,6 +569,9 @@ def _convert_actual_path( | |||
| or storage_location.startswith(f"{StorageType.S3A.value}://") | |||
| ): | |||
| actual_prefix = infer_storage_options(storage_location)["path"] | |||
| elif storage_location.startswith(f"{StorageType.OSS.value}:/"): | |||
| ops = infer_storage_options(storage_location) | |||
| actual_prefix = ops["host"] + ops["path"] | |||
There was a problem hiding this comment.
it's better to check the ops whether has host and path attributes.
| @@ -762,6 +769,13 @@ def _strip_storage_protocol(storage_type: StorageType, path: str): | |||
| if storage_type == StorageType.LOCAL: | |||
| return path[len(f"{StorageType.LOCAL.value}:") :] | |||
|
|
|||
| # OSS has different behavior than S3 and GCS, if we do not remove the | |||
| # protocol, it will always return an empty array. | |||
| if storage_type == StorageType.OSS: | |||
There was a problem hiding this comment.
It's better add the uri example in the method annotation.
There was a problem hiding this comment.
Yeah, please explain more.
| @@ -22,7 +22,7 @@ black==24.4.2 | |||
| twine==5.1.1 | |||
| coverage==7.5.1 | |||
| pandas==2.0.3 | |||
| pyarrow==15.0.2 | |||
| pyarrow==17.0.0 | |||
There was a problem hiding this comment.
Why we need upgrade pyarrow version here?
There was a problem hiding this comment.
This is an unexpected change and I have reverted it.
xloya
reviewed
Oct 24, 2024
| @@ -756,12 +768,42 @@ def _strip_storage_protocol(storage_type: StorageType, path: str): | |||
| :param storage_type: The storage type | |||
| :param path: The path | |||
| :return: The stripped path | |||
|
|
|||
| We will handle OSS differently from S3 and GCS, because OSS has different behavior than S3 and GCS. | |||
There was a problem hiding this comment.
You'd better explain the difference behavior clearly, otherwise someone may not be able to spot the difference at first glance when looking at the examples.
There was a problem hiding this comment.
Add sentence
Please take a look at the above example: if we do not remove the protocol (starts with oss://),
it will always return an empty array when we call `oss.ls`, however, if we remove the protocol,
it will return the correct result as expected.
|
Overall LGTM, but a minor annotation issue. |
jerryshao
approved these changes
Oct 24, 2024
github-actions bot
pushed a commit
that referenced
this pull request
Oct 24, 2024
…5225) ### What changes were proposed in this pull request? Add support for Aliyun OSS python client. ### Why are the changes needed? It's a need Fix: #5221 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? Test locally. Do the following change to `test_gvfs_with_oss.py` and run `./gradlew :clients:client-python:test -PskipDockerTests=false` <img width="1261" alt="image" src="https://github.com/user-attachments/assets/a57a3f2e-178b-4b50-8219-5a95d0329cd1"> <img width="1211" alt="image" src="https://github.com/user-attachments/assets/b2675a44-d9db-4d11-a505-8e098ef3173f">
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Add support for Aliyun OSS python client.
Why are the changes needed?
It's a need
Fix: #5221
Does this PR introduce any user-facing change?
N/A
How was this patch tested?
Test locally. Do the following change to
test_gvfs_with_oss.pyand run./gradlew :clients:client-python:test -PskipDockerTests=false