laskoviymishka commented on code in PR #1665:
URL: https://github.com/apache/iceberg-go/pull/1665#discussion_r3740296753
##########
table/sorting.go:
##########
@@ -191,9 +191,11 @@ func (s *SortField) UnmarshalJSON(b []byte) error {
return nil
}
+// Zero is valid: unbound sort orders carry client ordinal placeholder IDs that
+// start at 0. CheckCompatibility rejects IDs that don't resolve against a
schema.
func validateSortSourceID(id int) error {
- if id <= 0 {
- return fmt.Errorf("source ID must be positive: %d", id)
+ if id < 0 {
Review Comment:
Beyond the shared decode issue, relaxing `validateSortSourceID` also changes
the public `NewSortOrder` constructor, which builds bound orders. It's called
from there with `validateSourceIDs=true`, so a programmatic caller can now
construct a `SortOrder` with source-id 0 and get no error at build time, and
the flipped test (`require.Error` to `require.NoError` in
`TestNewSortOrderAcceptsZeroSourceID`) turns that into a semver-observable
removal of a published guarantee.
I'd keep `NewSortOrder` rejecting source-id 0 and let only the
request-decode path permit it. Happy to be wrong if there's a bound case where
0 is legitimate, but I can't think of one.
##########
table/metadata_internal_test.go:
##########
@@ -711,6 +711,83 @@ func
TestRejectStructurallyInvalidHistoricalPartitionSpec(t *testing.T) {
assert.ErrorContains(t, err, "spec ID must be non-negative")
}
+// A create-table request carries unbound placeholder IDs rather than final
+// field IDs. Spark numbers the root struct's fields by ordinal, so the first
+// column is field-id 0 and partitioning or sorting by it yields source-id 0.
+// NewMetadata must accept that and remap every source ID by name, matching
+// Java's TableMetadata.newTableMetadata.
+func TestNewMetadataFromOrdinalNumberedRequest(t *testing.T) {
Review Comment:
This covers the accept side nicely, and the round-trip assert is a good
touch. The reject side zeroshade asked for isn't here yet though, and there's a
real gap behind it: `checkSortOrders` only runs `CheckCompatibility` on the
default order and `checkPartitionSpecs` only checks that the default spec id
appears in the list. So once the parser stops rejecting source-id 0, a
persisted historical (non-default) spec or order carrying 0 has no validation
net anywhere.
I'd add persisted v1/v2/v3 rejection tests here covering historical
specs/orders, not just the default, plus a round-trip check that write paths
only ever emit positive IDs. If instead the permissive parse path stays,
`checkSortOrders` and `checkPartitionSpecs` would need to validate every
spec/order rather than just the default.
##########
partitions.go:
##########
@@ -162,10 +162,13 @@ func (p *PartitionField) UnmarshalJSON(b []byte) error {
} else {
p.SourceIDs = []int{aux.SourceID}
}
+ // Positivity only holds for specs already bound to a schema. Decoding
also
+ // covers unbound specs from create-table and commit requests, whose
source
+ // IDs are client ordinal placeholders starting at 0, remapped by name
at
+ // bind time.
for _, sourceID := range p.SourceIDs {
- _, isVoid := p.Transform.(VoidTransform)
- if sourceID <= 0 && (!isVoid || hasSourceID || hasSourceIDs) {
- return fmt.Errorf("%w: partition source ID must be
positive: %d", ErrInvalidPartitionSpec, sourceID)
+ if sourceID < 0 {
Review Comment:
I'd keep this check strict and move the ordinal-0 allowance to a
request-specific path. `PartitionField.UnmarshalJSON` is the same function
`ParseMetadataBytes` uses to parse persisted v1/v2/v3 metadata, so relaxing `<=
0` to `< 0` here lets an already-bound persisted spec carry source-id 0 with no
error, and `sourceIdToFields` ends up keyed on 0. It also quietly drops the old
`VoidTransform` carve-out, so `{"source-id":0,"transform":"void"}` used to be
rejected and now isn't.
The create-table path doesn't need the parser relaxed, `reassignIDs` already
remaps ordinals by name via `previousMapFn(f.SourceID())`, so I'd leave this
rejecting source-id `<= 0` and add an unbound decode path for requests. Same
shape on the sort side in `validateSortSourceID`. wdyt?
##########
table/metadata_internal_test.go:
##########
@@ -711,6 +711,83 @@ func
TestRejectStructurallyInvalidHistoricalPartitionSpec(t *testing.T) {
assert.ErrorContains(t, err, "spec ID must be non-negative")
}
+// A create-table request carries unbound placeholder IDs rather than final
+// field IDs. Spark numbers the root struct's fields by ordinal, so the first
+// column is field-id 0 and partitioning or sorting by it yields source-id 0.
+// NewMetadata must accept that and remap every source ID by name, matching
+// Java's TableMetadata.newTableMetadata.
+func TestNewMetadataFromOrdinalNumberedRequest(t *testing.T) {
+ const requestSchema = `{
+ "type": "struct", "schema-id": 0,
+ "fields": [
+ {"id": 0, "name": "my_ints", "required": false, "type":
"int"},
+ {"id": 1, "name": "my_floats", "required": false,
"type": "double"},
+ {"id": 2, "name": "strings", "required": false, "type":
"string"}
+ ]
+ }`
+
+ for _, tt := range []struct {
+ name string
+ spec string
+ wantSourceID int
+ wantName string
+ }{
+ {
+ name: "identity on first column",
+ spec:
`{"spec-id":0,"fields":[{"name":"my_ints","transform":"identity","source-id":0,"field-id":1000}]}`,
+ wantSourceID: 1,
+ wantName: "my_ints",
+ },
+ {
+ name: "bucket on first column",
+ spec:
`{"spec-id":0,"fields":[{"name":"my_ints_bucket","transform":"bucket[16]","source-id":0,"field-id":1000}]}`,
+ wantSourceID: 1,
+ wantName: "my_ints_bucket",
+ },
+ {
+ name: "identity on last column",
+ spec:
`{"spec-id":0,"fields":[{"name":"strings","transform":"identity","source-id":2,"field-id":1000}]}`,
+ wantSourceID: 3,
+ wantName: "strings",
+ },
+ } {
+ t.Run(tt.name, func(t *testing.T) {
+ var sc iceberg.Schema
+ require.NoError(t,
json.Unmarshal([]byte(requestSchema), &sc))
+
+ var spec iceberg.PartitionSpec
+ require.NoError(t, json.Unmarshal([]byte(tt.spec),
&spec))
+
+ var order SortOrder
+ require.NoError(t, json.Unmarshal([]byte(
+
`{"order-id":1,"fields":[{"transform":"identity","source-id":0,"direction":"asc","null-order":"nulls-first"}]}`,
+ ), &order))
+
+ meta, err := NewMetadata(&sc, &spec, order,
"s3://bucket/tbl", nil)
+ require.NoError(t, err)
+
+ gotSpec := meta.PartitionSpec()
+ require.Equal(t, 1, gotSpec.NumFields())
+
+ assert.Equal(t, tt.wantSourceID,
gotSpec.Field(0).SourceID())
+ assert.Equal(t, tt.wantName, gotSpec.Field(0).Name)
+
+ // The sort order references the first column, so it
remaps to 1.
+ for _, field := range meta.SortOrder().Fields() {
Review Comment:
This loop can pass without asserting anything. If `meta.SortOrder()` comes
back as `UnsortedSortOrder` (0 fields) because the sort remap silently failed,
the range body never runs and the assertion is vacuous, so the exact regression
this test is meant to catch would slip through green.
I'd add `assert.Equal(t, 1, meta.SortOrder().Len())` right before the loop,
mirroring the `require.Equal(t, 1, gotSpec.NumFields())` guard you already have
on the partition side.
--
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]
