You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "rambleraptor (via GitHub)" <gi...@apache.org> on 2026/07/13 21:27:22 UTC
[PR] feat(transforms): read unknown partition and sort transforms (v3) [iceberg-go]
rambleraptor opened a new pull request, #1463:
URL: https://github.com/apache/iceberg-go/pull/1463
## Why
The v3 spec requires readers to load tables whose partition specs or sort orders use transforms the implementation doesn't recognize, and to **ignore those partition fields when filtering** (spec [Β§Partition Transforms](https://iceberg.apache.org/spec/#partition-transforms): *"All v3 readers are required to read tables with unknown transforms, ignoring them"*). Today `ParseTransform` returns `ErrInvalidTransform` for any unrecognized string, so `UnmarshalJSON` of a partition field or sort field fails outright β a v3 interop hazard and a spec violation.
This matches the Java reference, where `Transforms.fromString` returns an `UnknownTransform` for anything it doesn't recognize.
## What changed
- **`UnknownTransform`** (`transforms.go`): a placeholder carrying the original transform string. `CanTransform` is permissive, `ResultType` is `string` (matching Java), and `Project` returns `nil` so scan planning never prunes on an unknown partition field. Round-trips verbatim (original casing preserved).
- **`ParseTransform`** now returns `UnknownTransform` for unrecognized strings instead of erroring. A *malformed known* transform (`bucket[0]`, `truncate[0]`, width overflow) still errors, matching Java (`Bucket.get`/`Truncate.get` validation).
- **Writer restriction**: `validateTransform` rejects committing a partition spec that uses an unknown transform (spec: *"Writers are not allowed to commit data using a partition spec that contains a field with an unknown transform"*). This covers all spec-construction paths (`NewPartitionSpecOpts`, `BindToSchema`, `UpdateSpec.Apply`). The read path (`PartitionSpec.UnmarshalJSON`) sets fields directly and is unaffected.
- **Filter expressions stay strict**: an unknown transform in an expression term is still rejected β a filter referencing a transform we can't evaluate must not silently drop semantics. Only partition/sort *metadata* tolerates unknown transforms.
- Propagate the `ParseTransform` error in `UpdateSpec.partitionField` instead of discarding it (was `transform, _ := ...`, a latent nil-transform hazard).
## Testing
- `ParseTransform`: unrecognized strings (incl. mixed-case) parse to `UnknownTransform` and round-trip; malformed known transforms still error.
- Partition field JSON round-trips an unknown transform; building a spec with one is rejected.
- Sort orders load unknown transforms (spec requires it) but still reject malformed known ones.
- Expression terms reject unknown transforms.
- `go test ./...` and `go vet ./...` pass; `golangci-lint` clean.
π€ Generated with [Claude Code](https://claude.com/claude-code)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transforms): read unknown partition and sort transforms (v3) [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#discussion_r3577774200
##########
transforms.go:
##########
@@ -77,7 +84,9 @@ func ParseTransform(s string) (Transform, error) {
return HourTransform{}, nil
}
- return nil, fmt.Errorf("%w: %s", ErrInvalidTransform, s)
+ // Unknown transform: v3 readers must load these and ignore them when
+ // filtering instead of failing. Keep the original string so it round-trips.
+ return UnknownTransform{name: s}, nil
Review Comment:
Since `ParseTransform` now keeps the original casing here and `Equals` compares `name` byte-for-byte, `"Custom_V2[3]"` and `"custom_v2[3]"` become two distinct `UnknownTransform`s β the new test even pins that behavior. On a partition spec that'd let two effectively-identical unknown fields coexist. Probably fine in practice, but I'd either normalize the stored name or leave a comment noting the case-sensitivity is intentional for round-trip fidelity.
##########
table/sorting_test.go:
##########
@@ -262,6 +280,33 @@ func TestUnmarshalInvalidSortTransform(t *testing.T) {
]
}`
+ var order table.SortOrder
+ err := json.Unmarshal([]byte(j), &order)
+ require.NoError(t, err)
+ require.Equal(t, 3, order.Len())
+
+ var first table.SortField
+ for i, f := range order.Fields() {
Review Comment:
Tiny thing β this loop only exists to grab field 0. `order.Fields()[0]` (or a `Field(0)` accessor if we add one) reads cleaner.
##########
partitions_test.go:
##########
@@ -191,6 +191,47 @@ func TestPartitionSpec_MarshalTextRejectsInvalidBucketTransform(t *testing.T) {
require.ErrorContains(t, err, "numBuckets > 0")
}
+// A v3 table may use a partition transform this implementation doesn't know.
+// Readers must load it and preserve it on write, ignoring it when filtering.
+func TestPartitionFieldUnknownTransformRoundTrip(t *testing.T) {
Review Comment:
Nice that this proves the field round-trips. Could we add a sibling test that unmarshals a full table-metadata JSON β partition specs and all β with an unknown transform and asserts it loads clean? That's the case that actually matters for the read goal, and the one thing that proves loading a v3 table doesn't trip `validateTransform` anywhere on the metadata path. The field-level test doesn't exercise that.
##########
table/sorting.go:
##########
@@ -320,6 +320,10 @@ func newSortOrder(orderID int, fields []SortField, validateSourceIDs bool) (Sort
return SortOrder{}, fmt.Errorf("%w: sort field at index %d has invalid source IDs: %v",
ErrInvalidSortSourceID, idx, err)
}
+ if u, ok := field.Transform.(iceberg.UnknownTransform); ok {
Review Comment:
Two things on this guard.
First, it's stricter than the spec and than Java. The spec only forbids writing partition specs with unknown transforms, and Java's `SortOrder` builder doesn't reject unknown sort transforms (`canTransform` stays true). A v3 sort order round-tripped through `NewSortOrder` β say during `UpdateSortOrder` β would get wrongly rejected here. I think this ties into tanmayrauth's sort-order thread, so I want to make sure we actually want to reject on write at all, rather than tolerate it like Java does.
Second, if we do keep it: it's inside the `validateSourceIDs` block, so the JSON unmarshal path (`validateSourceIDs=false`) skips it entirely, and `MetadataBuilder.AddSortOrder` bypasses it too. That's an inconsistent guard. I'd pull it out of the `validateSourceIDs` branch so write and load agree. wdyt?
##########
partitions.go:
##########
@@ -291,6 +291,8 @@ func validateTransform(transform Transform) error {
return t.validateNumBuckets()
case *BucketTransform:
return t.validateNumBuckets()
+ case UnknownTransform:
Review Comment:
This case is right for a direct write, but it also fires on the preserve path. `UpdateSpec.Apply` rebuilds the whole spec through `NewPartitionSpecOpts`, so every existing field β including a passthrough unknown-transform one we loaded from a v3 table β gets re-validated here. That means once a table has an unknown partition field, we can't evolve its spec at all: renaming or dropping an unrelated field fails with `ErrInvalidTransform` at commit.
I'd only validate the fields being newly added, and route the preserved fields through a non-validating construction. Java rejects at the `UpdateSpec` constructor with a clear message rather than an opaque commit-time error, so an early guard there would be friendlier too. A test that evolves a spec on an unknown-transform table would lock this down. wdyt?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transforms): read unknown partition and sort transforms (v3) [iceberg-go]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#discussion_r3574774838
##########
partitions.go:
##########
@@ -291,6 +291,8 @@ func validateTransform(transform Transform) error {
return t.validateNumBuckets()
case *BucketTransform:
return t.validateNumBuckets()
+ case UnknownTransform:
Review Comment:
Mirrored the rejection.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transforms): read unknown partition and sort transforms (v3) [iceberg-go]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#discussion_r3582851610
##########
transforms.go:
##########
@@ -77,7 +84,9 @@ func ParseTransform(s string) (Transform, error) {
return HourTransform{}, nil
}
- return nil, fmt.Errorf("%w: %s", ErrInvalidTransform, s)
+ // Unknown transform: v3 readers must load these and ignore them when
+ // filtering instead of failing. Keep the original string so it round-trips.
+ return UnknownTransform{name: s}, nil
Review Comment:
I kept the behavior + added a comment. I double checked Java and it appears to do a byte-for-byte check as well.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transforms): read unknown partition and sort transforms (v3) [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transforms): read unknown partition and sort transforms (v3) [iceberg-go]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#issuecomment-5095823584
@zeroshade changes made! Thanks for your patience. I've been in-and-out on PTO.
One quick note - It looks like Java treats bucket[-1] as an unknown transform, not an error. I'm treating as an error now, but we can potentially open that up in the future.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transforms): read unknown partition and sort transforms (v3) [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#discussion_r3732110097
##########
expr_json.go:
##########
@@ -579,6 +579,11 @@ func decodeTerm(raw json.RawMessage) (UnboundTerm, error) {
if err != nil {
return nil, fmt.Errorf("%w: cannot parse transform term: %s", ErrInvalidArgument, err)
}
+ // Unknown transforms are tolerated in partition/sort metadata, but a
+ // filter expression referencing one can't be evaluated.
+ if _, ok := tf.(UnknownTransform); ok {
Review Comment:
This rejection has no test coverage at all. It is a small addition, but it is the only thing standing between an unknown transform and a malformed REST filter payload.
Suggested fix: add a `decodeTerm` case feeding a term whose transform is unknown and assert `ErrInvalidArgument`.
##########
transforms.go:
##########
@@ -230,6 +246,49 @@ func (VoidTransform) Project(string, BoundPredicate) (UnboundPredicate, error) {
return nil, nil
}
+// UnknownTransform is a placeholder for a partition or sort transform that
+// this implementation doesn't recognize. The v3 spec requires readers to load
+// tables that use unknown transforms and to ignore those fields when
+// filtering; writers must not commit a partition spec that uses one.
+type UnknownTransform struct {
+ name string
+}
+
+func (t UnknownTransform) MarshalText() ([]byte, error) {
+ return []byte(t.name), nil
+}
+
+func (t UnknownTransform) String() string { return t.name }
+
+// CanTransform assumes an unknown transform could apply to any type -- the
+// real applicability isn't known.
+func (UnknownTransform) CanTransform(Type) bool { return true }
Review Comment:
Non-blocking: because `CanTransform` is unconditionally true, `SortOrder.CheckCompatibility` silently accepts any source type for an unknown transform. That is the right runtime behavior β we genuinely cannot know β but the doc comment reads as if compatibility were verified.
Suggested fix: note on this method that compatibility is *unverifiable* for unknown transforms, not verified.
##########
transforms.go:
##########
@@ -230,6 +246,49 @@ func (VoidTransform) Project(string, BoundPredicate) (UnboundPredicate, error) {
return nil, nil
}
+// UnknownTransform is a placeholder for a partition or sort transform that
+// this implementation doesn't recognize. The v3 spec requires readers to load
+// tables that use unknown transforms and to ignore those fields when
+// filtering; writers must not commit a partition spec that uses one.
+type UnknownTransform struct {
+ name string
+}
+
+func (t UnknownTransform) MarshalText() ([]byte, error) {
+ return []byte(t.name), nil
+}
+
+func (t UnknownTransform) String() string { return t.name }
+
+// CanTransform assumes an unknown transform could apply to any type -- the
+// real applicability isn't known.
+func (UnknownTransform) CanTransform(Type) bool { return true }
+
+// ResultType is unknown, so report string, matching the Java reference.
+func (UnknownTransform) ResultType(Type) Type { return StringType{} }
+
+func (UnknownTransform) PreservesOrder() bool { return false }
+
+func (t UnknownTransform) Equals(other Transform) bool {
+ o, ok := other.(UnknownTransform)
+
+ return ok && t.name == o.name
+}
+
+// Apply can't be evaluated for an unknown transform.
+func (UnknownTransform) Apply(Optional[Literal]) Optional[Literal] {
+ return Optional[Literal]{}
+}
+
+func (t UnknownTransform) ToHumanStr(any) string { return t.name }
+
+func (t UnknownTransform) ToHumanStrType(Type, any) string { return t.name }
Review Comment:
**Blocking (part of the position-delete issue in the summary).** `ToHumanStrType` and `ToHumanStr` (`:283`) return the transform *name*, so they are constant for a given spec field regardless of the value passed in.
`PartitionSpec.PartitionToPath` (`partitions.go:710`) feeds this straight into the writer key at `table/rolling_data_writer.go:348`. Two genuinely different partition values therefore produce the identical path string, the map lookup returns the writer created for the first one, and the new `partitionValues` are discarded. The same collapse shows up in `table/snapshots.go:459,474`, where `partition-summaries` and `changed-partition-count` come out wrong.
Suggested fix: render the value rather than the transform name. Java's `Transform#toHumanString` default renders the value and `UnknownTransform` does not override it, so matching that behavior also fixes the summary counts and makes it impossible for any future `PartitionToPath` caller to collide.
##########
transforms.go:
##########
@@ -230,6 +246,49 @@ func (VoidTransform) Project(string, BoundPredicate) (UnboundPredicate, error) {
return nil, nil
}
+// UnknownTransform is a placeholder for a partition or sort transform that
+// this implementation doesn't recognize. The v3 spec requires readers to load
+// tables that use unknown transforms and to ignore those fields when
+// filtering; writers must not commit a partition spec that uses one.
+type UnknownTransform struct {
Review Comment:
Non-blocking: `iceberg.UnknownTransform{}` is a legal composite literal from outside the package β all fields being unexported makes them unsettable, not the struct unconstructible. That yields `name == ""`, which marshals to `"transform": ""` and produces metadata that cannot be read back. `MarshalText` returning no error also bypasses the `newSortOrder` backstop at `table/sorting.go:321`.
Suggested fix: reject empty names in `validateTransform` (`partitions.go:314`), or return an error from `MarshalText` when `name` is empty.
##########
table/sorting.go:
##########
@@ -249,6 +253,11 @@ func (s SortOrder) Len() int {
return len(s.fields)
}
+// Field returns the sort field at index i.
+func (s SortOrder) Field(i int) SortField {
Review Comment:
Non-blocking: the new `Field(i)` returns `s.fields[i]` raw while `Fields()` (`:242`) clones. `SortField.SourceIDs` is a slice, so a caller can mutate the sort order's internals through the returned value β which breaks exactly the invariant `TestSortOrderReturnsDefensiveCopies` asserts for `Fields()`.
Suggested fix: return `cloneSortField(s.fields[i])`, and document that an out-of-range index panics.
##########
table/update_spec.go:
##########
@@ -86,6 +86,12 @@ func NewUpdateSpec(t *Transaction, caseSensitive bool) *UpdateSpec {
nameToField := make(map[string]iceberg.PartitionField)
partitionSpec := t.tbl.Metadata().PartitionSpec()
for _, partitionField := range partitionSpec.Fields() {
+ if _, ok := partitionField.Transform.(iceberg.UnknownTransform); ok {
Review Comment:
Worth surfacing to users: this blanket rejection blocks *all* spec evolution on a table with an unknown transform, including unrelated renames and drops. You confirmed this matches Java and it is the safer default, so no change to the behavior β but the rationale currently lives only in `update_spec_test.go:64`.
Suggested fix: move that reasoning into the `NewUpdateSpec` doc comment, where someone hitting the error will actually find it.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transforms): read unknown partition and sort transforms (v3) [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#issuecomment-5178716052
Need @zeroshade approval to unblock merge 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transforms): read unknown partition and sort transforms (v3) [iceberg-go]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#discussion_r3582839102
##########
partitions_test.go:
##########
@@ -191,6 +191,47 @@ func TestPartitionSpec_MarshalTextRejectsInvalidBucketTransform(t *testing.T) {
require.ErrorContains(t, err, "numBuckets > 0")
}
+// A v3 table may use a partition transform this implementation doesn't know.
+// Readers must load it and preserve it on write, ignoring it when filtering.
+func TestPartitionFieldUnknownTransformRoundTrip(t *testing.T) {
Review Comment:
Great idea for a test. Added it.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transforms): read unknown partition and sort transforms (v3) [iceberg-go]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#issuecomment-4974312317
@laskoviymishka thanks for the review! I've gone ahead and made those changes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transforms): read unknown partition and sort transforms (v3) [iceberg-go]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#discussion_r3582846273
##########
table/sorting.go:
##########
@@ -320,6 +320,10 @@ func newSortOrder(orderID int, fields []SortField, validateSourceIDs bool) (Sort
return SortOrder{}, fmt.Errorf("%w: sort field at index %d has invalid source IDs: %v",
ErrInvalidSortSourceID, idx, err)
}
+ if u, ok := field.Transform.(iceberg.UnknownTransform); ok {
Review Comment:
Originally, I was thinking that we should be stricter than Java and then could open it up in a follow-up PR. Let's go ahead and tolerate it instead. It's better to stick with the reference implementation than figure out the rejection logic.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transforms): read unknown partition and sort transforms (v3) [iceberg-go]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#discussion_r3582837979
##########
partitions.go:
##########
@@ -291,6 +291,8 @@ func validateTransform(transform Transform) error {
return t.validateNumBuckets()
case *BucketTransform:
return t.validateNumBuckets()
+ case UnknownTransform:
Review Comment:
Sounds good. I added the early guard + tests.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transforms): read unknown partition and sort transforms (v3) [iceberg-go]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#issuecomment-5095888484
@laskoviymishka thanks for your patience on this one! just got back from vacation and made those changes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transforms): read unknown partition and sort transforms (v3) [iceberg-go]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#issuecomment-5172006814
@laskoviymishka handled the merge conflicts! thanks for your patience. this should be ready to merge.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transforms): read unknown partition and sort transforms (v3) [iceberg-go]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on code in PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#discussion_r3660332811
##########
table/update_spec.go:
##########
@@ -82,6 +82,12 @@ func NewUpdateSpec(t *Transaction, caseSensitive bool) *UpdateSpec {
nameToField := make(map[string]iceberg.PartitionField)
partitionSpec := t.tbl.Metadata().PartitionSpec()
for _, partitionField := range partitionSpec.Fields() {
+ if _, ok := partitionField.Transform.(iceberg.UnknownTransform); ok {
Review Comment:
Yeah, should've been more explicit about that. I did that for Java parity. Happy to change it back if you want.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transforms): read unknown partition and sort transforms (v3) [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#discussion_r3590554889
##########
transforms.go:
##########
@@ -230,6 +240,49 @@ func (VoidTransform) Project(string, BoundPredicate) (UnboundPredicate, error) {
return nil, nil
}
+// UnknownTransform is a placeholder for a partition or sort transform that
+// this implementation doesn't recognize. The v3 spec requires readers to load
+// tables that use unknown transforms and to ignore those fields when
+// filtering; writers must not commit a partition spec that uses one.
+type UnknownTransform struct {
+ name string
+}
+
+func (t UnknownTransform) MarshalText() ([]byte, error) {
+ return []byte(t.name), nil
+}
+
+func (t UnknownTransform) String() string { return t.name }
+
+// CanTransform assumes an unknown transform could apply to any type -- the
+// real applicability isn't known.
+func (UnknownTransform) CanTransform(Type) bool { return true }
+
+// ResultType is unknown, so report string, matching the Java reference.
+func (UnknownTransform) ResultType(Type) Type { return StringType{} }
+
+func (UnknownTransform) PreservesOrder() bool { return false }
+
+func (t UnknownTransform) Equals(other Transform) bool {
+ o, ok := other.(UnknownTransform)
+
+ return ok && t.name == o.name
+}
+
+// Apply can't be evaluated for an unknown transform.
+func (UnknownTransform) Apply(Optional[Literal]) Optional[Literal] {
Review Comment:
this is the round-1 write-path concern, still open β flagging it here because `Apply` returning an empty Optional (and `ToHumanStrType` just below returning the transform name) is exactly what lets it corrupt silently.
A table loaded with an unknown partition transform isn't unpartitioned (`IsUnpartitioned` only counts `VoidTransform`), so `WriteRecords` takes the partitioned path, `getRecordPartitions` calls this `Apply`, gets `Valid:false` for every row, and we write null partition values under a `field=<transform-name>` dir β unreadable by Java/PyIceberg. On the `AddFiles` path `fileToDataFile` panics instead.
I'd guard at `recordsToDataFiles`/`WriteRecords`: range the current spec and reject if any field is an `UnknownTransform` before writing anything. That's the spec rule too β writers must not commit against an unknown-transform spec. Once that's in, this is good to land.
##########
table/update_spec.go:
##########
@@ -82,6 +82,12 @@ func NewUpdateSpec(t *Transaction, caseSensitive bool) *UpdateSpec {
nameToField := make(map[string]iceberg.PartitionField)
partitionSpec := t.tbl.Metadata().PartitionSpec()
for _, partitionField := range partitionSpec.Fields() {
+ if _, ok := partitionField.Transform.(iceberg.UnknownTransform); ok {
Review Comment:
this went the opposite way from my round-1 ask, and I want to make sure that's intentional rather than a slip.
Round 1 I asked to validate only newly-added fields, so a table with an unknown transform on one column could still rename or drop an unrelated one. This rejects any evolution up front β the `UpdateSpec` is dead before a single op is queued.
If it's deliberate parity with Java's `BaseUpdatePartitionSpec`, I'm happy with it β simpler, clearer signal, and I'll take Java's precedent over my earlier ask. Just want to confirm it's the intended final direction. wdyt?
##########
transforms_test.go:
##########
@@ -103,6 +93,45 @@ func TestParseTransform(t *testing.T) {
assert.ErrorContains(t, err, tt.toparse)
})
}
+
+ // Unrecognized transform strings parse to an UnknownTransform (v3 requires
+ // readers to load unknown transforms) and round-trip verbatim, preserving
+ // the original casing.
+ unknownTests := []struct {
+ name string
+ toparse string
+ }{
+ {"foobar", "foobar"},
+ {"bucket no brackets", "bucket"},
+ {"truncate no brackets", "truncate"},
+ {"bucket no val", "bucket[]"},
+ {"truncate no val", "truncate[]"},
+ {"bucket neg", "bucket[-1]"},
+ {"truncate neg", "truncate[-1]"},
+ {"bucket extra suffix", "bucketx[5]"},
+ {"bucket extra token", "bucket_extra[5]"},
+ {"truncate extra suffix", "truncatefoo[10]"},
+ {"truncate extra token", "truncate_garbage[4]"},
+ {"preserves original case", "Custom_V2[3]"},
+ }
+
+ for _, tt := range unknownTests {
+ t.Run("unknown/"+tt.name, func(t *testing.T) {
+ tr, err := iceberg.ParseTransform(tt.toparse)
+ require.NoError(t, err)
+ _, ok := tr.(iceberg.UnknownTransform)
+ assert.True(t, ok)
Review Comment:
tiny one: `assert.True(t, ok)` is non-fatal, so if `ParseTransform` ever regresses and returns a non-`UnknownTransform`, execution falls through to the bare `u := tr.(iceberg.UnknownTransform)` on line 130 and panics the goroutine instead of failing cleanly. `require.True` here fixes it.
##########
table/metadata_internal_test.go:
##########
@@ -309,6 +309,31 @@ func TestMetadataV3Parsing(t *testing.T) {
assert.Equal(t, int64(2000), *secondSnapshot.FirstRowID)
}
+// A full v3 metadata document must load when its partition spec and sort order
+// use unknown transforms β nothing on the metadata path may trip a guard.
+func TestMetadataV3ParsesUnknownTransforms(t *testing.T) {
+ j := strings.NewReplacer(
+ `{"name": "x", "transform": "identity", "source-id": 1, "field-id": 1000}`,
+ `{"name": "x", "transform": "custom_transform[42]", "source-id": 1, "field-id": 1000}`,
+ `{"transform": "bucket[4]", "source-id": 3, "direction": "desc", "null-order": "nulls-last"}`,
+ `{"transform": "custom_sort[7]", "source-id": 3, "direction": "desc", "null-order": "nulls-last"}`,
+ ).Replace(ExampleTableMetadataV3)
+
+ meta, err := ParseMetadataBytes([]byte(j))
+ require.NoError(t, err)
+
+ spec := meta.PartitionSpec()
+ pf := spec.Field(0)
+ _, ok := pf.Transform.(iceberg.UnknownTransform)
+ assert.True(t, ok, "unknown partition transform should load")
+ assert.Equal(t, "custom_transform[42]", pf.Transform.String())
+
+ sf := meta.SortOrder().Field(1)
+ _, ok = sf.Transform.(iceberg.UnknownTransform)
+ assert.True(t, ok, "unknown sort transform should load")
+ assert.Equal(t, "custom_sort[7]", sf.Transform.String())
Review Comment:
this is the full-metadata round-trip I asked for last round, thanks β it proves the load path doesn't trip a guard.
It only goes one direction though: `ParseMetadataBytes` then assert. The field-level `TestPartitionFieldUnknownTransformRoundTrip` covers marshal-back, so serialization itself is tested β but a marshal-back assert here (that the output still contains `custom_transform[42]`/`custom_sort[7]`) is the exact guardrail against `validateTransform` sneaking onto the write path that I was after. Cheap to add while we're 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transforms): read unknown partition and sort transforms (v3) [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on code in PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#discussion_r3574533090
##########
partitions.go:
##########
@@ -291,6 +291,8 @@ func validateTransform(transform Transform) error {
return t.validateNumBuckets()
case *BucketTransform:
return t.validateNumBuckets()
+ case UnknownTransform:
Review Comment:
Partition specs reject unknown transforms on write here, but the sort-order path doesn't: newSortOrder (table/sorting.go:309) and CheckCompatibility (table/sorting.go:334) accept them, and CheckCompatibility passes only because UnknownTransform.CanTransform returns true unconditionally. An UnknownTransform is un-evaluatable (Apply returns empty, Project returns nil), so a SortOrder built with one commits an un-enforceable sort into metadata with no guard and no test β downstream engines then read a declared sort order they can't reproduce. Either mirror the partition-spec rejection on the sort-write path, or add a test pinning the intended behavior.
##########
transforms.go:
##########
@@ -230,6 +239,49 @@ func (VoidTransform) Project(string, BoundPredicate) (UnboundPredicate, error) {
return nil, nil
}
+// UnknownTransform is a placeholder for a partition or sort transform that
+// this implementation doesn't recognize. The v3 spec requires readers to load
+// tables that use unknown transforms and to ignore those fields when
+// filtering; writers must not commit a partition spec that uses one.
+type UnknownTransform struct {
+ name string
+}
+
+func (t UnknownTransform) MarshalText() ([]byte, error) {
+ return []byte(t.name), nil
+}
+
+func (t UnknownTransform) String() string { return t.name }
+
+// CanTransform assumes an unknown transform could apply to any type -- the
+// real applicability isn't known.
+func (UnknownTransform) CanTransform(Type) bool { return true }
+
+// ResultType is unknown, so report string, matching the Java reference.
+func (UnknownTransform) ResultType(Type) Type { return StringType{} }
+
+func (UnknownTransform) PreservesOrder() bool { return false }
+
+func (t UnknownTransform) Equals(other Transform) bool {
+ o, ok := other.(UnknownTransform)
+
+ return ok && t.name == o.name
+}
+
+// Apply can't be evaluated for an unknown transform.
+func (UnknownTransform) Apply(Optional[Literal]) Optional[Literal] {
+ return Optional[Literal]{}
+}
+
+func (UnknownTransform) ToHumanStr(any) string { return "null" }
+
+func (UnknownTransform) ToHumanStrType(Type, any) string { return "null" }
+
+// Project returns nil so scans don't prune on an unknown partition field.
+func (UnknownTransform) Project(string, BoundPredicate) (UnboundPredicate, error) {
Review Comment:
This returning nil is the entire mechanism for "ignore the unknown field when filtering" β inclusiveProjection in table/evaluators.go treats a nil projection as AlwaysTrue β but there's no test for it. If a later change makes Project return a non-nil predicate, or the projection loop stops treating nil as AlwaysTrue, unknown partition fields start pruning again and the suite stays green, silently dropping matching rows. Please add a test that builds a spec with an unknown transform and asserts a predicate on that source column plans without pruning.
##########
transforms.go:
##########
@@ -230,6 +239,49 @@ func (VoidTransform) Project(string, BoundPredicate) (UnboundPredicate, error) {
return nil, nil
}
+// UnknownTransform is a placeholder for a partition or sort transform that
+// this implementation doesn't recognize. The v3 spec requires readers to load
+// tables that use unknown transforms and to ignore those fields when
+// filtering; writers must not commit a partition spec that uses one.
+type UnknownTransform struct {
+ name string
+}
+
+func (t UnknownTransform) MarshalText() ([]byte, error) {
+ return []byte(t.name), nil
+}
+
+func (t UnknownTransform) String() string { return t.name }
+
+// CanTransform assumes an unknown transform could apply to any type -- the
+// real applicability isn't known.
+func (UnknownTransform) CanTransform(Type) bool { return true }
+
+// ResultType is unknown, so report string, matching the Java reference.
+func (UnknownTransform) ResultType(Type) Type { return StringType{} }
+
+func (UnknownTransform) PreservesOrder() bool { return false }
+
+func (t UnknownTransform) Equals(other Transform) bool {
+ o, ok := other.(UnknownTransform)
+
+ return ok && t.name == o.name
+}
+
+// Apply can't be evaluated for an unknown transform.
+func (UnknownTransform) Apply(Optional[Literal]) Optional[Literal] {
+ return Optional[Literal]{}
+}
+
+func (UnknownTransform) ToHumanStr(any) string { return "null" }
Review Comment:
Returning the literal "null" here (and in ToHumanStrType on 278) is indistinguishable from the human rendering of an actual null value, so partition-summary/describe output for an unknown-transform field reads as "null" and an operator can't tell "unknown transform" from "null partition value." A distinct marker (e.g. the transform name) would avoid the ambiguity.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] feat(transforms): read unknown partition and sort transforms (v3) [iceberg-go]
Posted by "rambleraptor (via GitHub)" <gi...@apache.org>.
rambleraptor commented on PR #1463:
URL: https://github.com/apache/iceberg-go/pull/1463#issuecomment-5210268294
@zeroshade made those changes! I do want to note that I can't enable those v3 tests since they're broken for unrelated reasons. I opened up #1693 to make a quick fix to the test suite.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org