laskoviymishka commented on code in PR #2134:
URL: https://github.com/apache/iceberg-go/pull/2134#discussion_r4208247755
##########
schema.go:
##########
@@ -582,11 +582,69 @@ func (s *Schema) Equals(other *Schema) bool {
// HighestFieldID returns the value of the numerically highest field ID
// in this schema.
func (s *Schema) HighestFieldID() int {
- id, _ := Visit(s, findLastFieldID{})
+ id, _ := highestFieldIDFields(s.fields)
return id
}
+func highestFieldIDFields(fields []NestedField) (int, bool) {
+ if len(fields) == 0 {
+ return 0, false
+ }
+
+ highest := 0
+ for _, field := range fields {
+ nested, ok := highestFieldIDType(field.Type)
+ if !ok {
+ return 0, false
+ }
+ highest = max(highest, field.ID, nested)
+ }
+
+ return highest, true
+}
+
+func highestFieldIDType(typ Type) (int, bool) {
+ switch typ := typ.(type) {
+ case *StructType:
+ if typ == nil {
+ return 0, false
+ }
+
+ return highestFieldIDFields(typ.FieldList)
+ case *ListType:
+ if typ == nil {
+ return 0, false
+ }
+ element, ok := highestFieldIDType(typ.Element)
+ if !ok {
+ return 0, false
+ }
+
+ return max(typ.ElementID, element), true
+ case *MapType:
+ if typ == nil {
+ return 0, false
+ }
+ key, ok := highestFieldIDType(typ.KeyType)
+ if !ok {
+ return 0, false
+ }
+ value, ok := highestFieldIDType(typ.ValueType)
+ if !ok {
+ return 0, false
+ }
+
+ return max(typ.KeyID, typ.ValueID, key, value), true
+ case VariantType:
Review Comment:
`VariantType` and `PrimitiveType` return the same thing, so `case
VariantType, PrimitiveType:` folds them into one. And if the bool goes away per
above, `default` collapses to `return 0`.
##########
schema.go:
##########
@@ -582,11 +582,69 @@ func (s *Schema) Equals(other *Schema) bool {
// HighestFieldID returns the value of the numerically highest field ID
// in this schema.
func (s *Schema) HighestFieldID() int {
- id, _ := Visit(s, findLastFieldID{})
+ id, _ := highestFieldIDFields(s.fields)
return id
}
+func highestFieldIDFields(fields []NestedField) (int, bool) {
+ if len(fields) == 0 {
+ return 0, false
Review Comment:
An empty struct anywhere zeroes the whole result: `highestFieldIDFields`
returns `(0, false)` for an empty field list, that `false` propagates up, and
`HighestFieldID` hands back a plain `0` even when a sibling carries id 50. The
`default` branch does the same for any `Type` the switch doesn't list. The old
visitor landed on `0` here too, but only because `slices.Max([]int{})` panicked
and `Visit` swallowed it, so this turns that accident into deliberate control
flow. And since the value seeds `last-column-id` and the next-id assignment in
`visitors.go`, a stray `0` can collide field ids on a later append.
I'd drop the `bool`. Nothing reads it (`HighestFieldID` already discards
it). Treat an empty field list, an unknown type, and a nil pointer as
contributing `0` and keep walking; the max falls out on its own and the nil
guards collapse to one `return 0`. That's a strict improvement over the old
behavior instead of a faithful copy of its quirk.
##########
schema_highest_field_id_bench_test.go:
##########
@@ -0,0 +1,103 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package iceberg
+
+import (
+ "fmt"
+ "testing"
+)
+
+var benchmarkHighestFieldIDResult int
+
+func BenchmarkHighestFieldID(b *testing.B) {
Review Comment:
This rewrites the semantics of a write-path function but only adds a
benchmark. The existing tests just hit `tableSchemaNested.HighestFieldID()`, so
the exact shapes where the walk could diverge aren't covered. I'd add a
table-driven `TestHighestFieldID`: max id landing in a map key, a map value, a
list element, a deeply nested struct, plus the empty-struct case to pin
whatever we settle on above. Keeping the old visitor around as an oracle for a
parity check wouldn't hurt. It can live in `schema_test.go` with the rest
rather than a separate bench file.
--
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]
