laskoviymishka commented on code in PR #2076:
URL: https://github.com/apache/iceberg-go/pull/2076#discussion_r4145275211
##########
schema.go:
##########
@@ -1555,6 +1569,13 @@ func (s *setFreshIDs) Variant(v VariantType) Type {
// fields in it. The nextID function is used to iteratively generate the ids,
if
// it is nil then a simple incrementing counter is used starting at 1.
func AssignFreshSchemaIDs(sc *Schema, nextID func() int) (*Schema, error) {
+ return AssignFreshSchemaIDsWithBase(sc, nil, nextID)
+}
+
+// AssignFreshSchemaIDsWithBase is like AssignFreshSchemaIDs, but fields whose
Review Comment:
Even with a caller-supplied `nextID`, nothing stops it minting an id that's
already been reused from base, which is the same duplicate-to-panic path as the
nil case, just pushed onto the caller. Java sidesteps this by contract (callers
pass `highestFieldID()+1`), but nothing here says so. I'd document that
`nextID` must return values above `base.HighestFieldID()`, and ideally guard
it: if a minted id is already assigned, return an error rather than let
`NewSchemaWithIdentifiers` panic.
While we're in this doc, worth stating the name match is case-sensitive
(`FindFieldByName`), since a caller merging schemas from a case-insensitive
engine will silently miss matches.
##########
schema.go:
##########
@@ -1486,15 +1486,29 @@ func buildAccessors(schema *Schema) (map[int]accessor,
error) {
type setFreshIDs struct {
oldIdToNew map[int]int
nextIDFunc func() int
+ visiting *Schema
Review Comment:
`visiting` is set once to `sc` and never mutated, so the name oversells it:
it reads like a moving cursor. I'd call it `source`, or `fromSchema` to match
Java's naming.
##########
schema.go:
##########
@@ -1486,15 +1486,29 @@ func buildAccessors(schema *Schema) (map[int]accessor,
error) {
type setFreshIDs struct {
oldIdToNew map[int]int
nextIDFunc func() int
+ visiting *Schema
+ base *Schema
}
func (s *setFreshIDs) getAndInc(currentID int) int {
- next := s.nextIDFunc()
+ next := s.idFor(currentID)
s.oldIdToNew[currentID] = next
return next
}
+func (s *setFreshIDs) idFor(currentID int) int {
+ if s.base != nil {
+ if name, ok := s.visiting.FindColumnName(currentID); ok {
+ if f, ok := s.base.FindFieldByName(name); ok {
+ return f.ID
+ }
+ }
+ }
+
+ return s.nextIDFunc()
Review Comment:
This is where `nextID` gets consumed, and the `Struct` visitor drives it in
a single interleaved pass: it recurses into a struct field's nested types
before moving on to the next sibling. Java's `AssignFreshIds.struct` assigns
all sibling ids first, then recurses. So for `{location: struct{long: NEW},
name: NEW}` with location ahead of name, Java gives `name` the lower fresh id
and Go gives it to `location.long`, and the ids come out swapped relative to
Java.
The spec only mandates uniqueness so this isn't a correctness bug, but the
PR sells itself as a Java port and anyone diffing IDs across the two clients
will see them drift. If parity is the goal I'd split the `Struct` loop into two
passes to match; the current test lists `name` before `location` so it never
hits this.
##########
schema.go:
##########
@@ -1563,7 +1584,7 @@ func AssignFreshSchemaIDs(sc *Schema, nextID func() int)
(*Schema, error) {
return id
}
}
- visitor := &setFreshIDs{oldIdToNew: make(map[int]int), nextIDFunc:
nextID}
+ visitor := &setFreshIDs{oldIdToNew: make(map[int]int), nextIDFunc:
nextID, visiting: sc, base: base}
Review Comment:
The identifier remap runs `sc.IdentifierFieldIDs` through `oldIdToNew`,
which returns 0 for any id that was never assigned (a stale identifier id in
`sc`). That 0 lands in the output `IdentifierFieldIDs` and slips past the dup
check in `NewSchemaWithIdentifiers` (it only flags ids > 0), so we can emit
`IdentifierFieldIDs=[0]`, which Java and PyIceberg read as field id 0, the
unassigned sentinel. Java re-resolves identifier fields by name
(`refreshIdentifierFields`) and errors when one is missing. I'd mirror that, or
at minimum drop unmapped ids and fail loudly. No test covers a new or stale
identifier field today either.
##########
schema_test.go:
##########
@@ -1573,6 +1573,99 @@ func TestAssignFreshSchemaIDsPreservesDefaults(t
*testing.T) {
assert.Nil(t, nestedWithoutDefaults.WriteDefault)
}
+func TestAssignFreshSchemaIDsWithBase(t *testing.T) {
Review Comment:
The cases here cover primitive lists, maps, and a scalar struct, but not a
`list<struct<...>>` with a new nested field, which is the one shape where
`FindColumnName` emits an "element" segment (`events.element.label`) and the
base lookup has to resolve through it. It reads correct, but that's exactly the
path most likely to break quietly. Worth adding a `base {events:
list<struct{ts}>}` / `sc {events: list<struct{ts, label: NEW}>}` case.
##########
schema.go:
##########
@@ -1555,6 +1569,13 @@ func (s *setFreshIDs) Variant(v VariantType) Type {
// fields in it. The nextID function is used to iteratively generate the ids,
if
// it is nil then a simple incrementing counter is used starting at 1.
func AssignFreshSchemaIDs(sc *Schema, nextID func() int) (*Schema, error) {
+ return AssignFreshSchemaIDsWithBase(sc, nil, nextID)
+}
+
+// AssignFreshSchemaIDsWithBase is like AssignFreshSchemaIDs, but fields whose
+// full name exists in base reuse the ID from base. Only fields not found in
+// base get fresh IDs from nextID.
+func AssignFreshSchemaIDsWithBase(sc, base *Schema, nextID func() int)
(*Schema, error) {
if nextID == nil {
id := 0
Review Comment:
When `base` is non-nil and `nextID` is nil, this default counter starts at 1
and hands out IDs that name-matched fields already reused from base. `base
{a:1, b:2}` with `sc {b, a, c(new)}` gives `c` the id 1, colliding with `a`.
The duplicate then trips `checkDuplicateFieldIDs` inside
`NewSchemaWithIdentifiers`, which panics instead of returning, and that panic
isn't under the `PreOrderVisit` recover, so it escapes the `(*Schema, error)`
contract entirely.
I'd seed the default at `max(sc.HighestFieldID(), base.HighestFieldID())`
when `base` is set so the first minted id is safely above everything reused,
and add a test on this exact path (nil `nextID`, low base ids) asserting a
clean result rather than a panic.
--
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]
