nssalian commented on issue #2062:
URL: https://github.com/apache/iceberg-go/issues/2062#issuecomment-6483944559
Thanks @zeroshade. I ran a local experiment on all three proposals against
`main` (d3ae21a). I used an AI-assisted tool to build the prototypes and
benchmarks, and I ran and checked each result myself.
### Catalog split: +1
With `Name()` added to Hive, Glue and Hadoop, the split builds and the
catalog and cmd tests pass. REST and Hive satisfy `ViewCatalog`; SQL doesn't. A
`WithName` option covers Hive and Glue without changing `NewCatalog`. Six test
doubles in the repo needed `Name()`, so third-party fakes will break too.
- **No way to change a view.** Only REST implements `UpdateView`, and Java's
`ViewCatalog` supports replace. This can't be added after v1.
- **Wrappers hide optional interfaces.** A `struct{ catalog.Catalog }`
decorator around REST fails the `TransactionalCatalog`, `ViewCatalog` and
`RegisterableCatalog` checks, so `NewMultiTableTransaction` rejects it. An
`Unwrap() Catalog` convention with a `catalog.As[T]` helper (the
`net/http.ResponseController` pattern) fixed this and still lets a wrapper
intercept a call. Huaxin flagged the same problem in review on
apache/iceberg#13979.
- **Hive drops the caller's `default-catalog`** and always writes `"hive"`
(`catalog/hive/hive.go:368`). Java keeps what the engine passes, and the view
spec treats an unset value as the view's own catalog. This is independent of
the split.
### Sentinel: I'd revisit the decision
It doesn't keep external callers out. From another module, `func callZero[A,
R any](f func(A) R) R { var a A; return f(a) }` calls `FieldsRef`,
`FindFieldByIDRef` and `PartitionSpec.FieldsRef`, because Go infers the
internal type. Writing through the results corrupted the schema and the
partition spec. An `init()`-registered function variable in `internal` can't be
imported from outside the module, and it benchmarked within a few percent of
the sentinel method, about 230 ns/op for both, with the faster one flipping
between runs (0 allocs either way).
### Deep immutability: +1 for before v1
`Fields()` on a 200-field nested schema, two runs of 3:
| | ns/op | allocs |
|---|---|---|
| today (deep clone) | 46,900 to 51,000 | 401 |
| shallow copy | 3,000 to 3,300 | 1 |
```go
for b.Loop() { sink += len(sc.Fields()) }
// today
for b.Loop() { sink += len(slices.Clone(sc.FieldsRef(internal.SchemaRef{})))
} // shallow copy
```
- **JSON depends on the exported fields.** With `FieldList` unexported,
decoding a real schema returned no error and an empty struct. A copy of the
round-trip test at `schema_test.go:1176` catches it.
- **Copies share `[]byte` defaults.** Mutating a copied `NestedField`'s
default changed the original. The spec already stores these as hex strings.
- **`Fields()` is part of `NestedType`.** An iterator would change struct,
list and map types too.
### Also pre-v1: context on read paths
`Snapshot.Manifests` passes `context.Background()` to `IO.Open`, so a
stalled `Open` couldn't be cancelled. From reading the code,
`ManifestFile.Entries`, `dv.ReadDV` and `dv.ReadDVs` do the same (not run).
### Proposed additions to the plan
Could we log each decision here as it's made, as you did for the sentinel,
so @laskoviymishka can update the issue body from those comments?
Decisions, in the order they block other work:
1. `UpdateView`: in `ViewCatalog` or its own optional interface. This sets
the `ViewCatalog` shape.
2. Wrappers: document that decorators must forward optional interfaces, or
add `Unwrap()` and `catalog.As[T]`.
3. Immutability scope: `PartitionField.SourceIDs` (473 `PartitionField{`
literals), `SortField.SourceIDs` and `Schema.IdentifierFieldIDs`.
4. Sentinel accessors: an `init()`-registered function in `internal`, or
keep them and reword #2348 to "must not be called", including
`VisitBoundPredicateRef`. Immutability removes the schema and partition spec
ones.
Changes, in order:
- [ ] Hive views keep the caller's `default-catalog`.
- [ ] `ctx` on `Snapshot.Manifests`, `ManifestFile.Entries`, `dv.ReadDV` and
`dv.ReadDVs`, with the manifest context item (@laskoviymishka).
- [ ] Catalog split with `WithName` for Hive and Glue, applying decisions 1
and 2. Release note: `Name()` breaks third-party `Catalog` implementations,
including fakes.
- [ ] Explicit JSON marshalers for `StructType`, `ListType` and `MapType`,
and hex-string `[]byte` defaults, before any schema field is unexported.
- [ ] Keep `Fields()` returning `[]NestedField` as a shallow copy.
Happy to review the PRs.
--
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]
