fivetran-kostaszoumpatianos commented on PR #1772: URL: https://github.com/apache/polaris/pull/1772#issuecomment-2931346272
thanks @adutra ! As far as I understand, `tableExists()` should only check if an actual table exists (based on how it's used). I don't think that it was intended behaviour to check for metadata tables. For example, I can see the following uses (and more but similar in nature): - FlinkCatalog::createTableLoader() - BaseMetaStoreCatalog::registerTable() - DynamoDbCatalog::ensureCatalogTableExistsOrCreate() - DynamoDbCatalog::ensureLockTableExistsOrCreate() - JdbcCatalog::ranameTable() - JdbcCatalog::ranameView() - JdbcCatalogOperations::createTable() - EcsCatalog::dropTable() - EcsCatalog::renameTable() Indeed, ne could fix that in iceberg, that was my first thought, but the fix would be similar in nature. Load table will internally try to resolve a metadata table that doesn't exist as a metadata table, that's ok and not really fixable since we have arbitrary namespace nesting. Instead, iceberg should provide a way of knowing if this table **actually** exists or not. I think semantically, this was the purpose of `tableExists()` and this function should only check for real tables, but since it is implemented at the base class, the only way the had to implement that in a generic way was by wrapping `loadTable()` in a `try/catch`, which I think is ok as generic, catalog-agnostic logic, but it breaks in the REST catalog case. So I think the Polaris catalog should provide an implementation that "unbreaks" it. What do you think? Do you think that semantically it should also check for metadata table names, and do you know of any such usages? Thanks again for looking at my PR! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: issues-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
