Hive: Optimize tableExists API in hive catalog by dramaticlly · Pull Request #11597 · apache/iceberg

dramaticlly · 2024-11-20T00:22:31Z

Skip creation of hive table operation when check existence of iceberg table in hive catalog.

Today the table existence rely on load the table first and return true if table can be loaded

iceberg/api/src/main/java/org/apache/iceberg/catalog/Catalog.java

Lines 279 to 286 in 3badfe0

    
           default boolean tableExists(TableIdentifier identifier) { 
        
             try { 
        
               loadTable(identifier); 
        
               return true; 
        
             } catch (NoSuchTableException e) { 
        
               return false; 
        
             } 
        
           }

I found there's opportunity for improvement on hive catalog where we can skip the instantiate of HiveTableOperations, avoid reading the iceberg metadata.json file by only rely on record within hive catalog.

This is important for REST based catalog which delegate work to hiveCatalog as API call volume can be high and this optimization can reduce API overhead and latency.

Why this is safe? HiveOperationsBase.validateTableIsIceberg is also used in catalog listTables API to differentiate the iceberg table from hive table

Skip creation of hive table operation when check existence of iceberg table in hive catalog

dramaticlly · 2024-11-20T00:22:57Z

FYI @szehon-ho and @haizhou-zhao if you are interested

haizhou-zhao

Thx @dramaticlly , nice work

pvary · 2024-11-20T06:36:26Z

Quick question: Is this a behavioral change? Previously we failed when the metadata was corrupt. After this, we succeed.

How do we handle corrupt metadata in other catalog implementations?

dramaticlly · 2024-11-20T19:58:28Z

Quick question: Is this a behavioral change? Previously we failed when the metadata was corrupt. After this, we succeed.

How do we handle corrupt metadata in other catalog implementations?

Thank you @pvary I think this indeed introduce a behavioral change. Majority of existing catalogs (except ECSCatalog) rely on this default implementation in Catalog interface where we tried to load the table first and return true if load is successful. I believe table exists here imply 2 things where both table entry exist in catalog as well as latest table metadata.json is not corrupted.

Personally I think we can focus on former only and here's my thought process

There are roughly 3 places where catalog.tableExists was used in iceberg code base

Check before table can be registered in registerTable API

iceberg/core/src/main/java/org/apache/iceberg/BaseMetastoreCatalog.java

Line 82 in 3badfe0

if (tableExists(identifier)) {

I believe behaviour change is allowed here as long as entry exist in catalog, register shall fail regardless files is corrupted

Check before table stage creation, this is only used in REST catalog handler

iceberg/core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java

Line 228 in 3badfe0

if (catalog.tableExists(ident)) {

I believe behaviour change is also allowed here as long as entry exist in catalog, stage creation shall fail regardless version files is corrupted

REST API to check for table existence:

iceberg/open-api/rest-catalog-open-api.yaml

Lines 1129 to 1133 in 3badfe0

    
           head: 
        
             tags: 
        
               - Catalog API 
        
             summary: Check if a table exists 
        
             operationId: tableExists

I think this is what I originally hoped for to optimize on, to speed up on the existence check without rely on reading metadata first. The reason is that sometimes existence check is all we need without subsequent load table call

pvary · 2024-11-21T13:19:08Z

Thanks for the check @dramaticlly!
I agree that this behavioural change is small, but I would like to raise awareness around the community about this. Could you please write a letter to the dev list describing what is planned here? So if there is someone who is against the change, they could raise their voices.

Thanks,
Peter

pvary · 2024-11-21T13:24:33Z

Do we want to do the same opimization for the viewExists method too?

karuppayya · 2024-11-21T23:07:09Z

Another minor behavioral change is , earlier if the user had access to both HMS table and storage, the table exists would pass.
With the change, tableExists would pass with only access to HMS table?

pvary · 2024-12-03T09:03:08Z


  /**
-   * Check whether table exists.
+   * Check whether table exists, including metadata table.


nit:

Suggested change

* Check whether table exists, including metadata table.

* Check whether table or metadata table exists.

gaborkaszab

Thanks for the changes! I just went through for educating myself and left some nits. Let me know if they doesn't make sense!

kevinjqliu · 2024-12-04T03:44:16Z


  /**
-   * Check whether table exists.
+   * Check whether table or metadata table exists.


Is checking whether the metadata table exists a new behavior? Since we're changing the comment for the Catalog interface here, it might affect catalogs that have not specifically overridden this function.

I believe it's the existing (undocumented) behaviour

Thanks! I didn't know about this behavior.

Changing this doc string in the Catalog interface will also change the doc string for functions that overrides tableExists. For example, the EcsCatalog overrides the tableExists and inherits the doc string in its javadocs https://iceberg.apache.org/javadoc/1.7.0/org/apache/iceberg/dell/ecs/EcsCatalog.html
but it doesn't look like this behavior is supported in the EcsCatalog.

There are a couple more places where this happens https://grep.app/search?q=boolean%20tableExists%28&filter[repo.pattern][0]=apache/iceberg

Perhaps it is better to move this into HiveCatalog or any other catalogs that support this behavior to avoid confusion in the future.

Yea it did cross my mind to be a bit hesitant to change the top level javadoc, I am not sure if metadata tables is a catalog wide concept. Kevin's suggestion makes sense.

I think it's otherwise, all other catalog reuse the default implementation where they tried to load table first as a way to determine whether table exists. The ECSCatalog is the only concrete implementation override this method which might not handle the metadata tables. Partly it was because such behaviour was implicit without unit test or documentation, until @szehon-ho mentioned it in #11597 (comment)

My vote is that we can override the javadoc in ECSCatalog instead, what do you think?

I would agree that we don't want to expose the metadata Table concept here.

thank you @danielcweeks , revert as suggested. My main concern is that if we dont document this explicitly, the future override of this method in catalog might not even take metadata table into consideration.

the metadata table concept only applied to loadTable right now
https://github.com/search?q=repo%3Aapache%2Ficeberg%20isValidMetadataIdentifier&type=code

so its a side effect of tableExists using the underlying loadTable function

on a meta level, its a bit weird to check if a metadata table exists. if the underlying iceberg table exists, I would assume its metadata table also exists.

that said, I'm in favor of documenting this hidden behavior of tableExists

Yea I think the main concern with putting on top level javadoc is that , there's a lot of Catalog in the wild that may not (and may not want to) support this. As @kevinjqliu points out it seems its a bit of an outlier that it happened here (that we should keep for backward compatbiility) and doc can probably be added in HiveCatalog specifically for this PR.

kevinjqliu

LGTM! Thanks for the optimization!

there seem to be implementations of HiveCatalog in other projects, maybe we can raise this optimization there as well
https://grep.app/search?q=HiveCatalog&filter[path.pattern][0]=HiveCatalog.java

gaborkaszab

Thanks for the changes! Just some really small nits, otherwise I'm fine with the patch.

pvary · 2024-12-09T14:05:23Z

@danielcweeks: If you don't have more comments here, then I think we could merge this PR

dramaticlly · 2024-12-10T22:19:24Z

@rdblue @danielcweeks can you help take anther look to see if we can move forward on this PR?

danielcweeks · 2024-12-10T23:36:32Z

@dramaticlly minor comment, but other than that LGTM.

danielcweeks · 2024-12-12T18:01:54Z

Thanks @dramaticlly !!

dramaticlly · 2024-12-12T18:11:39Z

Thank you @danielcweeks @szehon-ho @pvary @kevinjqliu @gaborkaszab @haizhou-zhao for the review! I will look into similar change for hive view existence check

Hive: Optimize tableExists API in hive catalog

600f292

Skip creation of hive table operation when check existence of iceberg table in hive catalog

github-actions Bot added the hive label Nov 20, 2024

haizhou-zhao approved these changes Nov 20, 2024

View reviewed changes

pvary reviewed Nov 20, 2024

View reviewed changes

Comment thread hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java

pvary reviewed Nov 20, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

Add a newline after if/else

77d984c

szehon-ho reviewed Nov 22, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Outdated

Add current thread interrupt

47dc279

szehon-ho reviewed Nov 22, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

kevinjqliu reviewed Nov 22, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Outdated

szehon-ho reviewed Nov 22, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Outdated

Handle metadata tables and separate the tests

3b4e75f

github-actions Bot added API core labels Dec 2, 2024

Add comment back

6022ed0

pvary reviewed Dec 3, 2024

View reviewed changes

pvary approved these changes Dec 3, 2024

View reviewed changes

gaborkaszab reviewed Dec 3, 2024

View reviewed changes

Address feedback

8b32e03

szehon-ho reviewed Dec 3, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

szehon-ho approved these changes Dec 3, 2024

View reviewed changes

kevinjqliu reviewed Dec 4, 2024

View reviewed changes

Add extra comment for EcsCatalog override method

566bd92

github-actions Bot added the DELL label Dec 4, 2024

danielcweeks reviewed Dec 4, 2024

View reviewed changes

Comment thread dell/src/main/java/org/apache/iceberg/dell/ecs/EcsCatalog.java Outdated

Move javadoc around

033f398

github-actions Bot removed API DELL labels Dec 4, 2024

kevinjqliu approved these changes Dec 4, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

gaborkaszab reviewed Dec 6, 2024

View reviewed changes

Comment thread hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java Outdated

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

dramaticlly added 2 commits December 6, 2024 12:00

Added note if hive table with same name exists

4558f82

Added note if hive table with same name exists

efe0956

gaborkaszab approved these changes Dec 7, 2024

View reviewed changes

danielcweeks reviewed Dec 10, 2024

View reviewed changes

Comment thread hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java

Add test with invalid identifier

5521bff

danielcweeks approved these changes Dec 12, 2024

View reviewed changes

danielcweeks merged commit a3dcfd1 into apache:main Dec 12, 2024

dramaticlly deleted the hiveTableExists branch December 12, 2024 18:11

dramaticlly mentioned this pull request Dec 18, 2024

Hive: Optimize viewExists API in hive catalog #11813

Merged

	default boolean tableExists(TableIdentifier identifier) {
	try {
	loadTable(identifier);
	return true;
	} catch (NoSuchTableException e) {
	return false;
	}
	}

	* Check whether table exists, including metadata table.
	* Check whether table or metadata table exists.

Uh oh!

Conversation

dramaticlly commented Nov 20, 2024

Uh oh!

dramaticlly commented Nov 20, 2024

Uh oh!

haizhou-zhao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pvary commented Nov 20, 2024

Uh oh!

dramaticlly commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pvary commented Nov 21, 2024

Uh oh!

pvary commented Nov 21, 2024

Uh oh!

karuppayya commented Nov 21, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaborkaszab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kevinjqliu Dec 4, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinjqliu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gaborkaszab left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pvary commented Dec 9, 2024

Uh oh!

dramaticlly commented Dec 10, 2024

Uh oh!

Uh oh!

danielcweeks commented Dec 10, 2024

Uh oh!

danielcweeks commented Dec 12, 2024

Uh oh!

dramaticlly commented Dec 12, 2024

Uh oh!

Reviewers

dramaticlly commented Nov 20, 2024 •

edited

Loading

kevinjqliu Dec 4, 2024 •

edited

Loading