Add ConnectorMetadata#isMaterializedView

[ksobolew]

Description

The current implementation of MetadataManager#isMaterializedView calls ConnectorMetadata#getMaterializedView under the covers. This means that we have to load and deserialize the materialized view metadata just to see if it exists, which is unnecessary most of the time. This may also cause issues when the metadata is currupted in some way (see the last commit for relevant test cases, where we add support for this method to the Iceberg plugin). In such a case, we want to still be able to DROP the corrupted materialized view - which is currently impossible, because the DropMaterializedViewTask will call getMaterializedView, which will fail when it tries to load the corrupted metadata. With this change this is less likely.

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# Section
* Fix failure in `DROP MATERIALIZED VIEW` in Iceberg when the materialized view metadata is missing or corrupted.

[ksobolew]

CI #16437

[findepi]

And that would prevent it from being DROPped, because the DropMaterializedViewTask first wants to check if the entity to be dropped is a materialized view. If it called getMaterializedView, as it used to, dropping such a view would fail too; but with isMaterializedView that is not a problem.

please add a test for that

This may not be just an optimization,

is this also an optimization? if so, there should be a test change (eg counting file system or metastore access) that shows the improvement.

[findepi]

(x) This is not user-visible or is docs only, and no release notes are required.

The description contradicts that.

[ksobolew]

is this also an optimization? if so, there should be a test change (eg counting file system or metastore access) that shows the improvement.

There are already changes to the TestTrinoDatabaseMetaData that show changes in the call patterns. It's not exactly fewer calls, but fewer to getMaterializedView.

[findepi]

TestTrinoDatabaseMetaData is quite tied to JDBC driver
can we have a test for Iceberg connector in the Iceberg module as well?
I think it would make sense, given that Iceberg is the only connector that has MV support

see eg

trino/plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/TestIcebergMetastoreAccessOperations.java

Lines 328 to 372 in 8f25ce5

	@Test(dataProvider = "metadataQueriesTestTableCountDataProvider")
	public void testInformationSchemaColumns(int tables)
	{
	String schemaName = "test_i_s_columns_schema" + randomNameSuffix();
	assertUpdate("CREATE SCHEMA " + schemaName);
	Session session = Session.builder(getSession())
	.setSchema(schemaName)
	.build();
	for (int i = 0; i < tables; i++) {
	assertUpdate(session, "CREATE TABLE test_select_i_s_columns" + i + "(id varchar, age integer)");
	// Produce multiple snapshots and metadata files
	assertUpdate(session, "INSERT INTO test_select_i_s_columns" + i + " VALUES ('abc', 11)", 1);
	assertUpdate(session, "INSERT INTO test_select_i_s_columns" + i + " VALUES ('xyz', 12)", 1);
	assertUpdate(session, "CREATE TABLE test_other_select_i_s_columns" + i + "(id varchar, age integer)"); // won't match the filter
	}
	// Bulk retrieval
	assertMetastoreInvocations(session, "SELECT * FROM information_schema.columns WHERE table_schema = CURRENT_SCHEMA AND table_name LIKE 'test_select_i_s_columns%'",
	ImmutableMultiset.builder()
	.add(GET_ALL_TABLES_FROM_DATABASE)
	.addCopies(GET_TABLE, tables * 2)
	.addCopies(GET_TABLES_WITH_PARAMETER, 2)
	.build());
	// Pointed lookup
	assertMetastoreInvocations(session, "SELECT * FROM information_schema.columns WHERE table_schema = CURRENT_SCHEMA AND table_name = 'test_select_i_s_columns0'",
	ImmutableMultiset.builder()
	.add(GET_TABLE)
	.build());
	// Pointed lookup via DESCRIBE (which does some additional things before delegating to information_schema.columns)
	assertMetastoreInvocations(session, "DESCRIBE test_select_i_s_columns0",
	ImmutableMultiset.builder()
	.add(GET_DATABASE)
	.add(GET_TABLE)
	.build());
	for (int i = 0; i < tables; i++) {
	assertUpdate(session, "DROP TABLE test_select_i_s_columns" + i);
	assertUpdate(session, "DROP TABLE test_other_select_i_s_columns" + i);
	}
	}

[ksobolew]

Sure, I'm working on tests right now (though demonstrating how a corrupted metadata can prevent DROP MV from executing is kinda challenging)

[ksobolew]

@findepi Addressed comments, PTAL.

[ksobolew]

TestTrinoDatabaseMetaData is quite tied to JDBC driver
can we have a test for Iceberg connector in the Iceberg module as well?
I think it would make sense, given that Iceberg is the only connector that has MV support

Added such a test, but it's not very meaningful, IMO. It just says that we call getTable on the metastore one time less when we DROP MATERIALIZED VIEW.

[ksobolew]

(x) This is not user-visible or is docs only, and no release notes are required.

The description contradicts that.

Added a release note.

[ksobolew]

And that would prevent it from being DROPped, because the DropMaterializedViewTask first wants to check if the entity to be dropped is a materialized view. If it called getMaterializedView, as it used to, dropping such a view would fail too; but with isMaterializedView that is not a problem.

please add a test for that

Finally found a way to do that; test added.

This may not be just an optimization,

is this also an optimization? if so, there should be a test change (eg counting file system or metastore access) that shows the improvement.

I rewrote the commit message (and the PR description) to phrase it as primarily a correctness issue, with some efficiency improvements added as a bonus.

[ksobolew]

See #19177 for an example of a failure that this PR can mitigate.

[findepi]

/test-with-secrets sha=f5d848b3678109c4fe1e67b18b798d5c0e1ee04e

[findepi]

i plan to review this (later)

[github-actions]

The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/6352010690

[findepi]

This means that we have to load and deserialize the materialized view metadata just to see if it exists, which is unnecessary most of the time

This is true, but actually it is probably much lesser problem than it sounds it is

. This may also cause issues when the metadata is currupted in some way (see the last commit for relevant test cases, where we add support for this method to the Iceberg plugin). In such a case, we want to still be able to DROP the corrupted materialized view

The ConnectorMaterializedViewDefinition is the "materialized view definition", it shouldn't need to load the storage table for anything.
I think the fact that in Iceberg case it loads properties from the storage table is sub-optimal design and we should revisit it.

• Alex's Remove Iceberg materialized view storage tables from metastores #18853 will squash MV and storage table within metastore
• plus, we should preserve user-visible MV properties in the metastore level object as well
  • we can do so safely, since storage table is "owned" by the MV so it must not change behind the scenes in any way

This is important not only for DROP corrupted_mv case.
It is also important for all metadata queries. Querying system.metatda.materialized_views should ideally not need to go to S3 to obtain "materialized view definitions", especially not O(number of MVs) times.

[ksobolew]

This is true, but actually it is probably much lesser problem than it sounds it is

It is a problem, at least, for the use case I'm trying to fix.

This is important not only for DROP corrupted_mv case.
It is also important for all metadata queries. Querying system.metatda.materialized_views should ideally not need to go to S3 to obtain "materialized view definitions", especially not O(number of MVs) times.

That's true as well.

I appreciate the feedback and I agree that we don't want to add more code which may be considered a workaround for limitations of the current architecture, but I also don't want this to become a case of "sure, but first please spend 6 months refactoring this complex piece of code". What would be the commended course of action then?

[github-actions]

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

[mosabua]

Reminder to continue work on this @ksobolew @findepi or close it.

[ksobolew]

This work is currently on hold and it's not clear this is the way forward. I'll reopen if it crystalizes.