fix: Trino get_columns

[betodealmeida]

SUMMARY

This PR is a workaround to a weird bug we've encountered in Trino when adding a physical dataset.

When adding the dataset, Superset calls get_columns on the inspector. The Trino driver then does a check to see if the table exists, using has_table:

https://github.com/trinodb/trino-python-client/blob/505989a59e93f6319e8b6be89a16f81034a06404/trino/sqlalchemy/dialect.py#L176

The has_table method runs this query:

            SELECT "table_name"
            FROM "information_schema"."tables"
            WHERE "table_schema" = :schema
              AND "table_name" = :table

And here's the weird part: even though the table DOES exist, the query can return nothing. Even weirder: running a SHOW COLUMNS FROM :schema.:table in that case returns data.

The Trino DB engine spec already has a custom get_indexes method with the warning:

Trino dialect raises NoSuchTableError in get_indexes if table is empty.

superset/superset/db_engine_specs/trino.py

Lines 442 to 462 in 84a1cd2

	@classmethod
	def get_indexes(
	cls,
	database: Database,
	inspector: Inspector,
	table: Table,
	) -> list[dict[str, Any]]:
	"""
	Get the indexes associated with the specified schema/table.
	Trino dialect raises NoSuchTableError in get_indexes if table is empty.
	:param database: The database to inspect
	:param inspector: The SQLAlchemy inspector
	:param table: The table instance to inspect
	:returns: The indexes
	"""
	try:
	return super().get_indexes(database, inspector, table)
	except NoSuchTableError:
	return []

So I suspect it's the same problem.

To fix this, I modified the Trino DB engine spec to first try the inspector.get_columns method, which queries the information_schema.table. If this fails with NoSuchTableError then it falls back to the SHOW COLUMNS FROM ... approach.

The Presto DB engine spec already implements a get_columns that uses SHOW COLUMNS FROM ..., so I moved it to PrestoBaseEngineSpec, so it can be used both by PrestoEngineSpec and TrinoEngineSpec.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[betodealmeida]

These methods were moved to the base class, PrestoBaseEngineSpec.

[michael-s-molina]

Nice!

[michael-s-molina]

I appreciate the comments 👍🏼