laskoviymishka commented on code in PR #1519:
URL: https://github.com/apache/iceberg-go/pull/1519#discussion_r3645169286
##########
catalog/glue/schema.go:
##########
@@ -120,7 +120,7 @@ func icebergTypeToGlueType(typ iceberg.Type) string {
case iceberg.DecimalType:
return fmt.Sprintf("decimal(%d,%d)", t.Precision(), t.Scale())
case iceberg.FixedType:
- return fmt.Sprintf("binary(%d)", t.Len())
+ return "binary"
Review Comment:
This is the right call — Glue/Hive `BINARY` has no length parameter, so
plain `binary` is what both Java's `IcebergToGlueConverter` and PyIceberg emit.
I'd drop a one-line comment noting the length is intentionally discarded,
otherwise someone's liable to "fix" this back to `binary(%d)` and reintroduce
the bug.
```suggestion
// Glue/Hive BINARY has no length parameter; fixed[n] maps to
plain binary.
return "binary"
```
##########
catalog/glue/schema_test.go:
##########
@@ -101,7 +101,16 @@ func TestIcebergTypeToGlueType(t *testing.T) {
{
name: "fixed type",
input: iceberg.FixedTypeOf(16),
- expected: "binary(16)",
+ expected: "binary",
+ },
+ {
+ name: "nested fixed type",
+ input: &iceberg.ListType{
+ ElementID: 1,
+ Element: iceberg.FixedTypeOf(16),
+ ElementRequired: true,
+ },
+ expected: "array<binary>",
Review Comment:
Good to have a nested case here. One thought: `array<binary>` mostly
re-exercises the list-wrapping path that `TestNestedListType` already covers,
and `FixedType` is a leaf with a single return, so this isn't closing a real
branch.
A `struct<fingerprint:binary>` or `map<string,binary>` case would be more
representative of real schemas and document the rendered string across the
container positions. wdyt?
--
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]
