laskoviymishka commented on issue #2062:
URL: https://github.com/apache/iceberg-go/issues/2062#issuecomment-6492532710
Thanks @zeroshade and @nssalian. My take:
- **Catalog split:** +1. I’d keep `UpdateView` out of `ViewCatalog` and make
it a separate optional interface, like `RenameView` and `RegisterView`. Only
REST has it today, and this lets Hive (and SQL after the alignment PR)
implement `ViewCatalog` without stubs. We can always add a new interface after
v1; growing `ViewCatalog` later is the breaking part.
- **Wrappers:** +1 to `Unwrap() Catalog` plus `catalog.As[T]`, and have
`NewMultiTableTransaction` and other capability checks use that. Asking every
wrapper to forward every optional interface is easy to get wrong, and when
someone misses one it just disappears.
- **Hive `default-catalog`:** I hit a broader issue while reviewing #2324.
Hive rebuilds the caller’s version from one representation, so it also drops
the other representations, summary, and default namespace. I’d pass the full
`*view.Version` through instead. Happy to take that.
- **Sentinel:** `internal.SchemaRef` doesn’t actually stop outside callers,
because Go can infer it through generics. From another module this works:
`func callZero[A, R any](f func(A) R) R { var a A; return f(a) }`
and `callZero(sc.FieldsRef)` compiles. I also verified that mutating the
returned slice changes the schema.
So I wouldn’t export the `Ref` methods in v1. Deep immutability removes
the need for them anyway. If that slips, `init()`-registered functions in
`internal` give us the same thing without adding public API, and @nssalian’s
numbers show the cost is the same.
- **Deep immutability:** +1 before v1. I’d start with @nssalian’s
prerequisite PRs: explicit JSON marshalers with round-trip tests, and `[]byte`
defaults stored as hex strings. Keep `Fields()` as a shallow `[]NestedField`
copy so `NestedType` doesn’t change, then do Matt’s sequencing and freeze.
I’d include `Schema.IdentifierFieldIDs`; it’s an exported slice on a
shared `*Schema`. I would not unexport `PartitionField.SourceIDs` or
`SortField.SourceIDs`. That breaks every spec/order built from literals, and
there are 473 `PartitionField{` literals in the repo alone, just to avoid
copying a tiny slice.
- **Context on read paths:** confirmed on main. `Snapshot.Manifests` opens
with `context.Background()`, and `ManifestFile.Entries`, `dv.ReadDV`, and
`dv.ReadDVs` take no ctx. I’ll include those in the manifest context work.
+1 to logging each decision here.
--
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]
