fix(iceberg): fix table creation without namespace location #26744
Conversation
fix table creation on non-s3tables REST catalogs that don't specify namespace location
| @@ -1301,18 +1281,21 @@ public ConnectorOutputTableHandle beginCreateTable(ConnectorSession session, Con | |||
| .orElseGet(() -> catalog.defaultTableLocation(session, tableMetadata.getTable())); | |||
| } | |||
| transaction = newCreateTableTransaction(catalog, tableMetadata, session, replace, tableLocation, allowedExtraProperties); | |||
|
|
|||
|
Sure, could you point me in the correct direction @ebyhr? I likely simply didn't see it, where is this behavior currently being tested? I checked |
| if (!isS3Tables(location.toString())) { | ||
| // we create a non-staged table if tableLocation.isEmpty(), in that case, the table location will not be | ||
| // empty | ||
| if (!isS3Tables(location.toString()) && !(tableLocation != null && tableLocation.isEmpty())) { |
There was a problem hiding this comment.
what about non-zero length whitespaced string : " " or " " ?
should we also add tableLocation.isBlank() e.g.
if (!isS3Tables(location.toString()) && !(tableLocation != null && tableLocation.isEmpty() && tableLocation.isBlank())) {
There was a problem hiding this comment.
Can add those, wondering if location should reject such invalid location strings?
There was a problem hiding this comment.
TBH, this all looks a bit hacky, ideally, we'd know if a table's creation was staged or not and only verify the location's emptiness based on that
Description
This should fix #26742 where it's impossible to create a table on non-s3tables REST catalogs that don't return a namespace location.
Additional context and related issues
There are two separate conditions being used to determine if a table should be non-staged and if its location has to be empty. If the table is being created as non-staged, then the location won't be empty.
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: