AWS: do not list non-iceberg table in Glue#2267
Conversation
| Table.builder().databaseName("db1").name("t2").build() | ||
| Table.builder().databaseName("db1").name("t1").parameters( | ||
| ImmutableMap.of( | ||
| BaseMetastoreTableOperations.TABLE_TYPE_PROP, BaseMetastoreTableOperations.ICEBERG_TABLE_TYPE_VALUE |
There was a problem hiding this comment.
I think we should also test the case where the namespace is non-empty, but there is no Iceberg table.
There was a problem hiding this comment.
Great catch, dropping namespace with non-empty non-Iceberg table is interesting, in theory Iceberg should not care about it, but in practice we should probably not delete the namespace. So I updated the logic to check for iceberg or non-iceberg tables, and throw exception with different message for different cases. I have updated both the unit test and also the integration test.
|
Looks like a correct implementation. I think this needs a test for the non-empty drop behavior so that we know what happens there, but otherwise I think this is good. Thanks, @jackye1995. |
yyanyy
left a comment
There was a problem hiding this comment.
LGTM, wondering if we want to mention in AWS documentation page that iceberg table has to contain this table type property to be considered as iceberg table, in case there are other implementations to this.
| .build()); | ||
|
|
||
| if (response.hasTableList() && !response.tableList().isEmpty()) { | ||
| Table table = response.tableList().get(0); |
There was a problem hiding this comment.
Probably not really need to differentiate the two, but I don't think it's worth blocking the PR.
|
Looks good, I'll merge. Thanks @jackye1995! |
People might store non-Iceberg table in the same Glue database, and
listTablesshould list non-Iceberg tables.