laskoviymishka commented on code in PR #2066:
URL: https://github.com/apache/iceberg-go/pull/2066#discussion_r4119818257
##########
catalog/internal/utils.go:
##########
@@ -324,3 +324,53 @@ func UpdateAndStageTable(ctx context.Context, catprops
iceberg.Properties, curre
),
}, nil
}
+
+func checkForOverlap(removals []string, updates iceberg.Properties) error {
+ overlap := []string{}
+ for _, key := range removals {
+ if _, ok := updates[key]; ok {
+ overlap = append(overlap, key)
+ }
+ }
+ if len(overlap) > 0 {
+ return fmt.Errorf("conflict between removals and updates for
keys: %v", overlap)
+ }
+
+ return nil
+}
+
+// GetUpdatedPropsAndUpdateSummary applies removals and updates to currentProps
+// and returns the updated properties alongside a summary of the changes. It is
+// shared by the catalog backend implementations.
+func GetUpdatedPropsAndUpdateSummary(currentProps iceberg.Properties, removals
[]string, updates iceberg.Properties) (iceberg.Properties,
catalog.PropertiesUpdateSummary, error) {
Review Comment:
Now that this is a plain exported function with no backend dependencies, I'd
add a table-driven test right here in `catalog/internal`. Before this move it
could only be exercised indirectly through each backend, and it's the shared
source of truth for namespace-property update semantics across sql/hive/glue.
Worth covering the overlap-conflict error, the `Missing` computation, and the
normal add/update/remove path. That also lets the three backend test files drop
their duplicated cases.
##########
catalog/internal/utils.go:
##########
@@ -324,3 +324,53 @@ func UpdateAndStageTable(ctx context.Context, catprops
iceberg.Properties, curre
),
}, nil
}
+
+func checkForOverlap(removals []string, updates iceberg.Properties) error {
+ overlap := []string{}
+ for _, key := range removals {
+ if _, ok := updates[key]; ok {
+ overlap = append(overlap, key)
+ }
+ }
+ if len(overlap) > 0 {
+ return fmt.Errorf("conflict between removals and updates for
keys: %v", overlap)
+ }
+
+ return nil
+}
+
+// GetUpdatedPropsAndUpdateSummary applies removals and updates to currentProps
+// and returns the updated properties alongside a summary of the changes. It is
+// shared by the catalog backend implementations.
+func GetUpdatedPropsAndUpdateSummary(currentProps iceberg.Properties, removals
[]string, updates iceberg.Properties) (iceberg.Properties,
catalog.PropertiesUpdateSummary, error) {
+ if err := checkForOverlap(removals, updates); err != nil {
+ return nil, catalog.PropertiesUpdateSummary{}, err
+ }
+ var (
+ updatedProps = maps.Clone(currentProps)
+ removed = make([]string, 0, len(removals))
+ updated = make([]string, 0, len(updates))
+ )
+
+ for _, key := range removals {
+ if _, exists := updatedProps[key]; exists {
+ delete(updatedProps, key)
+ removed = append(removed, key)
+ }
+ }
+
+ for key, value := range updates {
+ if updatedProps[key] != value {
Review Comment:
Flagging for a follow-up, not this PR. This branch is moved verbatim, so
it's not a regression. But it's where Go diverges from the Java REST reference:
we only add a key to `Updated` when the value actually changes, whereas
`CatalogHandlers.updateNamespaceProperties` (and PyIceberg's
`_get_updated_props_and_update_summary`) add every requested key
unconditionally. So a caller re-sending the same value idempotently gets it
reported as updated by a Java-backed catalog but silently omitted by Go's
sql/hive/glue. Since this PR is the point where the logic becomes shared and
independently testable, it's a good moment to file it (or fix while the diff is
fresh) so the three backends match the reference.
##########
catalog/internal/utils.go:
##########
@@ -324,3 +324,53 @@ func UpdateAndStageTable(ctx context.Context, catprops
iceberg.Properties, curre
),
}, nil
}
+
+func checkForOverlap(removals []string, updates iceberg.Properties) error {
+ overlap := []string{}
+ for _, key := range removals {
+ if _, ok := updates[key]; ok {
+ overlap = append(overlap, key)
+ }
+ }
+ if len(overlap) > 0 {
+ return fmt.Errorf("conflict between removals and updates for
keys: %v", overlap)
+ }
+
+ return nil
+}
+
+// GetUpdatedPropsAndUpdateSummary applies removals and updates to currentProps
+// and returns the updated properties alongside a summary of the changes. It is
+// shared by the catalog backend implementations.
+func GetUpdatedPropsAndUpdateSummary(currentProps iceberg.Properties, removals
[]string, updates iceberg.Properties) (iceberg.Properties,
catalog.PropertiesUpdateSummary, error) {
+ if err := checkForOverlap(removals, updates); err != nil {
+ return nil, catalog.PropertiesUpdateSummary{}, err
+ }
+ var (
+ updatedProps = maps.Clone(currentProps)
+ removed = make([]string, 0, len(removals))
+ updated = make([]string, 0, len(updates))
+ )
+
+ for _, key := range removals {
+ if _, exists := updatedProps[key]; exists {
+ delete(updatedProps, key)
+ removed = append(removed, key)
+ }
+ }
+
+ for key, value := range updates {
+ if updatedProps[key] != value {
+ updated = append(updated, key)
+ updatedProps[key] = value
+ }
+ }
+
+ summary := catalog.PropertiesUpdateSummary{
+ Removed: removed,
+ Updated: updated,
+ Missing: internal.Difference(removals, removed),
Review Comment:
This file is itself `package internal` but imports the top-level
`github.com/apache/iceberg-go/internal` unaliased, so `internal.Difference`
here reads like a self-reference when it's actually the sibling package. The
old `catalog.go` aliased it `iceinternal` to avoid exactly this. Since this
move adds a second call site leaning on the shadowing, I'd carry that alias
over.
--
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]
