laskoviymishka commented on code in PR #1411:
URL: https://github.com/apache/iceberg-go/pull/1411#discussion_r3647981230
##########
catalog/sql/sql.go:
##########
@@ -224,12 +224,17 @@ func init() {
var _ catalog.Catalog = (*Catalog)(nil)
var (
- minimalNamespaceProps = iceberg.Properties{"exists": "true"}
+ namespaceExistsProperty = "exists"
Review Comment:
Since `namespaceExistsProperty` is a fixed protocol value we share with Java
and PyIceberg, I'd make it a `const` rather than a `var` — nothing should ever
reassign it. It has to move out of this block though, since
`minimalNamespaceProps` is a map and can't be const:
```go
const namespaceExistsProperty = "exists"
var (
minimalNamespaceProps = iceberg.Properties{namespaceExistsProperty:
"true"}
...
)
```
##########
catalog/sql/sql.go:
##########
@@ -1111,13 +1116,30 @@ func (c *Catalog) LoadNamespaceProperties(ctx
context.Context, namespace table.I
result := make(iceberg.Properties)
for _, p := range props {
+ if isReservedNamespaceProperty(p.PropertyKey) {
+ continue
+ }
result[p.PropertyKey] = p.PropertyValue.String
}
return result, nil
})
}
+func validateNamespacePropertyUpdates(removals []string, updates
iceberg.Properties) error {
Review Comment:
We reject the sentinel on update and remove here, but `CreateNamespace`
still writes caller props verbatim — nothing guards `exists` on the way in.
So a caller can do `CreateNamespace(ctx, ns, {"exists": "false"})` and then
they're stuck: Load filters the key out, and this validation rejects any
attempt to update or remove it. The three paths don't agree on what "reserved"
means, and that leaves the namespace in a state the public API can't get out of.
I'd guard create with the same `isReservedNamespaceProperty` predicate —
either reject reserved keys up front, or strip/overwrite them the way Java does
(it always forces `exists=true`). tanmayrauth raised this too; I think it's the
one thing worth settling before merge. wdyt?
##########
catalog/sql/sql_test.go:
##########
@@ -1882,7 +1882,71 @@ func (s *SqliteCatalogTestSuite)
TestLoadEmptyNamespaceProperties() {
s.Require().NoError(tt.cat.CreateNamespace(ctx, tt.namespace,
nil))
props, err := tt.cat.LoadNamespaceProperties(ctx, tt.namespace)
s.Require().NoError(err)
- s.Equal(iceberg.Properties{"exists": "true"}, props)
+ s.Empty(props)
+ }
+}
+
+func (s *SqliteCatalogTestSuite) TestNamespacePropertiesCannotUpdateSentinel()
{
+ tests := []struct {
+ name string
+ cat *sqlcat.Catalog
+ namespace table.Identifier
+ createProps iceberg.Properties
+ expectedProps iceberg.Properties
+ }{
+ {
+ name: "memory with user props",
+ cat: s.getCatalogMemory(),
+ namespace: table.Identifier{databaseName()},
+ createProps: iceberg.Properties{"comment":
"baseline"},
+ expectedProps: iceberg.Properties{"comment":
"baseline"},
+ },
+ {
+ name: "memory sentinel only",
+ cat: s.getCatalogMemory(),
+ namespace: table.Identifier{databaseName()},
+ createProps: nil,
+ expectedProps: nil,
+ },
+ {
+ name: "sqlite with user props",
+ cat: s.getCatalogSqlite(),
+ namespace:
strings.Split(hiearchicalNamespaceName(), "."),
+ createProps: iceberg.Properties{"comment":
"baseline"},
+ expectedProps: iceberg.Properties{"comment":
"baseline"},
+ },
+ {
+ name: "sqlite sentinel only",
+ cat: s.getCatalogSqlite(),
+ namespace:
strings.Split(hiearchicalNamespaceName(), "."),
+ createProps: nil,
+ expectedProps: nil,
+ },
+ }
+
+ ctx := context.Background()
+ for _, tt := range tests {
+ tt := tt
+ s.Run(tt.name, func() {
+ s.Require().NoError(tt.cat.CreateNamespace(ctx,
tt.namespace, tt.createProps))
+
+ _, err := tt.cat.UpdateNamespaceProperties(ctx,
tt.namespace, []string{"exists"}, nil)
+ s.ErrorIs(err, iceberg.ErrInvalidArgument)
+ s.ErrorContains(err, "cannot remove reserved namespace
property")
+ s.ErrorContains(err, `"exists"`)
+
+ _, err = tt.cat.UpdateNamespaceProperties(ctx,
tt.namespace, nil, iceberg.Properties{"exists": "false"})
+ s.ErrorIs(err, iceberg.ErrInvalidArgument)
+ s.ErrorContains(err, "cannot update reserved namespace
property")
+
+ props, err := tt.cat.LoadNamespaceProperties(ctx,
tt.namespace)
Review Comment:
Could we also assert the namespace is still intact after these rejected
updates? A quick `s.True(tt.cat.CheckNamespaceExists(ctx, tt.namespace))` here
would confirm the sentinel row actually survived — right now the test proves
the calls fail but not that existence state is preserved.
##########
catalog/sql/sql_test.go:
##########
@@ -1882,7 +1882,71 @@ func (s *SqliteCatalogTestSuite)
TestLoadEmptyNamespaceProperties() {
s.Require().NoError(tt.cat.CreateNamespace(ctx, tt.namespace,
nil))
props, err := tt.cat.LoadNamespaceProperties(ctx, tt.namespace)
s.Require().NoError(err)
- s.Equal(iceberg.Properties{"exists": "true"}, props)
+ s.Empty(props)
+ }
+}
+
+func (s *SqliteCatalogTestSuite) TestNamespacePropertiesCannotUpdateSentinel()
{
+ tests := []struct {
+ name string
+ cat *sqlcat.Catalog
+ namespace table.Identifier
+ createProps iceberg.Properties
+ expectedProps iceberg.Properties
+ }{
+ {
+ name: "memory with user props",
+ cat: s.getCatalogMemory(),
+ namespace: table.Identifier{databaseName()},
+ createProps: iceberg.Properties{"comment":
"baseline"},
+ expectedProps: iceberg.Properties{"comment":
"baseline"},
+ },
+ {
+ name: "memory sentinel only",
+ cat: s.getCatalogMemory(),
+ namespace: table.Identifier{databaseName()},
+ createProps: nil,
+ expectedProps: nil,
+ },
+ {
+ name: "sqlite with user props",
+ cat: s.getCatalogSqlite(),
+ namespace:
strings.Split(hiearchicalNamespaceName(), "."),
+ createProps: iceberg.Properties{"comment":
"baseline"},
+ expectedProps: iceberg.Properties{"comment":
"baseline"},
+ },
+ {
+ name: "sqlite sentinel only",
+ cat: s.getCatalogSqlite(),
+ namespace:
strings.Split(hiearchicalNamespaceName(), "."),
+ createProps: nil,
+ expectedProps: nil,
+ },
+ }
+
+ ctx := context.Background()
+ for _, tt := range tests {
+ tt := tt
Review Comment:
`tt := tt` is a no-op on Go 1.22+ (go.mod is on 1.25) — loop vars are
per-iteration now, so this can go.
##########
catalog/sql/sql.go:
##########
@@ -1111,13 +1116,30 @@ func (c *Catalog) LoadNamespaceProperties(ctx
context.Context, namespace table.I
result := make(iceberg.Properties)
for _, p := range props {
+ if isReservedNamespaceProperty(p.PropertyKey) {
+ continue
+ }
result[p.PropertyKey] = p.PropertyValue.String
}
return result, nil
})
}
+func validateNamespacePropertyUpdates(removals []string, updates
iceberg.Properties) error {
+ if _, ok := updates[namespaceExistsProperty]; ok {
+ return fmt.Errorf("%w: cannot update reserved namespace
property %q", iceberg.ErrInvalidArgument, namespaceExistsProperty)
+ }
+
+ for _, key := range removals {
+ if isReservedNamespaceProperty(key) {
Review Comment:
One thing worth deciding: Java's `removeProperties` doesn't guard `exists`
at all — it lets you remove it. And the removals contract here normally routes
unknown keys to the Missing set rather than erroring, so a caller who never saw
`exists` from Load now gets a hard failure for removing a key they didn't know
existed.
Blocking it is arguably safer (removing `exists` on a sentinel-only
namespace would orphan it), so I'm fine keeping the guard — but I'd either drop
`exists` into a silent no-op to match the removals contract, or keep the error
and add a comment that we're intentionally stricter than Java here. wdyt?
##########
catalog/sql/sql.go:
##########
@@ -1241,6 +1263,10 @@ func (c *Catalog) UpdateNamespaceProperties(ctx
context.Context, namespace table
return summary, err
}
+ if err := validateNamespacePropertyUpdates(removals, updates); err !=
nil {
Review Comment:
This runs after `LoadNamespaceProperties`, so a bad `exists` in removals
costs a DB round-trip before we return the arg error. Argument validation
should come before the side effects — I'd move this to the top of
`UpdateNamespaceProperties`, ahead of the load.
##########
catalog/sql/sql.go:
##########
@@ -1111,13 +1116,30 @@ func (c *Catalog) LoadNamespaceProperties(ctx
context.Context, namespace table.I
result := make(iceberg.Properties)
for _, p := range props {
+ if isReservedNamespaceProperty(p.PropertyKey) {
+ continue
+ }
result[p.PropertyKey] = p.PropertyValue.String
}
return result, nil
})
}
+func validateNamespacePropertyUpdates(removals []string, updates
iceberg.Properties) error {
+ if _, ok := updates[namespaceExistsProperty]; ok {
Review Comment:
The removals loop below goes through `isReservedNamespaceProperty`, but this
update check reaches into the map directly. Same result today, but if a second
reserved key ever gets added to the predicate this branch silently misses it —
I'd range over `updates` and call the helper here too, so there's one source of
truth.
--
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]
