laskoviymishka commented on code in PR #1417:
URL: https://github.com/apache/iceberg-go/pull/1417#discussion_r3558198111
##########
table/metadata_schema_compatibility.go:
##########
@@ -133,11 +137,11 @@ func checkSchemaCompatibility(sc *iceberg.Schema,
formatVersion int) error {
})
}
default:
- if field.InitialDefault != nil && formatVersion <
defaultValuesMinFormatVersion {
+ if (field.InitialDefault != nil || field.WriteDefault
!= nil) && formatVersion < defaultValuesMinFormatVersion {
Review Comment:
One thing I want to make sure of: Java's `Schema.checkCompatibility` only
gates on `initialDefault()`, not `writeDefault()` — write-default is described
as forward-compatible (old writers just fail on the missing field). So this now
rejects a v2 schema that Java/PyIceberg would accept (initial-default null,
write-default set).
If `checkSchemaCompatibility` only runs on iceberg-go-initiated schema
construction/evolution, this is fine and arguably more correct. But if it's
also on the read path via `MetadataBuilder.AddSchema`, we could fail to load
externally-produced v2 metadata that carries a write-default. Do you know which
path this fires on? If it's read-side, I'd want to loosen this to match Java;
if it's construction-only, worth a one-line comment noting the intentional
divergence (the `// Java: Schema::checkCompatibility` note above becomes stale
otherwise).
##########
table/metadata_schema_compatibility.go:
##########
@@ -47,7 +47,12 @@ func (e ErrIncompatibleSchema) Error() string {
fmt.Fprintf(&problems, "\n- invalid
write default for %s: %s columns must default to null", f.ColName, f.Field.Type)
}
} else {
- fmt.Fprintf(&problems, "\n- invalid initial
default for %s: non-null default (%v) is not supported until v%d", f.ColName,
f.Field.InitialDefault, f.InvalidDefault.MinFormatVersion)
+ if f.Field.InitialDefault != nil {
+ fmt.Fprintf(&problems, "\n- invalid
initial default for %s: non-null default (%v) is not supported until v%d",
f.ColName, f.Field.InitialDefault, f.InvalidDefault.MinFormatVersion)
+ }
+ if f.Field.WriteDefault != nil {
+ fmt.Fprintf(&problems, "\n- invalid
write default for %s: non-null default (%v) is not supported until v%d",
f.ColName, f.Field.WriteDefault, f.InvalidDefault.MinFormatVersion)
Review Comment:
`InitialDefault`/`WriteDefault` are `any`, and every construction site here
(including these new tests and `TestGeometryGeographyNullOnlyDefaults`) sets
them to `&defaultValue`. `%v` on a pointer-to-scalar prints the address, so
this renders `non-null default (0xc0000140a0)` rather than `(7)` — which
defeats the point of the split, and the tests don't catch it since they only
`ErrorContains` the fixed substrings.
I'd dereference before formatting (a small helper or `reflect.Indirect`),
and while we're here add a `require.ErrorContains(t, err, "(7)")` to one
subtest so the value is actually pinned. wdyt?
##########
table/metadata_builder_internal_test.go:
##########
@@ -2232,6 +2232,81 @@ func TestGeometryGeographyNullOnlyDefaults(t *testing.T)
{
}
}
+func TestNonSpecialDefaultsRequireV3(t *testing.T) {
Review Comment:
Coverage here is at the `checkSchemaCompatibility` layer, but the bug this
fixes is reached through `UpdateColumn(path, ColumnUpdate{WriteDefault:
...}).Commit()` on a v2 table. Would be worth one test at that layer asserting
the error surfaces end-to-end through `AddSchema` — that's the path a real
caller hits, and there's currently no `WriteDefault` case in `TestUpdateColumn`
at all. Non-blocking, but it'd lock the actual regression.
--
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]
