badalprasadsingh commented on code in PR #2391:
URL: https://github.com/apache/iceberg-go/pull/2391#discussion_r4442243454
##########
table/arrow_utils.go:
##########
@@ -1547,75 +1555,67 @@ func checkArrowSchemaCompatWithProperties(requested
*iceberg.Schema, provided *a
}
func (sc *schemaCompatVisitor) isFieldCompat(lhs iceberg.NestedField) bool {
- rhs, ok := sc.provided.FindFieldByID(lhs.ID)
+ rhs, ok := sc.provided.FindFieldByIDRef(lhs.ID, internal.SchemaRef{})
if !ok {
- if lhs.Required && lhs.WriteDefault == nil &&
lhs.InitialDefault == nil {
- sc.errorData = append(sc.errorData,
- []string{"❌", lhs.String(), "missing"})
-
- return false
- }
- sc.errorData = append(sc.errorData,
- []string{"✅", lhs.String(), "missing"})
-
- return true
+ return sc.report(!lhs.Required || lhs.WriteDefault != nil ||
lhs.InitialDefault != nil, lhs, nil)
}
Review Comment:
The `report` condition here inverts the old one, but only the missing
optional case is tested. Required against optional has no test either. Could
you please add these to `TestCheckSchemaCompat`?
1. required field missing: error
2. required field missing with `WriteDefault` or `InitialDefault`: no error
3. required against optional: error
4. list element and map value mismatch: error
##########
table/arrow_utils.go:
##########
@@ -1926,16 +1926,20 @@ func fileToDataFile(ctx context.Context, fileIO
iceio.IO, filePath string, curre
defer rdr.Close()
arrSchema := must(rdr.Schema())
- if err := checkArrowSchemaCompatWithProperties(currentSchema,
arrSchema, false, props); err != nil {
+ fileSchema := must(ArrowSchemaToIcebergWithOptions(arrSchema,
ArrowToIcebergOptions{
+ NameMapping: currentSchema.NameMapping(),
+ TableSchema: currentSchema,
+ TableProperties: props,
+ }))
+ if err := checkSchemaCompat(currentSchema, fileSchema); err != nil {
panic(err)
}
+ // Converting a schema with field IDs ignores the name mapping, so for a
+ // file with IDs fileSchema maps its column paths to those IDs.
pathToIDSchema := currentSchema
if fileHasIDs := must(VisitArrowSchema(arrSchema, hasIDs{}));
fileHasIDs {
- pathToIDSchema =
must(ArrowSchemaToIcebergWithOptions(arrSchema, ArrowToIcebergOptions{
- TableSchema: currentSchema,
- TableProperties: props,
- }))
+ pathToIDSchema = fileSchema
Review Comment:
This walks the schema with `hasIDs` again, right after
`ArrowSchemaToIcebergWithOptions` did. `fileSchema` already maps file paths to
table IDs in both cases, from the file or from the name mapping, so the branch
can go and the call below becomes:
```go
must(format.PathToIDMapping(fileSchema)),
```
One error message changes: a null column over a struct or list column still
fails, with different text.
--
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]
