Remove redundant checks from DropSchemaTask

[findepi]

listTables includes views and materialized views, so calling
listViews and listMaterializedViews is redundant.

[losipiuk]

I did not check. Is this accurate for all the connector now?

[findinpath]

I went through all the implementations of io.trino.spi.connector.ConnectorMetadata#listViews and io.trino.spi.connector.ConnectorMetadata#listMaterializedViews and the corresponding listTables implementation does indeed return the views / materialized views as well.

[findepi]

I did not check. Is this accurate for all the connector now?

even if it wasn't the case yet, we already have this

trino/core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java

Lines 210 to 213 in 8b0c754

	/**
	* List table, view and materialized view names, possibly filtered by schema. An empty list is returned if none match.
	*/
	default List<SchemaTableName> listTables(ConnectorSession session, Optional<String> schemaName)

and Metadata just calls that method.

[hashhar]

Good direction but I don't think all connectors check for existence of materialized views.
All of them do check for views though.

We should also check if there are any connectors whose drop schema impl doens't check for existence of objects in the schema. (i.e. remote views or materialized views or other objects) (I'll check and re-review).

[wendigo]

Personally I don't have an opinion here as I don't track whether all connectors consistently implement SPI. Isn't that change risky?

[findinpath]

It would be beneficial to add views & materialized views in the io.trino.testing.BaseConnectorTest#testDropNonEmptySchema depending on:

• SUPPORTS_CREATE_VIEW
• SUPPORTS_CREATE_MATERIALIZED_VIEW

[findepi]

Good idea, but we would have to test all these 3 cases separately. I don't plan to do this though. Do you want to want to follow up?

[findinpath]

Yes, I'll create a PR based on your changes.

See #11614

[findepi]

Yes, I'll create a PR based on your changes.

many thanks!

[findepi]

Good direction but I don't think all connectors check for existence of materialized views.

@hashhar I think exactly one connector supports MVs today, am i right?

[hashhar]

@findepi Yes, but how do you ensure this contract is maintained going forward without either tests or by default calling listMaterializedViews (whose default impl returns empty list).

(Because I'm not confident we'll catch this at review time).

[findepi]

(Because I'm not confident we'll catch this at review time).

Failure to list MVs within listTables has other consequences, like SHOW TABLES, information_schema.tables, .columns, system.jdbc.tables, .columns. See eg #11063
So we better make sure that a connector gaining MV support does it correctly, or we have bugs.