badalprasadsingh commented on code in PR #2512:
URL: https://github.com/apache/iceberg-go/pull/2512#discussion_r4534750578
##########
name_mapping.go:
##########
@@ -453,7 +453,7 @@ func (n *nameMapProjectVisitor) Field(field NestedField,
fieldPartner *MappedFie
}
return NestedField{
- ID: *fieldPartner.FieldID,
+ ID: fieldPartner.ID(),
Review Comment:
P1: scans of these files still fail, column prunging runs before this, and
`pruneParquetSchema.fieldID` in table/internal/parquet_files.go
(;line3112-line3120) rejects an entry without field-id as missing field_id.
better: return mapping.ID() there too, so the column is skipped:
```go
if mapping != nil {
return mapping.ID()
}
```
##########
table/arrow_utils_test.go:
##########
@@ -1072,6 +1072,24 @@ func TestVariantProjectionExclusion(t *testing.T) {
assert.Equal(t, "id", arrowSc.Field(0).Name)
}
+// Imported columns without Iceberg field IDs are legal in a name mapping.
+// Schema conversion must keep their unknown ID rather than failing.
+func TestArrowSchemaWithOptionalNameMappingIDs(t *testing.T) {
Review Comment:
P1: this test and the one in `name_mapping_test.go` call the schema helpers
directly, so they pass while a scan of the same file fails
better: add a scan test (it can replace this one) covering an id-less
top-level column and an id-less child in a mapped struct
##########
name_mapping.go:
##########
@@ -453,7 +453,7 @@ func (n *nameMapProjectVisitor) Field(field NestedField,
fieldPartner *MappedFie
}
return NestedField{
- ID: *fieldPartner.FieldID,
+ ID: fieldPartner.ID(),
Review Comment:
also, this pr answers the question in the `MappedField` comment, better:
replace it with the rule:
```go
// FieldID is nil for fields that exist only in imported files, as the
// spec allows. ApplyNameMapping gives such fields ID -1.
FieldID *int `json:"field-id,omitempty"`
```
--
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]
