Core: Fix caching table with metadata table names

[manuzhang]

When CachingCatalog finds a table having a MetadataTableType as its identifier name, it will load its namespace as a normal table. However, spark_catalog.default.history is a valid table identifier, although having MetadataTableType as its name. In this case, CachingCatalog throws NoSuchTableException currently. This PR catches the exception and load the table as a normal table.

cc @wypoon @ajantha-bhat @nastra

[manuzhang]

@nastra @ajantha-bhat @RussellSpitzer Please take a look when you find time.

[gaborkaszab]

I found that there is an existing test suite, TestCachingCatalog. Wouldn't it be cleaner to add test coverage there instead of to the Spark test suite?

[manuzhang]

That test is to test CachingCatalog itself. This test is to test caching behavior in a real catalog.

[gaborkaszab]

I get that, but I think this patch did change the CachingCatalog internals itself, I don't see what is the additional value in putting this into a Spark test. TestCachingCatalog should definitely have coverage for behaviours like this. If you feel that an additional Spark test is also beneficial, I don't mind. I might miss some background, though.

[gaborkaszab]

nit: I know there was only an indentation change here, but there is an odd linebreak in this line

[gaborkaszab]

FWIW, as I see the RESTSessionCatalog does this the other way around: First tries to load the table as a regular table and then if that fails it tries to load it as a metadata table. https://github.com/apache/iceberg/blob/main/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java#L461
I find that approach cleaner, but let me know what you think.

[manuzhang]

Do you suggest we call catalog::loadTable first? If so, we might not be able to cache the original table, since the underlying catalog can load the metadata table directly.

[gaborkaszab]

What I had in mind was something different:
try {
return tableCache.get(canonicalized, catalog::loadTable);
} catch (NoSuchTableException) {
// Verify it's a metadata table
// Do whatever we want in case of a metadata table, even cache the original table.
}
What do you think?

[manuzhang]

The issue is noNoSuchTableException will ever be thrown on cache miss since catalog::loadTable can successfully load the metadata table. For example, in BaseMetastoreCatalog

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

Lines 45 to 71 in fa00482

	public Table loadTable(TableIdentifier identifier) {
	Table result;
	if (isValidIdentifier(identifier)) {
	TableOperations ops = newTableOps(identifier);
	if (ops.current() == null) {
	// the identifier may be valid for both tables and metadata tables
	if (isValidMetadataIdentifier(identifier)) {
	result = loadMetadataTable(identifier);
	} else {
	throw new NoSuchTableException("Table does not exist: %s", identifier);
	}
	} else {
	result = new BaseTable(ops, fullTableName(name(), identifier), metricsReporter());
	}
	} else if (isValidMetadataIdentifier(identifier)) {
	result = loadMetadataTable(identifier);
	} else {
	throw new NoSuchTableException("Invalid table identifier: %s", identifier);
	}
	LOG.info("Table loaded by catalog: {}", result);
	return result;
	}

In this case, we don't cache the original table and change the current behavior.

[gaborkaszab]

Finally I had some time to take a deeper look on this. You are right, with my above proposal we won't run the specific code for metadata tables like caching the underlying table and sharing the table operations.

I did find a way simpler version of this fix, however. I also wrote some tests in TestCachingCatalog. gaborkaszab@c1eca8b
What do you think? Would you mind if I opened an alternative fix PR?

[manuzhang]

Please go ahead.

[gaborkaszab]

Thanks @manuzhang !
I created this PR for the alternative fix: #11738 Added you as a co-author.
@RussellSpitzer I saw your thumbs-up on one of my comments. Would you mind taking a look at the PR?