qzyu999 commented on code in PR #3721: URL: https://github.com/apache/iceberg-python/pull/3721#discussion_r3694065165
########## pyiceberg/io/__init__.py: ########## @@ -41,6 +42,17 @@ logger = logging.getLogger(__name__) + +def _is_windows_drive_letter(scheme: str) -> bool: + r"""Check if a parsed URL scheme is actually a Windows drive letter. + + On Windows, Python's urlparse treats paths like 'C:\\Users\\...' as having + scheme='c'. This detects that case so callers can route to the local filesystem. + Only returns True on Windows — no behavior change on other platforms. + """ + return sys.platform == "win32" and len(scheme) == 1 and scheme.isalpha() Review Comment: Hi @jayceslesar, interesting suggestion, and I think it's reasonable. `os.path.splitdrive` however operates on full paths, but our function receives the `scheme` string after `urlparse` has already extracted it, so we only have `"c"`, not the original path. We could restructure to check the original path with `splitdrive` before calling `urlparse`, but that would change the function signature and all 3 call sites. The current approach keeps the fix minimal and contained within the existing parse flow. However, refactoring for the `splitdrive` change is possibly a cleaner design and at a first glance may remove the need for `sys.platform` and `_is_windows_drive_letter`. WDYT, should we proceed with this or keep the minimal change? -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
