feat: trino support replace table iceberg and delta#5279
Conversation
eakmanrq
force-pushed
the
eakmanrq/trino_replace_table_support
branch
from
3ee5976 to
4744324
Compare
| ) | ||
| assert adapter.current_catalog_type == storage_type | ||
| expected_current_type = storage_type | ||
| assert adapter.current_catalog_type == expected_current_type |
There was a problem hiding this comment.
Does this change do anything?
| ) | ||
| assert adapter.get_catalog_type(f"datalake_{storage_type}") == storage_type | ||
| expected_type = storage_type | ||
| assert adapter.get_catalog_type(f"datalake_{storage_type}") == expected_type |
| supports_replace_table_override: t.Optional[bool] = None, | ||
| **kwargs: t.Any, | ||
| ) -> None: | ||
| catalog_type = self.get_catalog_type(self.get_catalog_type_from_table(table_name)) |
There was a problem hiding this comment.
Doesn't self.get_catalog_type_from_table(table_name) already call self.get_catalog_type()?
So this would do:
self.get_catalog_type_from_table(table_name) -> may return "delta_lake"
Then it would call:
self.get_catalog_type("delta_lake")
There is probably no catalog called delta_lake as it's a trino connector type (which is what we call "catalog type" here), so this would fall back on the connector type of the default catalog and not that of the table.
I think this "worked" in the test you wrote because for unit testing purposes we infer the catalog type from the catalog name, but this doesn't happen on a running cluster.
The catalog can be named anything you want and the catalog type comes from whatever was configured for the connector.name property for that catalog in the Trino config
4 participants
Prior to this PR we assumed all Trino catalogs didn't support replace table but this isn't true for Iceberg and Delta Lake. Therefore this PR updates Trino to use replace table for those catalogs.
Some other key changes:
current_catalog_typebut this just looks at the current default catalog for the connection. We should be looking at the target table of the operation and it's catalog if it exists and then fallback to the connection catalog if it does not.current_catalog_typestill exists just to support truncating comment methods. They should also be updated to use the target table/catalog but that change was proving to be a larger refactor and determined to be out of scope of this PR.deltawhen it should bedelta_lake. I base this on documentation and our own integration tests