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

TEST: adopt new rest catalog image and enable tableExists tests #1389

Merged
merged 4 commits into from
Dec 7, 2024

Conversation

sungwy
Copy link
Collaborator

@sungwy sungwy commented Nov 29, 2024

NOTE: the test is failing because tableExists endpoint isn't exposed on the adapter. Related fix: apache/iceberg#11678

@Fokko
Copy link
Contributor

Fokko commented Dec 4, 2024

The image is published, including the fix around the tableExists, so 🤞

@Fokko
Copy link
Contributor

Fokko commented Dec 4, 2024

Hmm, still an issue with the tableExists:

    @pytest.mark.integration
    def test_table_v1_with_null_nested_namespace(session_catalog: Catalog, arrow_table_with_null: pa.Table) -> None:
        identifier = "default.lower.table_v1_with_null_nested_namespace"
        tbl = _create_table(session_catalog, identifier, {"format-version": "1"}, [arrow_table_with_null])
        assert tbl.format_version == 1, f"Expected v1, got: v{tbl.format_version}"
    
        assert session_catalog.load_table(identifier) is not None
>       assert session_catalog.table_exists(identifier)
E       AssertionError: assert False
E        +  where False = <bound method RestCatalog.table_exists of local (<class 'pyiceberg.catalog.rest.RestCatalog'>)>('default.lower.table_v1_with_null_nested_namespace')
E        +    where <bound method RestCatalog.table_exists of local (<class 'pyiceberg.catalog.rest.RestCatalog'>)> = local (<class 'pyiceberg.catalog.rest.RestCatalog'>).table_exists

tests/integration/test_writes/test_writes.py:1373: AssertionError
=============================== warnings summary ========================

@sungwy
Copy link
Collaborator Author

sungwy commented Dec 4, 2024

:) @Fokko I'm on my way to the office, but will take a look at this in a few minutes

@sungwy sungwy marked this pull request as ready for review December 4, 2024 13:48
@sungwy
Copy link
Collaborator Author

sungwy commented Dec 4, 2024

@Fokko - it looks like the issue is because the rest-fixture is returning a 200 response. I've put in edits to allow PyIceberg to infer a 200 response as being successful.

The proposed solution deviates from the REST Catalog Spec, but this is already the behavior of PyIceberg when handles the success codes of the other endpoints that are also meant to return 204 response:

def _handle_non_200_response(self, exc: HTTPError, error_handler: Dict[int, Type[Exception]]) -> None:
exception: Type[Exception]
if exc.response is None:
raise ValueError("Did not receive a response")
code = exc.response.status_code
if code in error_handler:
exception = error_handler[code]
elif code == 400:
exception = BadRequestError
elif code == 401:
exception = UnauthorizedError
elif code == 403:
exception = ForbiddenError
elif code == 422:
exception = RESTError
elif code == 419:
exception = AuthorizationExpiredError
elif code == 501:
exception = NotImplementedError
elif code == 503:
exception = ServiceUnavailableError
elif 500 <= code < 600:
exception = ServerError
else:
exception = RESTError

@Fokko
Copy link
Contributor

Fokko commented Dec 4, 2024

Thanks for following this up @sungwy. I think it is okay to mark all 2xx responses as table exists. Polaris had a similar issue: #1363

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.

woot! LGTM

@@ -887,7 +887,7 @@ def table_exists(self, identifier: Union[str, Identifier]) -> bool:

if response.status_code == 404:
return False
elif response.status_code == 204:
elif response.status_code in (200, 204):
Copy link
Contributor

Choose a reason for hiding this comment

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

should we upstream this to the spec?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sungwy sungwy merged commit 6714257 into apache:main Dec 7, 2024
8 checks passed
@sungwy sungwy deleted the iceberg-rest-fixture-image branch December 7, 2024 02:36
@sungwy
Copy link
Collaborator Author

sungwy commented Dec 7, 2024

Thanks for the reviews @Fokko and @kevinjqliu !

sungwy added a commit to sungwy/iceberg-python that referenced this pull request Dec 7, 2024
…he#1389)

* test new rest catalog image

* Point to `iceberg-rest-fixture`

* allow 200 response in table_exists

* be graceful in handling 200 response in table_exists

---------

Co-authored-by: Fokko Driesprong <[email protected]>
sungwy added a commit to sungwy/iceberg-python that referenced this pull request Dec 7, 2024
…he#1389)

* test new rest catalog image

* Point to `iceberg-rest-fixture`

* allow 200 response in table_exists

* be graceful in handling 200 response in table_exists

---------

Co-authored-by: Fokko Driesprong <[email protected]>
sungwy added a commit to sungwy/iceberg-python that referenced this pull request Dec 24, 2024
…he#1389)

* test new rest catalog image

* Point to `iceberg-rest-fixture`

* allow 200 response in table_exists

* be graceful in handling 200 response in table_exists

---------

Co-authored-by: Fokko Driesprong <[email protected]>
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.

3 participants