refactor(catalog): move namespace-property helper into catalog/internal, drop go:linkname - #2066
Conversation
…al, drop go:linkname
laskoviymishka
left a comment
There was a problem hiding this comment.
Dropping the //go:linkname + import _ "unsafe" triad across glue/hive/sql for a plain exported helper in catalog/internal is a real cleanup. That hack was fragile and bypassed the visibility rules for no reason now that all three backends already import the package. The move looks faithful and the build's clean.
Two small things before merge: I'd add a table-driven test for GetUpdatedPropsAndUpdateSummary now that it's finally testable in isolation (overlap error, Missing, the normal add/update/remove path), and I'd carry the old iceinternal alias over so internal.Difference doesn't read like a self-reference inside package internal.
One thing worth a follow-up, not this PR: the Updated summary only lists keys whose value actually changed, which diverges from the Java REST reference and PyIceberg (both list every requested key). It's moved verbatim so it's not a regression, but this is the moment it becomes shared and testable, so it's a good time to file it.
| // 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) { |
There was a problem hiding this 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.
| summary := catalog.PropertiesUpdateSummary{ | ||
| Removed: removed, | ||
| Updated: updated, | ||
| Missing: internal.Difference(removals, removed), |
There was a problem hiding this 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.
| } | ||
|
|
||
| for key, value := range updates { | ||
| if updatedProps[key] != value { |
There was a problem hiding this 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.
Relates to the v1 cleanup effort (#2062).
Changes
Moves the shared namespace-property helper
getUpdatedPropsAndUpdateSummary(andcheckForOverlap) from the rootcatalogpackage intocatalog/internal, and drops the//go:linknamehack plusimport _ "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/internalalready importscatalog, so the helper can live there, returncatalog.PropertiesUpdateSummarydirectly, and be called normally by the backends (which already importcatalog/internal). Logic is moved verbatim - no behavior change.Internal-only refactor; no exported
catalogsymbol changes.//go:linknameis fragile: it breaks silently if the target signature changes, is invisible to normal tooling, and needs a//lint:ignoreto satisfy staticcheck. Removing it is a maintainability win on its own.Testing
Verified locally:
go build ./...andgo vet ./catalog/...pass;go test ./catalog/internal/passes;golangci-lint run ./catalog/...reports no findings in the changed files.