laskoviymishka commented on code in PR #2027:
URL: https://github.com/apache/iceberg-go/pull/2027#discussion_r4060141589
##########
schema.go:
##########
@@ -424,6 +424,12 @@ func (s *Schema) FindColumnName(fieldID int) (string,
bool) {
return col, ok
}
+// ColumnNamesRef returns the schema-owned field-name index for trusted
+// internal callers. The returned map must be treated as read-only.
Review Comment:
This returns the schema's live `idToName` map, and "read-only" in the doc is
doing a lot of load-bearing work given how sharp the edge is.
`lazyIDToName` caches that exact map and hands it back on every hit, so any
write from a future caller (`names[id] = "x"`, or a `delete`) mutates the cache
for every goroutine holding the schema, not just a local copy. Today's caller
only reads it so nothing's broken, but I'd rather not ship the map itself under
a comment.
I'd narrow this to a per-lookup helper and skip exposing the map at all:
```go
func (s *Schema) LookupColumnNameRef(id int, _ internal.SchemaRef) (string,
bool)
```
That keeps the allocation win, removes the mutation surface, and drops the
error return that can't fire anyway (see below). wdyt?
##########
table/scan_planning_remote.go:
##########
@@ -52,14 +53,19 @@ func remotePlanningSelectedFields(scan *Scan, schema
*iceberg.Schema) ([]string,
}
ids := make([]int, 0, schema.NumFields())
- for _, field := range schema.Fields() {
+ for _, field := range schema.FieldsRef(iceinternal.SchemaRef{}) {
appendRemoteProjectedFieldIDs(&ids, field)
}
slices.Sort(ids)
+ columnNames, err := schema.ColumnNamesRef(iceinternal.SchemaRef{})
Review Comment:
This err branch can't actually fire: `lazyIDToName` only errors if
`IndexNameByID` hits duplicate names, and `checkDuplicateFieldIDs` already
rejects that at construction. Every sibling accessor (`FindColumnName`,
`FindFieldByIDRef`, `FieldIDs`) just does `idx, _ := ...` and moves on.
I'd match them and drop the error from `ColumnNamesRef` so this stays a
plain map fetch. The per-lookup-helper route above removes it for free either
way.
##########
table/scan_planning_remote.go:
##########
@@ -21,6 +21,7 @@ import (
"slices"
"github.com/apache/iceberg-go"
+ iceinternal "github.com/apache/iceberg-go/internal"
Review Comment:
Nit: the table package leans on `iceberginternal` for this import in most
production files (`iceinternal` is the minority). Not wrong, just worth
matching the plurality while we're adding a new user.
##########
schema.go:
##########
@@ -424,6 +424,12 @@ func (s *Schema) FindColumnName(fieldID int) (string,
bool) {
return col, ok
}
+// ColumnNamesRef returns the schema-owned field-name index for trusted
+// internal callers. The returned map must be treated as read-only.
+func (s *Schema) ColumnNamesRef(_ internal.SchemaRef) (map[int]string, error) {
Review Comment:
Small one: `Ref` elsewhere means "the zero-clone version of an existing
public method" (`Fields` → `FieldsRef`), but there's no `ColumnNames()` this
pairs with, and it returns an ID-to-name index rather than a list of names.
`IDToNameRef` (mirroring the internal `idToName` / `lazyIDToName`) reads truer.
Moot if we go the per-lookup-helper route.
--
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]
