You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/07/04 15:56:53 UTC
[PR] fix(cli): resolve partition spec fields by schema name [iceberg-go]
fallintoplace opened a new pull request, #1379:
URL: https://github.com/apache/iceberg-go/pull/1379
## Summary
- Resolve CLI --partition-spec entries against the parsed table schema instead of positional field IDs.
- Update table create path to pass schema into partition spec parser.
- Add regression tests for correct field-id mapping and unknown field rejection.
## Testing
- go test ./cmd/iceberg -run TestParsePartitionSpec -count=1
- go test ./cmd/iceberg -count=1
--
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] fix(cli): resolve partition spec fields by schema name [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1379:
URL: https://github.com/apache/iceberg-go/pull/1379#discussion_r3553758483
##########
cmd/iceberg/utils_test.go:
##########
@@ -242,3 +305,123 @@ func TestParseSortOrder(t *testing.T) {
})
}
}
+
+func TestParsePartitionSpecSendsFieldIDsInRestCreatePayload(t *testing.T) {
+ schema := iceberg.NewSchema(0,
+ iceberg.NestedField{
+ ID: 10,
+ Name: "customer_id",
+ Type: iceberg.PrimitiveTypes.String,
+ Required: false,
+ },
+ iceberg.NestedField{
+ ID: 20,
+ Name: "event_time",
+ Type: iceberg.PrimitiveTypes.TimestampNs,
+ Required: false,
+ },
+ )
+
+ spec, err := parsePartitionSpec("customer_id,event_time", schema)
+ require.NoError(t, err)
+ require.NotNil(t, spec)
+
+ var lastCreateBody map[string]any
+ createCalled := false
+
+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+ switch req.URL.Path {
+ case "/v1/config":
+ w.Header().Set("Content-Type", "application/json")
+ require.NoError(t, json.NewEncoder(w).Encode(map[string]any{
+ "defaults": map[string]any{},
+ "overrides": map[string]any{},
+ "endpoints": []string{},
+ }))
+
+ return
+ case "/v1/namespaces/db/tables":
+ require.Equal(t, http.MethodPost, req.Method)
+ createCalled = true
+
+ require.NoError(t, json.NewDecoder(req.Body).Decode(&lastCreateBody))
+
+ w.Header().Set("Content-Type", "application/json")
+ w.WriteHeader(http.StatusOK)
+ _, _ = w.Write([]byte(`{
+ "metadata-location": "s3://warehouse/db/tbl/metadata/v1.json",
+ "metadata": ` + tableMetadataJSON("") + `
+ }`))
+
+ return
+ default:
+ w.WriteHeader(http.StatusNotFound)
+ }
+ }))
+ defer srv.Close()
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", srv.URL)
+ require.NoError(t, err)
+
+ _, err = cat.CreateTable(context.Background(), table.Identifier{"db", "test_table"}, schema,
+ catalog.WithPartitionSpec(spec))
+ require.NoError(t, err)
+ require.True(t, createCalled, "create endpoint should be called")
+
+ rawPartitionSpec, ok := lastCreateBody["partition-spec"].(map[string]any)
+ require.True(t, ok)
+ fields, ok := rawPartitionSpec["fields"].([]any)
+ require.True(t, ok)
+ require.Len(t, fields, 2)
+
+ field0, ok := fields[0].(map[string]any)
+ require.True(t, ok)
+ field1, ok := fields[1].(map[string]any)
+ require.True(t, ok)
+ require.EqualValues(t, 1000, field0["field-id"])
+ require.EqualValues(t, 1001, field1["field-id"])
+}
+
+func tableMetadataJSON(tableUUID string) string {
Review Comment:
nit: this helper builds a schema that doesn't match the one exercised by the test above — harmless since only the partition spec matters for the assertion, but a matching schema would read less confusingly.
--
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] fix(cli): resolve partition spec fields by schema name [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #1379:
URL: https://github.com/apache/iceberg-go/pull/1379#discussion_r3523575868
##########
cmd/iceberg/utils.go:
##########
@@ -48,31 +49,33 @@ func parseProperties(propStr string) (iceberg.Properties, error) {
return props, nil
}
-func parsePartitionSpec(specStr string) (*iceberg.PartitionSpec, error) {
+func parsePartitionSpec(specStr string, schema *iceberg.Schema) (*iceberg.PartitionSpec, error) {
+ if schema == nil {
+ return nil, errors.New("schema is required for partition spec parsing")
+ }
+
if specStr == "" {
return iceberg.UnpartitionedSpec, nil
}
fields := strings.Split(specStr, ",")
- var partitionFields []iceberg.PartitionField
+ opts := make([]iceberg.PartitionOption, 0)
- for i, field := range fields {
+ for _, field := range fields {
field = strings.TrimSpace(field)
if field == "" {
continue
Review Comment:
Passing `nil` here leaves each generated `PartitionField.FieldID` at the internal unassigned value (`0`). `NewPartitionSpecOpts` validates/initializes the spec but does not assign partition field IDs, and REST create-table serializes this spec directly in the request payload. Please assign IDs while parsing (for example `iceberg.PartitionDataIDStart + len(opts)`) and add assertions that parsed fields receive `1000`, `1001`, ... plus a REST create-payload regression test.
--
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] fix(cli): resolve partition spec fields by schema name [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on code in PR #1379:
URL: https://github.com/apache/iceberg-go/pull/1379#discussion_r3523587439
##########
cmd/iceberg/utils_test.go:
##########
@@ -81,40 +81,90 @@ func TestParseProperties(t *testing.T) {
}
func TestParsePartitionSpec(t *testing.T) {
+ schema := iceberg.NewSchema(0,
+ iceberg.NestedField{
+ ID: 10,
+ Name: "customer_id",
+ Type: iceberg.PrimitiveTypes.String,
+ Required: false,
+ },
+ iceberg.NestedField{
+ ID: 20,
+ Name: "event_time",
+ Type: iceberg.PrimitiveTypes.TimestampNs,
+ Required: false,
+ },
+ )
+
tests := []struct {
- name string
- input string
- isErr bool
+ name string
+ input string
+ wantSourceIDs [][]int
+ errContains string
+ isErr bool
}{
{
name: "empty string",
- input: "",
+ input: "", // keeps backward compatibility for no partitioning
},
{
name: "single field",
- input: "field1",
+ input: "customer_id",
+ wantSourceIDs: [][]int{
+ {10},
+ },
},
{
name: "multiple fields",
- input: "field1,field2,field3",
+ input: "customer_id,event_time",
+ wantSourceIDs: [][]int{
+ {10},
+ {20},
+ },
},
{
name: "with spaces",
- input: " field1 , field2 , field3 ",
+ input: " customer_id , event_time ",
+ wantSourceIDs: [][]int{
+ {10},
+ {20},
+ },
+ },
+ {
+ name: "unknown field",
+ input: "no_such_field",
+ isErr: true,
+ errContains: "cannot find source column with name: no_such_field in schema",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
- got, err := parsePartitionSpec(tt.input)
+ got, err := parsePartitionSpec(tt.input, schema)
if (err != nil) != tt.isErr {
t.Errorf("parsePartitionSpec() error = %v, isErr %v", err, tt.isErr)
return
}
+ if tt.isErr {
+ if tt.errContains != "" {
+ require.ErrorContains(t, err, tt.errContains)
+ }
+
+ return
+ }
if !tt.isErr && got == nil {
t.Errorf("parsePartitionSpec() returned nil for valid input")
}
+ if len(tt.wantSourceIDs) == 0 {
+ require.Equal(t, iceberg.UnpartitionedSpec, got)
+
+ return
+ }
+ require.Equal(t, len(tt.wantSourceIDs), got.NumFields())
+ for i, wantIDs := range tt.wantSourceIDs {
+ require.Equal(t, wantIDs, got.Field(i).SourceIDs)
Review Comment:
`require.Equal(t, wantIDs, got.Field(i).SourceIDs)` — the tests only assert SourceIDs, never FieldID, which is why they pass with field-id: 0 still going out. Could you add a FieldID assertion (1000, 1001, …) and a REST create-payload regression test that marshals the spec and checks `field-id` isn't 0? That locks in the fix above.
##########
cmd/iceberg/utils.go:
##########
@@ -48,31 +49,33 @@ func parseProperties(propStr string) (iceberg.Properties, error) {
return props, nil
}
-func parsePartitionSpec(specStr string) (*iceberg.PartitionSpec, error) {
+func parsePartitionSpec(specStr string, schema *iceberg.Schema) (*iceberg.PartitionSpec, error) {
+ if schema == nil {
+ return nil, errors.New("schema is required for partition spec parsing")
+ }
+
if specStr == "" {
return iceberg.UnpartitionedSpec, nil
}
fields := strings.Split(specStr, ",")
- var partitionFields []iceberg.PartitionField
+ opts := make([]iceberg.PartitionOption, 0)
- for i, field := range fields {
+ for _, field := range fields {
field = strings.TrimSpace(field)
if field == "" {
continue
}
- partitionFields = append(partitionFields, iceberg.PartitionField{
- SourceIDs: []int{i + 1},
- FieldID: i + iceberg.PartitionDataIDStart,
- Name: field,
- Transform: iceberg.IdentityTransform{},
- })
+ opts = append(opts, iceberg.AddPartitionFieldByName(field, field, iceberg.IdentityTransform{}, schema, nil))
Review Comment:
`iceberg.AddPartitionFieldByName(field, field, iceberg.IdentityTransform{}, schema, nil)` — the nil field ID is the problem. `NewPartitionSpecOpts` only runs `initialize()`, never `assignPartitionFieldIds`, so every field keeps the
unassigned value (0). rest.go serializes `cfg.PartitionSpec` directly into the create-table payload, so the server gets `field-id: 0` for all of them — which is also a duplicate and invalid.
Assign a stable ID here instead, e.g.:
id := iceberg.PartitionDataIDStart + len(opts)
opts = append(opts, iceberg.AddPartitionFieldByName(field, field, iceberg.IdentityTransform{}, schema, &id))
Since empty entries are `continue`d before the append, `len(opts)` gives dense 1000, 1001, … even for input like `a,,b` (the old `i + PartitionDataIDStart` would've left a gap there — small bonus).
--
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] fix(cli): resolve partition spec fields by schema name [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1379:
URL: https://github.com/apache/iceberg-go/pull/1379
--
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