Skip to content

REST: Preserve view metadata location - #4088

Open
Soumo-git-hub wants to merge 1 commit into
apache:mainfrom
Soumo-git-hub:fix/4073-view-metadata-location
Open

Soumo-git-hub wants to merge 1 commit into
apache:mainfrom
Soumo-git-hub:fix/4073-view-metadata-location

Conversation

@Soumo-git-hub

Copy link
Copy Markdown

Closes #4073

Rationale for this change

REST view responses include metadata-location, but PyIceberg currently drops this value when constructing a View. As a result, callers cannot access the metadata file location returned by the REST catalog.

This change preserves the response value on View.metadata_location.

What changed

  • Add an optional metadata_location attribute to View while preserving the existing constructor argument order.
  • Propagate ViewResponse.metadata_location through RestCatalog._response_to_view.
  • Add regression coverage for create_view, load_view, and the shared register_view response path.

Are these changes tested?

Yes.

  • python -m pytest tests/catalog/test_rest.py -m unit -v -p no:cacheprovider -k "test_create_view_200 or test_load_view_200 or test_register_view_200" — 3 passed
  • python -m pytest tests/catalog/test_rest.py -m unit -v -p no:cacheprovider — 167 passed

Are there any user-facing changes?

Yes. View now exposes the metadata location returned by the REST catalog through View.metadata_location.

@rambleraptor rambleraptor left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This looks great! Thanks for contributing this fix!

_identifier: Identifier
metadata: ViewMetadata
config: dict[str, str]
metadata_location: str | None

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: The REST spec marks metadata-location as required, but ViewResponse already makes it optional, so str | None is consistent. Could we document when it's None?


def __eq__(self, other: Any) -> bool:
"""Return the equality of two instances of the View class."""
return self.name() == other.name() and self.metadata == other.metadata if isinstance(other, View) else False

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: __eq__ ignores metadata_location. That seems intentional, but a short comment saying so might be helpful.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

View does not expose metadata_location: RestCatalog.load_view discards it from the server's response

3 participants