tanmayrauth commented on code in PR #1411:
URL: https://github.com/apache/iceberg-go/pull/1411#discussion_r3525956586
##########
catalog/sql/sql_test.go:
##########
@@ -1129,7 +1129,35 @@ 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 {
+ cat *sqlcat.Catalog
+ namespace table.Identifier
+ }{
+ {s.getCatalogMemory(), table.Identifier{databaseName()}},
Review Comment:
Since this seeds `{"comment": "baseline"}`, no sentinel row is actually
written, so the guard is only exercised at the argument layer. Could you add a
case that creates a namespace with empty props (which does insert the
`exists=true` row) and then asserts remove/update of `exists` is still rejected
and the namespace loads empty? That covers the real-world path where the
sentinel is present.
##########
catalog/sql/sql.go:
##########
@@ -672,13 +677,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) {
+ return fmt.Errorf("%w: cannot remove reserved namespace
property %q", iceberg.ErrInvalidArgument, namespaceExistsProperty)
Review Comment:
Small one: the loop binds `key` but this formats
`namespaceExistsProperty`. Same value today since only "exists" is reserved,
but if the reserved set ever grows this will name the wrong key in the error —
probably want `key` here.
--
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]
