Implementing namespace_exists function on the REST Catalog by AhmedNader42 · Pull Request #1434 · apache/iceberg-python

AhmedNader42 · 2024-12-16T15:32:17Z

As per #1430 I have added the namespace_exists functionality for REST Catalog.

Here's an example usage similar to the table_exists function of the same class

Please review the changes

- Added the relevant unit tests

kevinjqliu · 2024-12-16T16:39:20Z

tests/catalog/test_rest.py

+def test_namespace_exists_200(rest_mock: Mocker) -> None:
+    rest_mock.head(
+        f"{TEST_URI}v1/namespaces/fokko",
+        status_code=200,
+        request_headers=TEST_HEADERS,
+    )
+    catalog = RestCatalog("rest", uri=TEST_URI, token=TEST_TOKEN)
+
+    assert catalog.namespace_exists("fokko")


nit: the tests here are only verifying against the mock response.
it would be great to test against the actual REST catalog in integration tests.

i noticed that does not exist right now, perhaps we can start a tests/integration/test_rest_catalog.py

Yes, Good point! I was looking for how to properly test this but didn't find any tests touching the REST Catalog. I thought it was a bit out of scope for the initial issue to add a new integration test flow.

Maybe we should open a new enhancement and add the details to it to implement a new integration test for REST Catalog at tests/integration/test_rest_catalog.py like you mentioned. Thoughts? I'm willing to pick it up

sungwy · 2024-12-16T21:01:08Z

Hi @AhmedNader42 - thank you very much for picking up this issue and getting a working solution up already!

I'm in agreement with @kevinjqliu 's comment, that it would be great to have an integration test as well, and I think it is well within the scope of this PR.

https://github.com/apache/iceberg-python/blob/main/tests/integration/test_reads.py already uses session_catalog in its tests, which is a rest catalog fixture defined in the conftest.py, and I think it would be a good place to add this test case.

AhmedNader42 · 2024-12-17T08:07:59Z

Hello @sungwy, Thank you for pointing that out! Sounds good to me. I'll work on adding some test cases for REST Catalog in test_reads.py

Hi @AhmedNader42 - thank you very much for picking up this issue and getting a working solution up already!

I'm in agreement with @kevinjqliu 's comment, that it would be great to have an integration test as well, and I think it is well within the scope of this PR.

https://github.com/apache/iceberg-python/blob/main/tests/integration/test_reads.py already uses session_catalog in its tests, which is a rest catalog fixture defined in the conftest.py, and I think it would be a good place to add this test case.

AhmedNader42 · 2024-12-17T18:59:52Z

I created a new file for REST Catalog integration tests tests/integration/test_rest_catalog.py as suggestsed by @kevinjqliu and included some tests covering the namespace_exists functionality. Kindly re-check

kevinjqliu

LGTM! Thank you for adding the integration test for rest catalog, much needed :)

I double-checked with namespaceExists in the REST spec, everything looks good.
Similar to tableExists, we're including 200 status code as a convenience. I'll see if we can add this to the spec itself

kevinjqliu · 2024-12-17T20:18:28Z

@AhmedNader42 looks like the RAT check failed. For new files, we need to include the ASF license on top, like

iceberg-python/tests/catalog/test_rest.py

Lines 1 to 16 in ceffe08

    
           #  Licensed to the Apache Software Foundation (ASF) under one 
        
           #  or more contributor license agreements.  See the NOTICE file 
        
           #  distributed with this work for additional information 
        
           #  regarding copyright ownership.  The ASF licenses this file 
        
           #  to you under the Apache License, Version 2.0 (the 
        
           #  "License"); you may not use this file except in compliance 
        
           #  with the License.  You may obtain a copy of the License at 
        
           # 
        
           #    http://www.apache.org/licenses/LICENSE-2.0 
        
           # 
        
           #  Unless required by applicable law or agreed to in writing, 
        
           #  software distributed under the License is distributed on an 
        
           #  "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY 
        
           #  KIND, either express or implied.  See the License for the 
        
           #  specific language governing permissions and limitations 
        
           #  under the License.

Fokko · 2024-12-17T20:23:53Z

tests/integration/test_rest_catalog.py

+
+from pyiceberg.catalog.rest import RestCatalog
+
+TEST_NAMESPACE_IDENTIFIER = "TEST NS"


Hmm, I didn't know that spaces are allowed :D

kevinjqliu · 2024-12-17T21:01:28Z

Thanks for the contribution, @AhmedNader42 !
I created #1439 so that we can add more integration tests for the REST catalog.
I also started this devlist discussion to propose adding 200 status code as a valid response for HEAD requests.

* - Added the namespace_exists function in the RESTCatalog - Added the relevant unit tests * - Removed docstring to match other namespace functions * - Added integration test for REST Catalog namespace_exists functionality * - Added ASF license to test_rest_catalog.py to recover from failing test

- Added the namespace_exists function in the RESTCatalog

0d01759

- Added the relevant unit tests

AhmedNader42 mentioned this pull request Dec 16, 2024

Implement namespace_exists function on the REST Catalog #1430

Closed

- Removed docstring to match other namespace functions

d1e643c

kevinjqliu reviewed Dec 16, 2024

View reviewed changes

- Added integration test for REST Catalog namespace_exists functionality

ceffe08

kevinjqliu approved these changes Dec 17, 2024

View reviewed changes

- Added ASF license to test_rest_catalog.py to recover from failing test

fcd1111

Fokko approved these changes Dec 17, 2024

View reviewed changes

kevinjqliu mentioned this pull request Dec 17, 2024

Add REST catalog integration tests #1439

Closed

kevinjqliu merged commit d8c6c94 into apache:main Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Implementing namespace_exists function on the REST Catalog#1434

Implementing namespace_exists function on the REST Catalog#1434
kevinjqliu merged 4 commits intoapache:mainfrom
AhmedNader42:adding-namespace-exists

AhmedNader42 commented Dec 16, 2024 •

edited

Loading

Uh oh!

kevinjqliu Dec 16, 2024

Uh oh!

AhmedNader42 Dec 16, 2024

Uh oh!

sungwy commented Dec 16, 2024

Uh oh!

AhmedNader42 commented Dec 17, 2024

Uh oh!

AhmedNader42 commented Dec 17, 2024

Uh oh!

kevinjqliu left a comment

Uh oh!

kevinjqliu commented Dec 17, 2024

Uh oh!

Fokko Dec 17, 2024

Uh oh!

kevinjqliu commented Dec 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants


		from pyiceberg.catalog.rest import RestCatalog

		TEST_NAMESPACE_IDENTIFIER = "TEST NS"

Comments

Conversation

AhmedNader42 commented Dec 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinjqliu Dec 16, 2024

Choose a reason for hiding this comment

Uh oh!

AhmedNader42 Dec 16, 2024

Choose a reason for hiding this comment

Uh oh!

sungwy commented Dec 16, 2024

Uh oh!

AhmedNader42 commented Dec 17, 2024

Uh oh!

AhmedNader42 commented Dec 17, 2024

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

kevinjqliu commented Dec 17, 2024

Uh oh!

Fokko Dec 17, 2024

Choose a reason for hiding this comment

Uh oh!

kevinjqliu commented Dec 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

AhmedNader42 commented Dec 16, 2024 •

edited

Loading