zeroshade commented on issue #2062:
URL: https://github.com/apache/iceberg-go/issues/2062#issuecomment-6476832377
## Proposal: make schema types deeply immutable before v1
This follows up on the `FieldsRef` decision above. The sentinel accessors
exist only because schema types can be mutated through references. If we remove
that mutability, every public read becomes zero-copy, and the `Ref` accessors,
`cloneField`, `cloneFields` and `cloneType` can all be deleted. This changes
the public API, so it has to land before v1 or wait for `/v2`.
### Why reads clone today
`NestedField` embeds `Type`, and the composite types are pointers whose
contents are exported:
- `StructType.FieldList []NestedField`
- `ListType.Element Type`
- `MapType.KeyType` / `ValueType Type`
- `PartitionField.SourceIDs []int`
- `NestedField.InitialDefault` / `WriteDefault any`, which can hold `[]byte`
for binary and fixed defaults
A caller holding any field can reach into the schema's own data. So since
#1534, `Schema.Fields()`, `Field(i)` and `FindFieldByID()` deep-clone
everything they return, and the hot paths needed `FieldsRef` /
`FindFieldByIDRef` / `PartitionSpec.FieldsRef` to avoid that cost (#1945,
#1957).
### Proposed change
- **Composite types become opaque values with constructors and accessors.**
- `NewStructType(fields ...NestedField)`, `NewListType(elementID int,
element Type, required bool)` and `NewMapType(keyID int, key Type, valueID int,
value Type, valueRequired bool)`.
- Read through accessors: `Fields() iter.Seq[NestedField]`, `Field(i)`,
`NumFields()`, `Element()`, `ElementID()`, `KeyType()`, `ValueType()`, and so
on.
- Constructors copy their input slices, so the caller keeps no reference
to the type's internal state.
- **`NestedField` stays a plain struct with exported scalar fields.** Once
its `Type` is immutable, copying a `NestedField` is enough to make it safe.
That keeps the roughly 2,400 `NestedField{...}` literals in the repo, and those
in user code, unchanged.
- **Defaults:** copy `[]byte` values on construction and return copies, or
store them as strings internally. Other default values are already immutable
scalars.
- **`PartitionField.SourceIDs`:** unexport it behind `SourceIDs()
iter.Seq[int]` (or a copy) plus the existing `FirstSourceID()`.
- **Reads:**
- `Schema.FindFieldByID` and `Field(i)` return the stored field by value,
with no cloning.
- `Schema.Fields()` returns an iterator, or a shallow copy of the
top-level slice. Either is cheap, unlike today's deep clone.
- **Removed:** `FieldsRef`, `FindFieldByIDRef`, `PartitionSpec.FieldsRef`,
`internal.SchemaRef`, `internal.PartitionSpecRef`, and the clone helpers.
The JSON and Avro encodings stay the same; only the Go API changes.
### Cost
- **In this repo:** about 620 composite-type literals need rewriting
(roughly 315 `StructType{`, 153 `ListType{` and 150 `MapType{`), plus every
read of `.FieldList`, `.Element`, `.KeyType` and `.ValueType`. The rewrite is
mechanical and can be scripted with `gofmt -r` or AST rewrites, but it touches
most packages.
- **For users:** any code that builds types with struct literals or reads
those fields directly must switch to the constructors and accessors. The
release note can include the mechanical rewrite.
- **Conflicts:** because it touches so many files, it will conflict with
almost every open PR.
### Suggested sequencing
1. Land the other API-breaking items listed here first: the manifest-context
change, the inspect move, the `Catalog` split and the SQL view-signature
alignment.
2. Then announce a short freeze, land this as its own PR series
(constructors and accessors first, then the call-site migration, then removal
of the clone helpers and `Ref` accessors), and rebase open PRs afterwards.
3. It should be claimed here before anyone starts. It's large enough that
two people starting on it in parallel would waste a lot of work.
If we decide not to do this before v1, the sentinel accessors stay as
documented in #2348, and the deep clone on public reads stays as it is.
--
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]
