From 4a4baeac0436aa3777bb54f46d2f9cdca530b01d Mon Sep 17 00:00:00 2001 From: Anas Khan <83116240+anxkhn@users.noreply.github.com> Date: Sat, 4 Jul 2026 21:47:20 +0530 Subject: [PATCH 1/2] fix: build StaticTable version-hint metadata locations with forward slashes StaticTable._metadata_location_from_version_hint built the version-hint and resolved metadata locations with os.path.join. Iceberg locations are URIs and must always be forward-slash separated, but os.path.join uses the OS separator, so on Windows it produced backslash-separated keys such as s3://.../taxis\metadata\version-hint.text. That is an invalid object-store key, which breaks the documented "pass the table root path" usage of StaticTable.from_metadata on Windows. Join the components with explicit forward slashes, matching how locations are built elsewhere in pyiceberg (for example pyiceberg/table/locations.py). Behavior is unchanged on POSIX. The now-unused os import is removed. Add an OS-independent regression test that swaps os.path.join for the Windows join so the metadata location is asserted to stay a valid forward-slash URI on any platform (the existing test only exercised local temp paths, which use the POSIX separator on Linux CI and never caught this). Related: #2477 --- pyiceberg/table/__init__.py | 10 ++++----- tests/table/test_init.py | 43 +++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/pyiceberg/table/__init__.py b/pyiceberg/table/__init__.py index 303b3db135..45ef6bc88d 100644 --- a/pyiceberg/table/__init__.py +++ b/pyiceberg/table/__init__.py @@ -18,7 +18,6 @@ import itertools import logging -import os import random import time import uuid @@ -1963,7 +1962,8 @@ def refresh(self) -> Table: @classmethod def _metadata_location_from_version_hint(cls, metadata_location: str, properties: Properties = EMPTY_DICT) -> str: - version_hint_location = os.path.join(metadata_location, "metadata", "version-hint.text") + metadata_dir = f"{metadata_location.rstrip('/')}/metadata" + version_hint_location = f"{metadata_dir}/version-hint.text" io = load_file_io(properties=properties, location=version_hint_location) file = io.new_input(version_hint_location) @@ -1971,11 +1971,11 @@ def _metadata_location_from_version_hint(cls, metadata_location: str, properties content = stream.read().decode("utf-8") if content.endswith(".metadata.json"): - return os.path.join(metadata_location, "metadata", content) + return f"{metadata_dir}/{content}" elif content.isnumeric(): - return os.path.join(metadata_location, "metadata", f"v{content}.metadata.json") + return f"{metadata_dir}/v{content}.metadata.json" else: - return os.path.join(metadata_location, "metadata", f"{content}.metadata.json") + return f"{metadata_dir}/{content}.metadata.json" @classmethod def from_metadata(cls, metadata_location: str, properties: Properties = EMPTY_DICT) -> StaticTable: diff --git a/tests/table/test_init.py b/tests/table/test_init.py index 3bc70afc8b..af933e1857 100644 --- a/tests/table/test_init.py +++ b/tests/table/test_init.py @@ -590,6 +590,49 @@ def test_static_table_version_hint_same_as_table( assert static_table.metadata == table_v2.metadata +@pytest.mark.parametrize( + "version_hint_content, expected_metadata_file", + [ + ("v3.metadata.json", "v3.metadata.json"), + ("3", "v3.metadata.json"), + ("some-uuid", "some-uuid.metadata.json"), + ], +) +def test_static_table_version_hint_location_uses_forward_slashes( + version_hint_content: str, expected_metadata_file: str, monkeypatch: pytest.MonkeyPatch +) -> None: + # Iceberg locations are URIs and must always be forward-slash separated, regardless of the host OS. + table_root = "s3://warehouse/wh/nyc.db/taxis" + requested_locations: list[str] = [] + + class _FakeStream: + def __enter__(self) -> "_FakeStream": + return self + + def __exit__(self, *args: Any) -> None: + return None + + def read(self, size: int = 0) -> bytes: + return version_hint_content.encode("utf-8") + + class _FakeInputFile: + def open(self, seekable: bool = True) -> "_FakeStream": + return _FakeStream() + + class _FakeFileIO: + def new_input(self, location: str) -> "_FakeInputFile": + requested_locations.append(location) + return _FakeInputFile() + + monkeypatch.setattr("pyiceberg.table.load_file_io", lambda *args, **kwargs: _FakeFileIO()) + + resolved_location = StaticTable._metadata_location_from_version_hint(table_root) + + assert requested_locations == [f"{table_root}/metadata/version-hint.text"] + assert resolved_location == f"{table_root}/metadata/{expected_metadata_file}" + assert "\\" not in resolved_location + + def test_static_table_io_does_not_exist(metadata_location: str) -> None: with pytest.raises(ValueError): StaticTable.from_metadata(metadata_location, {PY_IO_IMPL: "pyiceberg.does.not.exist.FileIO"}) From 2f32f6e0ba79f91772625db7ec06ec6f23ca3a79 Mon Sep 17 00:00:00 2001 From: Kevin Liu Date: Fri, 23 Oct 2026 22:59:24 -0700 Subject: [PATCH 2/2] Test version-hint resolution with real files instead of fakes Replace the fake FileIO test with one that writes metadata and version-hint files under a file:// URI and asserts the resolved metadata location. Windows CI catches a regression to os.path.join. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- pyiceberg/table/__init__.py | 1 + tests/table/test_init.py | 53 +++++++++++-------------------------- 2 files changed, 17 insertions(+), 37 deletions(-) diff --git a/pyiceberg/table/__init__.py b/pyiceberg/table/__init__.py index 45ef6bc88d..a226ec66d4 100644 --- a/pyiceberg/table/__init__.py +++ b/pyiceberg/table/__init__.py @@ -1962,6 +1962,7 @@ def refresh(self) -> Table: @classmethod def _metadata_location_from_version_hint(cls, metadata_location: str, properties: Properties = EMPTY_DICT) -> str: + # Using os.path.join here would produce "\" separators on Windows, breaking URI locations metadata_dir = f"{metadata_location.rstrip('/')}/metadata" version_hint_location = f"{metadata_dir}/version-hint.text" io = load_file_io(properties=properties, location=version_hint_location) diff --git a/tests/table/test_init.py b/tests/table/test_init.py index af933e1857..725b9513b6 100644 --- a/tests/table/test_init.py +++ b/tests/table/test_init.py @@ -18,6 +18,7 @@ import json import uuid from copy import copy +from pathlib import Path from typing import Any import pytest @@ -37,6 +38,7 @@ from pyiceberg.io import PY_IO_IMPL, FileIO, load_file_io from pyiceberg.partitioning import PartitionField, PartitionSpec from pyiceberg.schema import Schema +from pyiceberg.serializers import ToOutputFile from pyiceberg.table import ( CommitTableRequest, StagedTable, @@ -590,47 +592,24 @@ def test_static_table_version_hint_same_as_table( assert static_table.metadata == table_v2.metadata -@pytest.mark.parametrize( - "version_hint_content, expected_metadata_file", - [ +def test_static_table_version_hint_uri_location(table_v2: Table, tmp_path: Path) -> None: + # Fails on Windows if the locations are built with os.path.join + table_root = f"file://{tmp_path}" + file_io = load_file_io(location=table_root) + + for version_hint_content, metadata_file in [ ("v3.metadata.json", "v3.metadata.json"), ("3", "v3.metadata.json"), ("some-uuid", "some-uuid.metadata.json"), - ], -) -def test_static_table_version_hint_location_uses_forward_slashes( - version_hint_content: str, expected_metadata_file: str, monkeypatch: pytest.MonkeyPatch -) -> None: - # Iceberg locations are URIs and must always be forward-slash separated, regardless of the host OS. - table_root = "s3://warehouse/wh/nyc.db/taxis" - requested_locations: list[str] = [] - - class _FakeStream: - def __enter__(self) -> "_FakeStream": - return self - - def __exit__(self, *args: Any) -> None: - return None - - def read(self, size: int = 0) -> bytes: - return version_hint_content.encode("utf-8") - - class _FakeInputFile: - def open(self, seekable: bool = True) -> "_FakeStream": - return _FakeStream() - - class _FakeFileIO: - def new_input(self, location: str) -> "_FakeInputFile": - requested_locations.append(location) - return _FakeInputFile() - - monkeypatch.setattr("pyiceberg.table.load_file_io", lambda *args, **kwargs: _FakeFileIO()) - - resolved_location = StaticTable._metadata_location_from_version_hint(table_root) + ]: + metadata_location = f"{table_root}/metadata/{metadata_file}" + ToOutputFile.table_metadata(table_v2.metadata, file_io.new_output(metadata_location), overwrite=True) + with file_io.new_output(f"{table_root}/metadata/version-hint.text").create(overwrite=True) as f: + f.write(version_hint_content.encode("utf-8")) - assert requested_locations == [f"{table_root}/metadata/version-hint.text"] - assert resolved_location == f"{table_root}/metadata/{expected_metadata_file}" - assert "\\" not in resolved_location + static_table = StaticTable.from_metadata(table_root) + assert static_table.metadata_location == metadata_location + assert static_table.metadata == table_v2.metadata def test_static_table_io_does_not_exist(metadata_location: str) -> None: