Hive: Optimize tableExists API in hive catalog #11597

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

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

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

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?

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

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

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

@@ -271,7 +271,7 @@ default Transaction newReplaceTableTransaction(
  }

  /**
-   * 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!

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

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

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

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

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

@@ -271,7 +271,7 @@ default Transaction newReplaceTableTransaction(
  }

  /**
-   * 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.

dell/src/main/java/org/apache/iceberg/dell/ecs/EcsCatalog.java

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

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

gaborkaszab

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

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

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

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?

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

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

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java Show resolved Hide resolved

pvary reviewed Nov 20, 2024

View reviewed changes

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Show resolved Hide resolved

Add a newline after if/else

77d984c

szehon-ho reviewed Nov 22, 2024

View reviewed changes

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Show resolved Hide resolved

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Outdated Show resolved Hide resolved

Add current thread interrupt

47dc279

szehon-ho reviewed Nov 22, 2024

View reviewed changes

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Show resolved Hide resolved

kevinjqliu reviewed Nov 22, 2024

View reviewed changes

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Outdated Show resolved Hide resolved

szehon-ho reviewed Nov 22, 2024

View reviewed changes

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Outdated Show resolved Hide resolved

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

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Show resolved Hide resolved

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

dell/src/main/java/org/apache/iceberg/dell/ecs/EcsCatalog.java Outdated Show resolved Hide resolved

Move javadoc around

033f398

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

kevinjqliu approved these changes Dec 4, 2024

View reviewed changes

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Show resolved Hide resolved

gaborkaszab reviewed Dec 6, 2024

View reviewed changes

hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java Outdated Show resolved Hide resolved

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Show resolved Hide resolved

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

hive-metastore/src/main/java/org/apache/iceberg/hive/HiveCatalog.java Show resolved Hide resolved

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
49 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hive: Optimize tableExists API in hive catalog #11597

Hive: Optimize tableExists API in hive catalog #11597

dramaticlly commented Nov 20, 2024

dramaticlly commented Nov 20, 2024

haizhou-zhao left a comment

pvary commented Nov 20, 2024

dramaticlly commented Nov 20, 2024 •

edited

Loading

pvary commented Nov 21, 2024

pvary commented Nov 21, 2024

karuppayya commented Nov 21, 2024

pvary Dec 3, 2024

gaborkaszab left a comment

kevinjqliu Dec 4, 2024

dramaticlly Dec 4, 2024

kevinjqliu Dec 4, 2024 •

edited

Loading

szehon-ho Dec 4, 2024

dramaticlly Dec 4, 2024

danielcweeks Dec 4, 2024

dramaticlly Dec 4, 2024

kevinjqliu Dec 4, 2024

szehon-ho Dec 4, 2024

kevinjqliu left a comment

gaborkaszab left a comment

pvary commented Dec 9, 2024

dramaticlly commented Dec 10, 2024

danielcweeks commented Dec 10, 2024

danielcweeks commented Dec 12, 2024

dramaticlly commented Dec 12, 2024

	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.

Hive: Optimize tableExists API in hive catalog #11597

Hive: Optimize tableExists API in hive catalog #11597

Conversation

dramaticlly commented Nov 20, 2024

dramaticlly commented Nov 20, 2024

haizhou-zhao left a comment

Choose a reason for hiding this comment

pvary commented Nov 20, 2024

dramaticlly commented Nov 20, 2024 • edited Loading

pvary commented Nov 21, 2024

pvary commented Nov 21, 2024

karuppayya commented Nov 21, 2024

Choose a reason for hiding this comment

gaborkaszab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinjqliu Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinjqliu left a comment

Choose a reason for hiding this comment

gaborkaszab left a comment

Choose a reason for hiding this comment

pvary commented Dec 9, 2024

dramaticlly commented Dec 10, 2024

danielcweeks commented Dec 10, 2024

danielcweeks commented Dec 12, 2024

dramaticlly commented Dec 12, 2024

dramaticlly commented Nov 20, 2024 •

edited

Loading

kevinjqliu Dec 4, 2024 •

edited

Loading