laskoviymishka commented on code in PR #2372:
URL: https://github.com/apache/iceberg-go/pull/2372#discussion_r4427050210
##########
defaults.go:
##########
@@ -49,3 +53,108 @@ func CloneDefaultValue(value any) any {
return value
}
}
+
+// hexDefault returns the hex single-value form of a binary or fixed default
+// given as bytes. It reports false for any other type or value, including a
+// default that is already a string.
+func hexDefault(typ Type, value any) (string, bool) {
+ switch typ.(type) {
+ case BinaryType, FixedType:
+ switch value := value.(type) {
+ case []byte:
+ return internal.EncodeDefaultBytes(value), true
+ case BinaryLiteral:
+ return internal.EncodeDefaultBytes(value), true
+ case FixedLiteral:
+ return internal.EncodeDefaultBytes(value), true
+ }
+ }
+
+ return "", false
+}
+
+// normalizeDefaults stores binary and fixed defaults in their hex single-value
+// form, the same form metadata JSON decodes to, so a schema holds no byte
+// slices that callers could share or change. When something is rewritten it
+// copies the fields and nested types on the path to that default and reports
+// true; otherwise it returns fields itself. The input is never modified.
+func normalizeDefaults(fields []NestedField) ([]NestedField, bool) {
+ var out []NestedField
+ for i, field := range fields {
+ normalized, ok := normalizeFieldDefaults(field)
+ if !ok {
+ continue
+ }
+ if out == nil {
+ out = slices.Clone(fields)
+ }
+ out[i] = normalized
+ }
+ if out == nil {
+ return fields, false
+ }
+
+ return out, true
+}
+
+func normalizeFieldDefaults(field NestedField) (NestedField, bool) {
+ changed := false
+ if value, ok := hexDefault(field.Type, field.InitialDefault); ok {
+ field.InitialDefault, changed = value, true
+ }
+ if value, ok := hexDefault(field.Type, field.WriteDefault); ok {
+ field.WriteDefault, changed = value, true
+ }
+ if typ, ok := normalizeTypeDefaults(field.Type); ok {
+ field.Type, changed = typ, true
+ }
+
+ return field, changed
+}
+
+func normalizeTypeDefaults(typ Type) (Type, bool) {
Review Comment:
Could this reuse the `cloneFields` / `cloneType` walk instead of adding a
second one? `cloneType` already rebuilds struct, list and map nodes
([schema.go#L375-L422](https://github.com/apache/iceberg-go/blob/684acd75a95a6a30fe22968f6c5c4440f386f064/schema.go#L375-L422)).
As I read it, cloning in the constructor and calling `hexDefault` from
`cloneField` would reach every depth without touching the caller's types, and
would copy composite defaults too. The cost is a copy per constructor call,
which this version skips when there are no byte defaults. Which trade-off do
you prefer?
##########
website/src/releases.md:
##########
@@ -42,6 +42,7 @@ Per-release notes (highlights, breaking changes,
contributors) are published on
### Unreleased compatibility notes
+- **Schemas store binary and fixed defaults as hex strings.** `NewSchema`,
`NewSchemaWithIdentifiers` and `NewSchemaFromJSONFields` convert `[]byte`,
`BinaryLiteral` and `FixedLiteral` defaults on binary and fixed fields, at any
depth, to the hex string used in metadata JSON, so a field read back from a
schema holds a `string` such as `"0aff"`. Code that type-asserts these defaults
to `[]byte` must decode the string instead, for example with
`hex.DecodeString`. A schema built in process now equals the same schema read
from metadata. The fields and types passed in are not modified.
Review Comment:
Could the list name `UpdateSchema.AddColumn` / `Apply()` in place of
`NewSchemaFromJSONFields`? `AddColumn` results change type too
(`TestAddColumnStoresBinaryDefaultAsHex` pins it), while
`NewSchemaFromJSONFields` already held strings. Also, "equals the same schema
read from metadata" holds for lowercase hex only: Java writes uppercase
([SingleValueParser.java#L157-L176](https://github.com/apache/iceberg/blob/dfd7ef3b711cd1977b93f4e6f86cb4148712acd3/core/src/main/java/org/apache/iceberg/SingleValueParser.java#L157-L176))
and `Equals` is case-sensitive, so a short qualifier would help.
##########
defaults.go:
##########
@@ -49,3 +53,108 @@ func CloneDefaultValue(value any) any {
return value
}
}
+
+// hexDefault returns the hex single-value form of a binary or fixed default
+// given as bytes. It reports false for any other type or value, including a
+// default that is already a string.
+func hexDefault(typ Type, value any) (string, bool) {
+ switch typ.(type) {
+ case BinaryType, FixedType:
+ switch value := value.(type) {
+ case []byte:
+ return internal.EncodeDefaultBytes(value), true
+ case BinaryLiteral:
+ return internal.EncodeDefaultBytes(value), true
+ case FixedLiteral:
+ return internal.EncodeDefaultBytes(value), true
+ }
+ }
+
+ return "", false
+}
+
+// normalizeDefaults stores binary and fixed defaults in their hex single-value
+// form, the same form metadata JSON decodes to, so a schema holds no byte
Review Comment:
Could we narrow "holds no byte slices" to the binary or fixed field's own
default? `hexDefault` only converts that, so a `[]byte` inside a
`map[string]any` / `[]any` default stays bytes and still aliases the caller's
slice. `TestSchemaFieldGettersReturnDefensiveCopies` builds exactly that
through `NewSchema`
([schema_test.go#L335-L357](https://github.com/apache/iceberg-go/blob/684acd75a95a6a30fe22968f6c5c4440f386f064/schema_test.go#L335-L357)).
Same for "at any depth" in the `NewSchemaWithIdentifiers` doc.
--
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]
