table_exists unit/integration test for NoSuchTableError #678

MehulBatra · Apr 30, 2024

Implementation of table_exists method at Initialization level already exists bypassing it.
Unit Test for glue, sqlcatalog and dynamodb.
Integration test for glue and dynamodb.

kevinjqliu

lgtm! thanks for adding tests for different catalogs. I left a few minor comments

kevinjqliu · May 1, 2024

pyiceberg/catalog/__init__.py

        Returns:
            bool: True if the table exists, False otherwise.
        """
+        try:


can we get rid of the other table_exists function in the same file?

iceberg-python/pyiceberg/catalog/__init__.py

Lines 668 to 673 in 87c13b5

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

try:

self.load_table(identifier)

return True

except NoSuchTableError:

return False

what do you think about changing the REST catalog's table_exists implementation to be similar to this?
https://github.com/apache/iceberg-python/pull/512/files#diff-3bda7391ebd8aa3dcfd6703d8d2764830b9d9c35fa854188a37d69611274bd3dR722-R727

maybe calls super().table_exists()?

I think it is better for RestCatalog to maintain a separate implementation to make a head request to /v1/{prefix}/namespaces/{namespace}/tables/{table}, ref: #512 (comment), #507 (comment)

The try-catch implementation is for other non-rest catalogs and thus it is put in the MetastoreCatalog instead of the Catalog interface.

A little bit more context regarding the MetastoreCatalog: while working on #498, we found that many helper functions as well as some implementations are for non-rest catalogs only. So we decide to move those to another layer, MetastoreCatalog to make the inheritance look better: #498 (comment).

Do these sound reasonable to you?

makes sense, thanks for the detailed explanation

pushed the changes, as per the discussion, thank you, guys, for the feedback!

HonahX

Thanks for working on this! I think we've had the implementation of table_exists.

For non-rest catalog it is

iceberg-python/pyiceberg/catalog/__init__.py

Lines 668 to 673 in 87c13b5

    
           def table_exists(self, identifier: Union[str, Identifier]) -> bool: 
        
               try: 
        
                   self.load_table(identifier) 
        
                   return True 
        
               except NoSuchTableError: 
        
                   return False

as pointed out by @kevinjqliu

For rest catalog it is

iceberg-python/pyiceberg/catalog/rest.py

Lines 778 to 791 in a892309

    
               def table_exists(self, identifier: Union[str, Identifier]) -> bool: 
        
                   """Check if a table exists. 
        
                   Args: 
        
                       identifier (str | Identifier): Table identifier. 
        
                   Returns: 
        
                       bool: True if the table exists, False otherwise. 
        
                   """ 
        
                   identifier_tuple = self.identifier_to_tuple_without_catalog(identifier) 
        
                   response = self._session.head( 
        
                       self.url(Endpoints.load_table, prefixed=True, **self._split_identifier_for_path(identifier_tuple)) 
        
                   ) 
        
                   return response.status_code == 200

But we do miss relevant tests for glue, sql, dynamodb, ...

pyiceberg/catalog/__init__.py

tests/catalog/integration_test_dynamodb.py

Co-authored-by: Honah J. <undefined.newdb.newtable@gmail.com>

HonahX

Overall LGTM! The lint issue can be resolved by executing make lint

MehulBatra · May 3, 2024

Overall LGTM! The lint issue can be resolved by executing make lint

Thank you @HonahX and @kevinjqliu for all the feedback!

HonahX

Thanks for the work! This will be good to go when the CI pass.

kevinjqliu · May 3, 2024

Thank you for the PR! Do you mind changing the PR title and description to match?

HonahX · May 4, 2024

Thanks everyone! Merged!

check if table exists return true else false

87c13b5

MehulBatra mentioned this pull request Apr 30, 2024

Add table_exists method to the Catalog #507

Closed

kevinjqliu reviewed May 1, 2024

View reviewed changes

HonahX reviewed May 1, 2024

View reviewed changes

check if table exists return true else false

cad055e

HonahX reviewed May 2, 2024

View reviewed changes

pyiceberg/catalog/__init__.py Outdated Show resolved Hide resolved

tests/catalog/integration_test_dynamodb.py Outdated Show resolved Hide resolved

Update tests/catalog/integration_test_dynamodb.py

be4bb3e

Co-authored-by: Honah J. <undefined.newdb.newtable@gmail.com>

HonahX reviewed May 3, 2024

View reviewed changes

pick docstring from superclass

0e135bc

HonahX approved these changes May 3, 2024

View reviewed changes

fix linting

de4031e

MehulBatra changed the title ~~Implement table_exists method with try-catch for NoSuchTableError~~ table_exists unit/integration test for NoSuchTableError May 3, 2024

HonahX merged commit 7bd5d9e into apache:main May 4, 2024

	def table_exists(self, identifier: Union[str, Identifier]) -> bool:
	try:
	self.load_table(identifier)
	return True
	except NoSuchTableError:
	return False

	def table_exists(self, identifier: Union[str, Identifier]) -> bool:
	"""Check if a table exists.

	Args:
	identifier (str \| Identifier): Table identifier.

	Returns:
	bool: True if the table exists, False otherwise.
	"""
	identifier_tuple = self.identifier_to_tuple_without_catalog(identifier)
	response = self._session.head(
	self.url(Endpoints.load_table, prefixed=True, **self._split_identifier_for_path(identifier_tuple))
	)
	return response.status_code == 200

Search code, repositories, users, issues, pull requests...

table_exists unit/integration test for NoSuchTableError #678

table_exists unit/integration test for NoSuchTableError #678

Uh oh!

Conversation

MehulBatra commented Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

kevinjqliu May 1, 2024

Choose a reason for hiding this comment

Uh oh!

kevinjqliu May 1, 2024

Choose a reason for hiding this comment

Uh oh!

HonahX May 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinjqliu May 1, 2024

Choose a reason for hiding this comment

Uh oh!

MehulBatra May 1, 2024

Choose a reason for hiding this comment

Uh oh!

HonahX left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

HonahX left a comment

Choose a reason for hiding this comment

Uh oh!

MehulBatra commented May 3, 2024

Uh oh!

HonahX left a comment

Choose a reason for hiding this comment

Uh oh!

kevinjqliu commented May 3, 2024

Uh oh!

HonahX commented May 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

MehulBatra commented Apr 30, 2024 •

edited

Loading

HonahX May 1, 2024 •

edited

Loading