twuebi commented on code in PR #1665:
URL: https://github.com/apache/iceberg-go/pull/1665#discussion_r3796132776
##########
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:
The schema argument isn't the target schema, it's the schema the source IDs
refer to. For an unbound spec that's the request schema, whose field IDs are
the placeholders. I probed it: source-id 0 against a request schema with fields
0,1,2 resolves cleanly to ints, no error. So
`unboundSpec.BindToSchema(requestSchema, ...)` is the correct call and works
today.
Rust agrees, and so does Java. `UnboundPartitionSpec::bind`
(`crates/iceberg/src/spec/partition.rs:275`) resolves by source ID, and
`UnboundPartitionField` (`partition.rs:243-254`) carries `source_id`,
`field_id`, `name`, `transform`. Java is the same: `UnboundPartitionSpec.bind`
calls `schema.findType(field.sourceId)` (`UnboundPartitionSpec.java:61`) and
`builder.add(field.sourceId, ...)` (line 69), and its `UnboundPartitionField`
holds no source column name either. So the name isn't in the wire format at
all, `name` there is the partition field name, and a name-based override isn't
possible without also passing the request schema, which is what `reassignIDs`
already does at `table/metadata.go:2958`.
The real hazard was quieter than a failed lookup: `AssignFreshSchemaIDs`
numbers from 1 (`schema.go:1554-1562`) while placeholders start at 0, so
binding against the reassigned schema resolved every field off-by-one while
keeping the client's name, and nothing caught it, `addSpecFieldInternal`
checked only the transform's own parameters and duplicate partition names. Java
does catch it: `checkAndAddPartitionName` (`PartitionSpec.java:401-427`)
requires a partition name that is also a schema column to be sourced from that
column, and `TableMetadata.java:1189` uses that checked bind in the same
position as our `table/metadata.go:450`. I've now added the same rule in
`validatePartitionNameAgainstSchema` (`partitions.go:402`), called from
`addSpecFieldInternal`, which now takes the schema and so covers both
`BindToSchema` and the create-table path through `reassignIDs`. The shift above
now reports partition name floats matches schema column with field ID 2, but
the field is sourced fr
om 1. The only test that broke in the tree was the one asserting the silent
misbind, every catalog suite and the metadata replay tests pass untouched and
`TestPartitionNameMatchingSchemaColumn`(`partitions_test.go:1000`) covers the
rule directly.
--
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]
