laskoviymishka commented on code in PR #1665:
URL: https://github.com/apache/iceberg-go/pull/1665#discussion_r3753493691
##########
table/sorting.go:
##########
@@ -195,21 +208,26 @@ func (s *SortField) UnmarshalJSON(b []byte) error {
return nil
}
-func validateSortSourceID(id int) error {
- if id <= 0 {
+// Source IDs are schema field IDs, and therefore positive, once an order is
+// bound to a schema. Unbound orders carry client placeholders that start at
zero.
+func validateSortSourceID(id int, binding orderBinding) error {
+ if binding == boundOrder && id <= 0 {
return fmt.Errorf("source ID must be positive: %d", id)
}
+ if id < 0 {
+ return fmt.Errorf("source ID must be non-negative: %d", id)
Review Comment:
Small consistency thing while we're here: the new non-negative branch
returns a bare `fmt.Errorf` without wrapping `ErrInvalidSortSourceID`, whereas
`validatePartitionSourceID` wraps its sentinel directly in both branches. It
works today because `SortField.unmarshal` double-wraps at the call site, but a
future caller of `validateSortSourceID` would silently lose the sentinel.
Same theme one function down at `CheckCompatibility` (sorting.go:437), which
interpolates the inner error with `%v` instead of `%w`. I'd wrap the sentinel
here and switch that `%v` to `%w` so `errors.Is` keeps working through both
paths.
##########
table/metadata_internal_test.go:
##########
@@ -743,6 +743,240 @@ 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 by it yields source-id 0. NewMetadata
+// must accept an unbound spec and order and remap every source ID by name,
+// matching Java's TableMetadata.newTableMetadata.
+//
+// The schema and specs below are the bodies Spark 3.5 with
+// iceberg-spark-runtime posts to /v1/namespaces/{ns}/tables for
+// CREATE TABLE t (ints INT, floats DOUBLE, strings STRING) USING iceberg
+// PARTITIONED BY (...).
+func TestNewMetadataFromOrdinalNumberedRequest(t *testing.T) {
Review Comment:
One coverage gap worth closing here. Every sub-case remaps a placeholder
that happens to line up with a schema field ID (source-id 0 against a schema
whose IDs start at 0), so a name-based remap and an ID-based remap would give
the same answer. That leaves the interesting cases untested: a placeholder with
no matching column in the new schema (say source-id 5 against a 3-column
schema), and a schema whose field IDs aren't the ordinals (5,7,9) paired with
ordinal source IDs.
I'd add a sub-case for the out-of-range placeholder asserting it errors
rather than silently producing invalid metadata. That also documents whether
the remap is intentionally constrained to the Spark ordinal wire format.
##########
partitions.go:
##########
@@ -190,6 +206,24 @@ type PartitionSpec struct {
sourceIdToFields map[int][]PartitionField
}
+// UnboundPartitionSpec decodes a partition spec that a client sent in a
+// create-table request, before it has been bound to a schema. Such a spec
+// carries the client's placeholder source IDs rather than schema field IDs,
and
+// those placeholders start at zero: Spark numbers the columns of a new table
+// from zero, so partitioning by the first column arrives as source-id 0.
+// Binding the embedded spec to a schema resolves the placeholders to field
IDs.
+//
+// Use PartitionSpec for specs read from table metadata, where source IDs are
+// bound field IDs and must be positive. Catalog implementations that serve the
+// REST create-table request should decode its partition spec into this type.
+type UnboundPartitionSpec struct {
Review Comment:
I think there's a subtle trap in exposing the embedded `PartitionSpec` here.
`BindToSchema` resolves each field through `AddPartitionFieldBySourceID`, which
looks the source up by field ID. On an unbound spec the source IDs are the
client's zero-based placeholders, so `unboundSpec.BindToSchema(schema, ...)`
hits `FindFieldByID(0)`, matches nothing, and errors out. The only correct path
today is `NewMetadata` into `reassignIDs`, which resolves by name.
That's easy to trip over since `BindToSchema` sits right there on the public
surface. I'd either override it on `UnboundPartitionSpec` to remap by name
(what Java's `UnboundPartitionSpec.bind` does), or add a doc line saying
binding goes through `NewMetadata` and `BindToSchema` isn't valid on the
unbound type. 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: [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]
