tanmayrauth commented on code in PR #2001:
URL: https://github.com/apache/iceberg-go/pull/2001#discussion_r3986039132
##########
catalog/glue/glue.go:
##########
@@ -307,6 +337,113 @@ func (c *Catalog) CreateTable(ctx context.Context,
identifier table.Identifier,
return c.LoadTable(ctx, identifier)
}
+// isS3TablesDatabase reports whether the Glue database is federated to the
+// Amazon S3 Tables service, which owns table storage and location assignment.
+func (c *Catalog) isS3TablesDatabase(ctx context.Context, database string)
(bool, error) {
+ db, err := c.getDatabase(ctx, database)
+ if err != nil {
+ // A missing database is not fatal here; let the generic create
path
+ // surface it, so this probe never changes the error a caller
already saw.
+ if errors.Is(err, catalog.ErrNoSuchNamespace) {
+ return false, nil
+ }
+
+ return false, err
Review Comment:
Any non-NotFound getDatabase error here aborts the whole CreateTable. For
default (no-location) creates this doesn't change behavior — that path already
called getDatabase. But an explicit-location create (WithLocation) previously
never touched getDatabase, so after this PR such a create newly fails if the
caller's IAM grants glue:CreateTable but not glue:GetDatabase (getDatabase
returns AccessDenied, errors.As doesn't match EntityNotFoundException, this
line returns the error), or on a transient/throttled GetDatabase.
Since the probe only selects a create path and the generic path
re-surfaces genuine errors on its own, make it non-fatal: return (false, nil)
on any getDatabase failure and fall through, or at minimum swallow
AccessDeniedException alongside ErrNoSuchNamespace. Note the "get database
error → wantErr" case in TestGlueIsS3TablesDatabase locks in the current
behavior and would flip.
##########
catalog/glue/glue.go:
##########
@@ -277,6 +282,36 @@ var _ catalog.Closer = (*Catalog)(nil)
// This function will create the metadata file in S3 using the catalog and
table properties,
// to determine the bucket and key for the metadata location.
func (c *Catalog) CreateTable(ctx context.Context, identifier
table.Identifier, schema *iceberg.Schema, opts ...catalog.CreateTableOpt)
(*table.Table, error) {
+ // Keep the missing-namespace contract that CreateStagedTable enforced
when it
+ // ran first, before the federation probe below needed the database
name early.
+ if len(identifier) < 2 {
+ return nil, fmt.Errorf("%w: missing namespace or invalid
identifier %v", catalog.ErrNoSuchNamespace, identifier)
+ }
+
+ database, tableName, err := identifierToGlueTable(identifier)
+ if err != nil {
+ return nil, err
+ }
+
+ var cfg catalog.CreateTableCfg
+ for _, opt := range opts {
+ opt(&cfg)
+ }
+
+ federated, err := c.isS3TablesDatabase(ctx, database)
Review Comment:
This probe fires on every CreateTable. For a default (no explicit location)
create it's a redundant Glue round-trip, since the generic path already calls
getDatabase during location resolution (CreateStagedTable →
ResolveTableLocationWithNamespace → LoadNamespaceProperties → getDatabase). For
an explicit-location create the generic path never called getDatabase at all,
so this adds a brand-new round-trip.
Consider lazy detection: run the generic create, and only when it fails
with the "no default location" error, probe for federation and retry via the S3
Tables path. That removes the redundant/extra call in both cases.
##########
catalog/glue/glue.go:
##########
@@ -850,7 +987,11 @@ func (c *Catalog) getRawTable(ctx context.Context,
database, tableName string) (
return nil, fmt.Errorf("failed to get table %s.%s: missing Glue
table response", database, tableName)
}
- if aws.ToString(tblRes.Table.TableType) != glueTableType {
+ // S3 Tables federated entries carry their own TableType (e.g.
"customer") but
+ // are still Iceberg tables, marked by the table_type parameter.
+ tableType := tblRes.Table.Parameters[tableParamTableType]
+ isIceberg := strings.EqualFold(tableType, glueTypeIceberg) || tableType
== glueTypeIcebergRenaming
+ if aws.ToString(tblRes.Table.TableType) != glueTableType && !isIceberg {
Review Comment:
This relaxation applies to every database, not just S3 Tables federated
ones: any Glue entry whose table_type parameter is ICEBERG/ICEBERG_RENAMING now
passes getRawTable regardless of its Glue TableType. It's contained today
because getTable and convertGlueToIceberg re-check table_type and require
metadata_location, so no live bug — but it widens the accepted-input boundary
repo-wide for a federation-specific need. To keep it scoped, gate the relaxed
branch on the federated case, or add a comment that broadening it for all
databases is intentional.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
