You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nssalian (via GitHub)" <gi...@apache.org> on 2026/09/27 20:33:23 UTC
[PR] refactor(catalog): move namespace-property helper into catalog/internal, drop go:linkname [iceberg-go]
nssalian opened a new pull request, #2066:
URL: https://github.com/apache/iceberg-go/pull/2066
Relates to the v1 cleanup effort (#2062).
### Changes
Moves the shared namespace-property helper `getUpdatedPropsAndUpdateSummary` (and `checkForOverlap`) from the root `catalog` package into `catalog/internal`, and drops the `//go:linkname` hack plus `import _ "unsafe"` from the Glue, Hive, and SQL backends.
The linkname existed to avoid an import cycle while keeping the helper unexported. That no longer applies: `catalog/internal` already imports `catalog`, so the helper can live there, return `catalog.PropertiesUpdateSummary` directly, and be called normally by the backends (which already import `catalog/internal`). Logic is moved verbatim - no behavior change.
Internal-only refactor; no exported `catalog` symbol changes. `//go:linkname` is fragile: it breaks silently if the target signature changes, is invisible to normal tooling, and needs a `//lint:ignore` to satisfy staticcheck. Removing it is a maintainability win on its own.
### Testing
Verified locally: `go build ./...` and `go vet ./catalog/...` pass; `go test ./catalog/internal/` passes; `golangci-lint run ./catalog/...` reports no findings in the changed files.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(catalog): move namespace-property helper into catalog/internal, drop go:linkname [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] refactor(catalog): move namespace-property helper into catalog/internal, drop go:linkname [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #2066:
URL: https://github.com/apache/iceberg-go/pull/2066
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org