Skip to content
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.

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

Fix list namespace response in rest catalog #995

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

ndrluis
Copy link
Collaborator

@ndrluis ndrluis commented Aug 4, 2024

While testing PyIceberg with Polaris and multi-level namespacing, I noticed a behavior that doesn't align with the REST catalog specification. The expected response should be the fully qualified namespace.

Copy link
Collaborator

@sungwy sungwy left a comment

Choose a reason for hiding this comment

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

Hi @ndrluis - thank you for the fix. This behavior looks correct to me as well.

We must have misinterpreted the expected behavior from the REST Catalog Open API YAML in the initial implementation.

https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L209-L215

@sungwy sungwy added this to the PyIceberg 0.7.1 release milestone Aug 5, 2024
Copy link
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

Is there another way to test this behavior? test_rest.py is using mocks. Perhaps there's a way to write an integration test comparing to spark

@ndrluis
Copy link
Collaborator Author

ndrluis commented Aug 6, 2024

Is there another way to test this behavior? test_rest.py is using mocks. Perhaps there's a way to write an integration test comparing to spark

I'll create an issue to set up an integration test suite against our current setup (using the Tabular REST catalog image).

However, I think we should add to our roadmap creating a more robust structure. I'm not sure the Tabular REST catalog image will be maintained, and it would be good to test against other catalog implementations. We'll need a simple way to disable some tests in some cases, as tests might fail due to catalog bugs, and we would rather not be blocked by that.

@ndrluis
Copy link
Collaborator Author

ndrluis commented Aug 6, 2024

Issue created #1010

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Good catch @ndrluis, thanks for fixing this!

@kevinjqliu kevinjqliu merged commit 8432b11 into apache:main Aug 6, 2024
7 checks passed
@kevinjqliu
Copy link
Contributor

Thanks @ndrluis!

@ndrluis ndrluis deleted the fix-list-namespace branch August 20, 2024 18:13
sungwy pushed a commit to sungwy/iceberg-python that referenced this pull request Dec 7, 2024
sungwy pushed a commit to sungwy/iceberg-python that referenced this pull request Dec 7, 2024
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.

4 participants