You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "helmiazizm (via GitHub)" <gi...@apache.org> on 2025/03/12 05:44:20 UTC
[PR] Added `FsspecFileIO` method for OSS, virtual hosted style default to true, standardized key configurations for OSS [iceberg-python]
helmiazizm opened a new pull request, #1788:
URL: https://github.com/apache/iceberg-python/pull/1788
This pull request introduced `FsspecFileIo` for OSS configuration method as a backup when `PyArrowFileIO` fail. Using `S3FileSystem` class, the method should work as long as the virtual hosted style is invoked. For both method, virtual hosted style is set to true as default.
Also, since OSS configuration has its own section, I think it makes more sense to standardize the configuration keys with `oss.certain-config` instead of still relying with `s3` key.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Added `FsspecFileIO` method for OSS, virtual hosted style default to true, standardized key configurations for OSS [iceberg-python]
Posted by "helmiazizm (via GitHub)" <gi...@apache.org>.
helmiazizm commented on code in PR #1788:
URL: https://github.com/apache/iceberg-python/pull/1788#discussion_r1992537452
##########
pyiceberg/io/pyarrow.py:
##########
@@ -398,31 +402,13 @@ def _initialize_oss_fs(self) -> FileSystem:
from pyarrow.fs import S3FileSystem
client_kwargs: Dict[str, Any] = {
- "endpoint_override": self.properties.get(S3_ENDPOINT),
- "access_key": get_first_property_value(self.properties, S3_ACCESS_KEY_ID, AWS_ACCESS_KEY_ID),
- "secret_key": get_first_property_value(self.properties, S3_SECRET_ACCESS_KEY, AWS_SECRET_ACCESS_KEY),
- "session_token": get_first_property_value(self.properties, S3_SESSION_TOKEN, AWS_SESSION_TOKEN),
- "region": get_first_property_value(self.properties, S3_REGION, AWS_REGION),
Review Comment:
You're right. I'll fix it right away.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Added `FsspecFileIO` method for OSS, virtual hosted style default to true, standardized key configurations for OSS [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1788:
URL: https://github.com/apache/iceberg-python/pull/1788#discussion_r1997988907
##########
pyiceberg/io/fsspec.py:
##########
@@ -124,6 +128,22 @@ def _file(_: Properties) -> LocalFileSystem:
return LocalFileSystem(auto_mkdir=True)
+def _oss(properties: Properties) -> AbstractFileSystem:
+ from s3fs import S3FileSystem
+
+ client_kwargs = {
+ "endpoint_url": properties.get(OSS_ENDPOINT),
+ "aws_access_key_id": properties.get(OSS_ACCESS_KEY_ID),
+ "aws_secret_access_key": properties.get(OSS_ACCESS_KEY_SECRET),
+ "aws_session_token": properties.get(OSS_SESSION_TOKEN),
+ }
+ config_kwargs = {"s3": {"addressing_style": "virtual"}, "signature_version": "v4"}
Review Comment:
But we've already crossed that bridge with `PyArrowFileIO`: https://github.com/apache/iceberg-python/pull/1788/files#diff-8d5e63f2a87ead8cebe2fd8ac5dcf2198d229f01e16bb9e06e21f7277c328abdL423-L424
I think it would also be good to add a configuration option for it. we don't know if people are using it, and just removing it feels wrong.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Added `FsspecFileIO` method for OSS, virtual hosted style default to true, standardized key configurations for OSS [iceberg-python]
Posted by "helmiazizm (via GitHub)" <gi...@apache.org>.
helmiazizm commented on code in PR #1788:
URL: https://github.com/apache/iceberg-python/pull/1788#discussion_r2002796293
##########
pyiceberg/io/fsspec.py:
##########
@@ -124,6 +128,22 @@ def _file(_: Properties) -> LocalFileSystem:
return LocalFileSystem(auto_mkdir=True)
+def _oss(properties: Properties) -> AbstractFileSystem:
+ from s3fs import S3FileSystem
+
+ client_kwargs = {
+ "endpoint_url": properties.get(OSS_ENDPOINT),
+ "aws_access_key_id": properties.get(OSS_ACCESS_KEY_ID),
+ "aws_secret_access_key": properties.get(OSS_ACCESS_KEY_SECRET),
+ "aws_session_token": properties.get(OSS_SESSION_TOKEN),
+ }
+ config_kwargs = {"s3": {"addressing_style": "virtual"}, "signature_version": "v4"}
Review Comment:
To be honest, I'm not entirely sure which `config_kwargs` keys supported by s3fs that can work with OSS connection. The documentation for `boto3` compatibility only mentioned setting several keys: https://www.alibabacloud.com/help/en/oss/developer-reference/use-amazon-s3-sdks-to-access-oss#section-jmf-a67-hat
Do you think it'll make sense to copy the `config_kwargs` setting for S3 but with `oss.config-example` parameter?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Added `FsspecFileIO` method for OSS, virtual hosted style default to true, standardized key configurations for OSS [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1788:
URL: https://github.com/apache/iceberg-python/pull/1788#discussion_r1992187967
##########
pyiceberg/io/fsspec.py:
##########
@@ -124,6 +128,22 @@ def _file(_: Properties) -> LocalFileSystem:
return LocalFileSystem(auto_mkdir=True)
+def _oss(properties: Properties) -> AbstractFileSystem:
+ from s3fs import S3FileSystem
+
+ client_kwargs = {
+ "endpoint_url": properties.get(OSS_ENDPOINT),
+ "aws_access_key_id": properties.get(OSS_ACCESS_KEY_ID),
+ "aws_secret_access_key": properties.get(OSS_ACCESS_KEY_SECRET),
+ "aws_session_token": properties.get(OSS_SESSION_TOKEN),
+ }
+ config_kwargs = {"s3": {"addressing_style": "virtual"}, "signature_version": "v4"}
+
+ fs = S3FileSystem(client_kwargs=client_kwargs, config_kwargs=config_kwargs)
+
+ return fs
Review Comment:
Less is more!
```suggestion
return S3FileSystem(client_kwargs=client_kwargs, config_kwargs=config_kwargs)
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Added `FsspecFileIO` method for OSS, virtual hosted style default to true, standardized key configurations for OSS [iceberg-python]
Posted by "helmiazizm (via GitHub)" <gi...@apache.org>.
helmiazizm closed pull request #1788: Added `FsspecFileIO` method for OSS, virtual hosted style default to true, standardized key configurations for OSS
URL: https://github.com/apache/iceberg-python/pull/1788
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Added `FsspecFileIO` method for OSS, virtual hosted style default to true, standardized key configurations for OSS [iceberg-python]
Posted by "helmiazizm (via GitHub)" <gi...@apache.org>.
helmiazizm commented on PR #1788:
URL: https://github.com/apache/iceberg-python/pull/1788#issuecomment-2716592898
Local test result for `s3fs.S3FileSystem`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Added `FsspecFileIO` method for OSS, virtual hosted style default to true, standardized key configurations for OSS [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1788:
URL: https://github.com/apache/iceberg-python/pull/1788#discussion_r1992187633
##########
pyiceberg/io/fsspec.py:
##########
@@ -124,6 +128,22 @@ def _file(_: Properties) -> LocalFileSystem:
return LocalFileSystem(auto_mkdir=True)
+def _oss(properties: Properties) -> AbstractFileSystem:
+ from s3fs import S3FileSystem
+
+ client_kwargs = {
+ "endpoint_url": properties.get(OSS_ENDPOINT),
+ "aws_access_key_id": properties.get(OSS_ACCESS_KEY_ID),
+ "aws_secret_access_key": properties.get(OSS_ACCESS_KEY_SECRET),
+ "aws_session_token": properties.get(OSS_SESSION_TOKEN),
+ }
+ config_kwargs = {"s3": {"addressing_style": "virtual"}, "signature_version": "v4"}
Review Comment:
Should wel also make virtual addressing configurable? Similar to `S3_FORCE_VIRTUAL_ADDRESSING`?
```suggestion
config_kwargs = {"s3": {"addressing_style": "virtual"}, "signature_version": "v4"}
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Added `FsspecFileIO` method for OSS, virtual hosted style default to true, standardized key configurations for OSS [iceberg-python]
Posted by "helmiazizm (via GitHub)" <gi...@apache.org>.
helmiazizm commented on code in PR #1788:
URL: https://github.com/apache/iceberg-python/pull/1788#discussion_r1992533665
##########
pyiceberg/io/fsspec.py:
##########
@@ -124,6 +128,22 @@ def _file(_: Properties) -> LocalFileSystem:
return LocalFileSystem(auto_mkdir=True)
+def _oss(properties: Properties) -> AbstractFileSystem:
+ from s3fs import S3FileSystem
+
+ client_kwargs = {
+ "endpoint_url": properties.get(OSS_ENDPOINT),
+ "aws_access_key_id": properties.get(OSS_ACCESS_KEY_ID),
+ "aws_secret_access_key": properties.get(OSS_ACCESS_KEY_SECRET),
+ "aws_session_token": properties.get(OSS_SESSION_TOKEN),
+ }
+ config_kwargs = {"s3": {"addressing_style": "virtual"}, "signature_version": "v4"}
Review Comment:
Since the init function for OSS is already separated from S3, and users aren't able to interact with OSS without virtual addressing set to true anyway, I think it makes more sense to just set it to true by default to make it simpler.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Added `FsspecFileIO` method for OSS, virtual hosted style default to true, standardized key configurations for OSS [iceberg-python]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #1788:
URL: https://github.com/apache/iceberg-python/pull/1788#discussion_r1992191172
##########
pyiceberg/io/pyarrow.py:
##########
@@ -398,31 +402,13 @@ def _initialize_oss_fs(self) -> FileSystem:
from pyarrow.fs import S3FileSystem
client_kwargs: Dict[str, Any] = {
- "endpoint_override": self.properties.get(S3_ENDPOINT),
- "access_key": get_first_property_value(self.properties, S3_ACCESS_KEY_ID, AWS_ACCESS_KEY_ID),
- "secret_key": get_first_property_value(self.properties, S3_SECRET_ACCESS_KEY, AWS_SECRET_ACCESS_KEY),
- "session_token": get_first_property_value(self.properties, S3_SESSION_TOKEN, AWS_SESSION_TOKEN),
- "region": get_first_property_value(self.properties, S3_REGION, AWS_REGION),
Review Comment:
This is a breaking change. I don't think we can do this without a proper deprecation cycle.
If the user uses the old configuration parameters, we should emit a deprecation message and still use it.
Example can be found here: https://github.com/apache/iceberg-python/pull/1007/files#diff-24c3aa912b523fdb2afba6a0ea2dfe69fdcd05d9268e1e13ac1023ac26b54cccR175-R190
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org