Skip to content

Use ObjectStoreLocationProvider by default #1509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Fokko merged 3 commits into from
Jan 13, 2025
Merged

Use ObjectStoreLocationProvider by default #1509

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
30c77fc
Make object storage the default location provider
Jan 11, 2025
2856e53
Nit: Remove comment beside property to prefer docs
Jan 11, 2025
9980b39
Nit: Add asserts for table properties defaults as well as comment in …
Jan 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pyiceberg/table/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,7 @@ class TableProperties:
WRITE_PY_LOCATION_PROVIDER_IMPL = "write.py-location-provider.impl"

OBJECT_STORE_ENABLED = "write.object-storage.enabled"
OBJECT_STORE_ENABLED_DEFAULT = False
OBJECT_STORE_ENABLED_DEFAULT = True # Differs from the Java implementation
Copy link
Copy Markdown
Contributor

@kevinjqliu kevinjqliu Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: instead of the comment here, let's add it to the docs

Copy link
Copy Markdown
Contributor Author

@smaheshwar-pltr smaheshwar-pltr Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! I've remove for now.

Then we can add all the new location-relevant write options introduced in #1452 when we address #1510 together with the other docs?


WRITE_OBJECT_STORE_PARTITIONED_PATHS = "write.object-storage.partitioned-paths"
WRITE_OBJECT_STORE_PARTITIONED_PATHS_DEFAULT = True
Expand Down
4 changes: 2 additions & 2 deletions tests/integration/test_writes/test_partitioned_writes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,8 +297,8 @@ def test_object_storage_location_provider_excludes_partition_path(
tbl = _create_table(
session_catalog=session_catalog,
identifier=f"default.arrow_table_v{format_version}_with_null_partitioned_on_col_{part_col}",
# write.object-storage.partitioned-paths defaults to True
properties={"format-version": str(format_version), TableProperties.OBJECT_STORE_ENABLED: True},
# Both write.object-storage.enabled and write.object-storage.partitioned-paths default to True
Copy link
Copy Markdown
Contributor Author

@smaheshwar-pltr smaheshwar-pltr Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just changing this one integration test to test that it's now the default; I've kept test_writes::test_object_storage_data_files unchanged.

Copy link
Copy Markdown
Contributor

@kevinjqliu kevinjqliu Jan 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wydt about adding assert statements as well? comments can become stale over time

Copy link
Copy Markdown
Contributor Author

@smaheshwar-pltr smaheshwar-pltr Jan 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM, done

properties={"format-version": str(format_version)},
data=[arrow_table_with_null],
partition_spec=partition_spec,
)
Expand Down
7 changes: 3 additions & 4 deletions tests/table/test_locations.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def new_data_location(self, data_file_name: str, partition_key: Optional[Partiti


def test_default_location_provider() -> None:
provider = load_location_provider(table_location="table_location", table_properties=EMPTY_DICT)
provider = load_location_provider(table_location="table_location", table_properties={"write.object-storage.enabled": "false"})

assert provider.new_data_location("my_file") == "table_location/data/my_file"

Expand All @@ -66,7 +66,7 @@ def test_custom_location_provider_not_found() -> None:


def test_object_storage_injects_entropy() -> None:
provider = load_location_provider(table_location="table_location", table_properties={"write.object-storage.enabled": "true"})
provider = load_location_provider(table_location="table_location", table_properties=EMPTY_DICT)

location = provider.new_data_location("test.parquet")
parts = location.split("/")
Expand Down Expand Up @@ -104,7 +104,6 @@ def test_object_storage_partitioned_paths_disabled(partition_key: Optional[Parti
provider = load_location_provider(
table_location="table_location",
table_properties={
"write.object-storage.enabled": "true",
"write.object-storage.partitioned-paths": "false",
},
)
Expand All @@ -125,6 +124,6 @@ def test_object_storage_partitioned_paths_disabled(partition_key: Optional[Parti
],
)
def test_hash_injection(data_file_name: str, expected_hash: str) -> None:
provider = load_location_provider(table_location="table_location", table_properties={"write.object-storage.enabled": "true"})
provider = load_location_provider(table_location="table_location", table_properties=EMPTY_DICT)

assert provider.new_data_location(data_file_name) == f"table_location/data/{expected_hash}/{data_file_name}"