[GitHub] [iceberg] Fokko commented on a diff in pull request #6392: Python: Add adlfs support (Azure DataLake FileSystem)

[GitBox <gi...@apache.org>]

Fokko commented on code in PR #6392:
URL: https://github.com/apache/iceberg/pull/6392#discussion_r1049285936


##########
python/Makefile:
##########
@@ -26,14 +26,21 @@ lint:
 	poetry run pre-commit run --all-files
 
 test:
-	poetry run coverage run --source=pyiceberg/ -m pytest tests/ -m "not s3" ${PYTEST_ARGS}
+	poetry run coverage run --source=pyiceberg/ -m pytest tests/ -m "not s3 and not adlfs" ${PYTEST_ARGS}

Review Comment:
   I've been looking at this in https://github.com/apache/iceberg/pull/6398/ as well. I've added:
   
   ```python
   def pytest_collection_modifyitems(items, config):
       for item in items:
           if not any(item.iter_markers()):
               item.add_marker("unmarked")
   ```
   
   Which will automatically add the mark `unmarked` to each of the tests that doesn't have a marked, creating their own group. WDYT of that?



##########
python/tests/conftest.py:
##########
@@ -76,13 +76,30 @@
 
 
 def pytest_addoption(parser: pytest.Parser) -> None:
+    # S3 options
     parser.addoption(
         "--s3.endpoint", action="store", default="http://localhost:9000", help="The S3 endpoint URL for tests marked as s3"
     )
     parser.addoption("--s3.access-key-id", action="store", default="admin", help="The AWS access key ID for tests marked as s3")
     parser.addoption(
         "--s3.secret-access-key", action="store", default="password", help="The AWS secret access key ID for tests marked as s3"
     )
+    # ADLFS options
+    parser.addoption(
+        "--adlfs.endpoint",
+        action="store",
+        default="http://127.0.0.1:10000",
+        help="The ADLS endpoint URL for tests marked as adlfs",
+    )
+    parser.addoption(
+        "--adlfs.account-name", action="store", default="devstoreaccount1", help="The ADLS account key for tests marked as adlfs"
+    )
+    parser.addoption(
+        "--adlfs.account-key",
+        action="store",
+        default="Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OUzFT50uSRZ6IFsuFq2UVErCz4I6tq/K1SZFPTOtr/KBHBeksoGMGw==",

Review Comment:
   Maybe we should add this Github comment as a code comment, in case we start doing credential scanning one day.



##########
python/pyiceberg/io/fsspec.py:
##########
@@ -143,8 +153,15 @@ def open(self) -> InputStream:
 
         Returns:
             OpenFile: An fsspec compliant file-like object
+
+        Raises:
+            FileNotFoundError: If the file does not exist
         """
-        return self._fs.open(self.location, "rb")
+        try:
+            return self._fs.open(self.location, "rb")
+        except FileNotFoundError as e:
+            # To have a consistent error handling experience, make sure exception contains missing file location.

Review Comment:
   This is perfect! We don't want to return implementation-specific errors



-- 
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