fix(partitions,sort): accept ordinal source IDs when decoding - #1665
fix(partitions,sort): accept ordinal source IDs when decoding#1665twuebi wants to merge 1 commit into
Conversation
Decoding rejected source-id 0 in partition specs and sort orders, but positivity is only an invariant for specs already bound to a schema. Create-table and commit requests carry unbound specs whose source IDs are the client's ordinal placeholders: Spark numbers the root struct's fields from zero, so partitioning or sorting by a table's first column sends source-id 0. Those requests failed with "source ID must be positive: 0". Reject only negative IDs during decoding and leave positivity to schema binding, which is where Java enforces it. TableMetadata.newTableMetadata assigns fresh IDs and remaps each spec field by looking the source name up in the request schema; reassignIDs already does the same, so ordinal IDs were handled correctly once past decoding. Non-resolvable IDs are still caught: AddPartitionFieldBySourceID and SortOrder.CheckCompatibility report source IDs missing from the schema. Regression tests cover a create-table request numbered from zero through NewMetadata and a metadata round trip, for both partition specs and sort orders. Signed-off-by: Tobias Pütz <tobias@min.io>
|
Thanks for catching this. I think your solution is correct. I checked the Java implementation and Spark’s schema conversion, and they confirm that source ID 0 is valid as an ordinal placeholder during table creation before the IDs are reassigned. |
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Thanks for covering create-table requests from clients that use temporary IDs. The reassignment in table/metadata.go:2814-2841 performs an exact temporary-ID to name to fresh-ID mapping, so real IDs take precedence and there is no ordinal ambiguity.
However, this change does not do what the title says: it does not add an ordinal fallback. It accepts temporary ID zero globally, including persisted default and historical partition specs and sort orders. table/metadata.go:2207-2237 only confirms that the default spec/order exists and compatibility-checks the default order; it does not structurally validate every persisted spec and order, so invalid zero IDs can survive metadata parsing. Suggested fix: keep request decoding permissive in a request-specific path, but require positive source IDs for every bound/persisted spec and order, preserving only the source-less void exception.
Duplicate temporary IDs remain a separate issue. Multiple zeros can make the name lookup used by table/metadata.go:2814-2841 nondeterministic. Suggested fix: reject duplicate temporary IDs before reassignment. Please add persisted v1/v2/v3 rejection tests covering default and historical specs/orders, shuffled IDs proving exact matching, duplicate-zero rejection, and round-trip checks that write paths emit only positive IDs.
| _, 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 { |
There was a problem hiding this comment.
This decoder is also used for persisted default and historical partition specs, so changing the global invariant admits source ID zero into stored metadata; checkPartitionSpecs does not bind or validate those source IDs. Suggested fix: accept temporary zero only in the unbound request path, then require positive IDs for all bound/persisted specs while retaining the existing source-less void tombstone exception.
| func validateSortSourceID(id int) error { | ||
| if id <= 0 { | ||
| return fmt.Errorf("source ID must be positive: %d", id) | ||
| if id < 0 { |
There was a problem hiding this comment.
This global relaxation also admits zero into historical sort orders. Only the default order is compatibility-checked during metadata validation, so a historical order with source ID zero can persist. Suggested fix: limit zero to the unbound request path and structurally validate every persisted sort order with positive source IDs.
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
Nice work tracking this down to ordinal source IDs from Spark's create-table path, and the reassign-by-name round trip in NewMetadata is the right shape, it lines up with Java's TableMetadata.newTableMetadata.
I'd hold this before merging though. My main concern is where the relaxation lives: PartitionField.UnmarshalJSON and validateSortSourceID are the same functions ParseMetadataBytes uses to parse persisted v1/v2/v3 metadata, and validateSortSourceID also backs the public NewSortOrder constructor. So relaxing <= 0 to < 0 there doesn't just accept ordinal placeholders from create-table requests, it also lets a persisted spec or a programmatically built sort order carry source-id 0 with no error. The create-table path doesn't need the parser relaxed at all, since reassignIDs already remaps ordinals by name.
Agreed with zeroshade that the permissiveness leaks past request decoding into persisted and historical specs/orders, and that duplicate zeros make the name-based reassignment nondeterministic. I'd defer to that thread rather than repeat it. One thing I'd add on top: even the default-only validation net has a hole. checkSortOrders only runs CheckCompatibility on the default order and checkPartitionSpecs only checks that the default spec appears in the list, so a historical non-default order/spec with source-id 0 isn't caught anywhere today.
A few things I'd want settled before merge:
- keep
UnmarshalJSONandvalidateSortSourceIDrejecting source-id<= 0; move the ordinal-0 allowance to a request-specific unbound decode path - keep
NewSortOrderrejecting source-id 0 (the flipped test is a semver-observable change to a published guarantee) - if the permissive parse path stays instead, validate every persisted spec/order, not just the default
- add the persisted v1/v2/v3 rejection tests zeroshade asked for, covering historical specs/orders, not just the happy path
- tighten the sort-order assertion in
TestNewMetadataFromOrdinalNumberedRequestso it can't pass on an empty order
Once those are addressed, happy to take another pass and approve.
| _, 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 { |
There was a problem hiding this 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?
| func validateSortSourceID(id int) error { | ||
| if id <= 0 { | ||
| return fmt.Errorf("source ID must be positive: %d", id) | ||
| if id < 0 { |
There was a problem hiding this 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.
| // 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) { |
There was a problem hiding this 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.
| 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() { |
There was a problem hiding this 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.
4 participants
fixes #1664
Decoding rejected source-id 0 in partition specs and sort orders, but positivity is only an invariant for specs already bound to a schema. Create-table and commit requests carry unbound specs whose source IDs are the client's ordinal placeholders: Spark numbers the root struct's fields from zero, so partitioning or sorting by a table's first column sends source-id 0. Those requests failed with "source ID must be positive: 0".
Reject only negative IDs during decoding and leave positivity to schema binding, which is where Java enforces it. TableMetadata.newTableMetadata assigns fresh IDs and remaps each spec field by looking the source name up in the request schema; reassignIDs already does the same, so ordinal IDs were handled correctly once past decoding. Non-resolvable IDs are still caught: AddPartitionFieldBySourceID and SortOrder.CheckCompatibility report source IDs missing from the schema.
Regression tests cover a create-table request numbered from zero through NewMetadata and a metadata round trip, for both partition specs and sort orders.