Copilot commented on code in PR #6545:
URL: https://github.com/apache/texera/pull/6545#discussion_r3609792951
##########
amber/src/test/python/core/storage/iceberg/test_iceberg_utils_catalog.py:
##########
@@ -96,3 +100,141 @@ def test_postgres_uri_is_built_with_pg8000_scheme(self):
)
# And warehouse is still the plain path.
assert kwargs["warehouse"] == warehouse_path
+
+
+# FileIO selected for Windows-local warehouses in place of the default pyarrow
+# FileIO; its fsspec LocalFileSystem parses Windows drive paths.
+_FSSPEC_FILE_IO = "pyiceberg.io.fsspec.FsspecFileIO"
+
Review Comment:
The test hard-codes the FsspecFileIO implementation string, duplicating the
production constant in `iceberg_utils.py`. This can drift if the implementation
path changes; tests can instead reference the module constant.
##########
amber/src/main/python/core/storage/iceberg/iceberg_utils.py:
##########
@@ -35,6 +39,57 @@
# Suffix used to encode LARGE_BINARY fields in Iceberg (must match Scala
IcebergUtil)
LARGE_BINARY_FIELD_SUFFIX = "__texera_large_binary_ptr"
+# pyiceberg FileIO whose fsspec LocalFileSystem handles Windows drive paths,
+# which the default pyarrow FileIO cannot. See `_is_windows_local_warehouse`.
+_FSSPEC_FILE_IO = "pyiceberg.io.fsspec.FsspecFileIO"
+
+# Matches a filesystem path that starts with a Windows drive letter and a
+# separator, e.g. `C:\...` or `C:/...` (but not the drive-relative `C:foo`).
+_WINDOWS_DRIVE_PATH = re.compile(r"^[A-Za-z]:[\\/]")
+
+
+def _is_windows_local_warehouse(warehouse: str) -> bool:
+ """
+ Whether ``warehouse`` is a LOCAL filesystem path carrying a Windows drive
+ letter, either as a bare path (``C:\\...`` / ``C:/...``) or a ``file://``
+ URI (``file:///C:/...``).
+
+ Remote object stores (``s3://...``), POSIX paths, drive-less relative
paths,
+ and colon-stripped paths (the form the Scala side registers, ``C/...``) are
+ excluded, so the default pyarrow FileIO / plain-path convention that Linux,
+ macOS, CI, and the Scala engine rely on is left untouched (see #4409).
+ """
+ if not warehouse:
+ return False
+ text = str(warehouse)
+ if _WINDOWS_DRIVE_PATH.match(text):
+ return True
+ if text.startswith("file://"):
+ return bool(_WINDOWS_DRIVE_PATH.match(urlparse(text).path.lstrip("/")))
+ return False
Review Comment:
`file` URI schemes are case-insensitive, but `_is_windows_local_warehouse`
only recognizes lowercase `file://` via `startswith`. Using
`urlparse(...).scheme.lower() == "file"` makes the gate robust to inputs like
`FILE:///C:/...` without widening it beyond file URIs.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
