You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Fokko (via GitHub)" <gi...@apache.org> on 2023/09/11 14:55:21 UTC
[GitHub] [iceberg] Fokko opened a new pull request, #8548: Python: Inline the Filesystem imports
Fokko opened a new pull request, #8548:
URL: https://github.com/apache/iceberg/pull/8548
It can be that certain build flags are turned off:
```
-DARROW_GCS=ON: Build Arrow with GCS support (requires the GCloud SDK for C++)
-DARROW_HDFS=ON: Arrow integration with libhdfs for accessing the Hadoop Filesystem
```
From: https://arrow.apache.org/docs/dev/developers/cpp/building.html#optional-components
This will cause an ImportError when importing `pyarrow.py`, while it can be that you don't want to use a missing FS. Therefore it is better to inline the imports
--
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
[GitHub] [iceberg] Fokko commented on a diff in pull request #8548: Python: Inline the Filesystem imports
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #8548:
URL: https://github.com/apache/iceberg/pull/8548#discussion_r1322577107
##########
python/pyiceberg/io/pyarrow.py:
##########
@@ -317,6 +312,8 @@ def _get_fs(self, scheme: str) -> FileSystem:
if proxy_uri := self.properties.get(S3_PROXY_URI):
client_kwargs["proxy_options"] = proxy_uri
+ from pyarrow.fs import S3FileSystem
Review Comment:
Good one, I think I like that more as well 👍🏻
##########
python/pyiceberg/io/pyarrow.py:
##########
@@ -317,6 +312,8 @@ def _get_fs(self, scheme: str) -> FileSystem:
if proxy_uri := self.properties.get(S3_PROXY_URI):
client_kwargs["proxy_options"] = proxy_uri
+ from pyarrow.fs import S3FileSystem
Review Comment:
Good one, I think I like that more as well 👍🏻
--
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
[GitHub] [iceberg] amogh-jahagirdar commented on a diff in pull request #8548: Python: Inline the Filesystem imports
Posted by "amogh-jahagirdar (via GitHub)" <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #8548:
URL: https://github.com/apache/iceberg/pull/8548#discussion_r1321879233
##########
python/pyiceberg/io/pyarrow.py:
##########
@@ -329,6 +326,8 @@ def _get_fs(self, scheme: str) -> FileSystem:
hdfs_kwargs["user"] = user
if kerb_ticket := self.properties.get(HDFS_KERB_TICKET):
hdfs_kwargs["kerb_ticket"] = kerb_ticket
+ from pyarrow.fs import HadoopFileSystem
Review Comment:
Same nit as above, I think we can move it right after the elif
##########
python/pyiceberg/io/pyarrow.py:
##########
@@ -317,6 +312,8 @@ def _get_fs(self, scheme: str) -> FileSystem:
if proxy_uri := self.properties.get(S3_PROXY_URI):
client_kwargs["proxy_options"] = proxy_uri
+ from pyarrow.fs import S3FileSystem
Review Comment:
I guess we can move it to line 304 (after the if confirms we need to use the S3FileSystem)? feels a bit more readable rather than importing more lazily. but not strongly opinionated
--
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
[GitHub] [iceberg] Fokko merged pull request #8548: Python: Inline the Filesystem imports
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #8548:
URL: https://github.com/apache/iceberg/pull/8548
--
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
[GitHub] [iceberg] Fokko commented on pull request #8548: Python: Inline the Filesystem imports
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #8548:
URL: https://github.com/apache/iceberg/pull/8548#issuecomment-1715162099
This also shaves off 50ms wall clock when doing the imports 😱
--
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
[GitHub] [iceberg] Fokko commented on pull request #8548: Python: Inline the Filesystem imports
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #8548:
URL: https://github.com/apache/iceberg/pull/8548#issuecomment-1715249604
Appreciate the quick review @amogh-jahagirdar Thanks! 🙌🏻
--
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