laskoviymishka commented on code in PR #1284:
URL: https://github.com/apache/iceberg-go/pull/1284#discussion_r3466832961
##########
schema_test.go:
##########
@@ -28,6 +30,26 @@ import (
"github.com/stretchr/testify/require"
)
+// geoSchemaJavaFixtureName is a schema JSON fixture emitted by Apache Iceberg
+// Java SchemaParser.toJson on apache/iceberg main at
+// df6ea0820030d217d5f645469419cdbb3f54a41f.
+//
+// Source schema:
+//
+// new Schema(17, ImmutableList.of(
+// Types.NestedField.required(1, "id", Types.LongType.get()),
+// Types.NestedField.optional(2, "geom",
Types.GeometryType.of("srid:3857")),
+// Types.NestedField.optional(3, "geog",
+// Types.GeographyType.of("srid:4269", EdgeAlgorithm.KARNEY)),
+// Types.NestedField.optional(4, "geog_default_algo",
+// Types.GeographyType.of("srid:4269")),
+// Types.NestedField.optional(5, "geog_default_crs",
+// Types.GeographyType.of("OGC:CRS84", EdgeAlgorithm.KARNEY))))
+//
+// Java and Go write object keys in different orders, so the fixture test pins
+// the raw JSON string literals for field types instead of whole-object bytes.
Review Comment:
This pins the Java/Go form, but I'd be careful calling it
cross-implementation. PyIceberg emits *quoted* CRS — `geometry('srid:3857')` —
and its `GEOMETRY_REGEX` requires the quotes, so it won't parse the unquoted
form this fixture pins, and conversely Go's regex would capture `'srid:3857'`
(quotes and all) from PyIceberg's output. That's a real bidirectional
divergence.
The filename already scopes this to Java, which is good. I'd just add a
sentence here noting PyIceberg differs so nobody reads this as the canonical
cross-client format. Might also be worth a tracking issue for standardizing the
CRS quoting across clients — wdyt?
##########
schema_test.go:
##########
@@ -28,6 +30,26 @@ import (
"github.com/stretchr/testify/require"
)
+// geoSchemaJavaFixtureName is a schema JSON fixture emitted by Apache Iceberg
+// Java SchemaParser.toJson on apache/iceberg main at
+// df6ea0820030d217d5f645469419cdbb3f54a41f.
Review Comment:
This commit isn't the one that defines geo serialization — `df6ea082` is
"Data: Add metrics reporter to generic scan builder (#16664)", which touches
`IcebergGenerics.java` and has nothing to do with `SchemaParser` or geo types.
It looks like it was just the tip of main when the fixture was generated.
The geo format was actually added in `e1e0a740` (#12346, "Add geometry and
geography types support") and last touched in `8de302bf`. I'd cite one of those
instead so a reader can actually verify the fixture against the source — e.g.
"geo type serialization is stable since e1e0a740 (#12346)". The values
themselves are right; it's only the citation that's misleading.
##########
testdata/geo-schema-java-main.json:
##########
@@ -0,0 +1 @@
+{"type":"struct","schema-id":17,"fields":[{"id":1,"name":"id","required":true,"type":"long"},{"id":2,"name":"geom","required":false,"type":"geometry(srid:3857)"},{"id":3,"name":"geog","required":false,"type":"geography(srid:4269,
karney)"},{"id":4,"name":"geog_default_algo","required":false,"type":"geography(srid:4269)"},{"id":5,"name":"geog_default_crs","required":false,"type":"geography(OGC:CRS84,
karney)"}]}
Review Comment:
All four geo cases here carry params, but the most common form — bare
`geometry`/`geography` with the default CRS omitted (Java's
`GeometryType.crs84().toString()` returns just `"geometry"`) — isn't pinned
anywhere. A regression turning bare `GeometryType{}.Type()` into `"geometry()"`
would slip right through.
I'd add two more fields (a plain `geometry` and a plain `geography`) plus
their asserts so the default path is locked down too.
##########
schema_test.go:
##########
@@ -1177,6 +1199,47 @@ func TestSchemaWithGeometryGeographyTypes(t *testing.T) {
assert.True(t, schema.Equals(&unmarshaledSchema))
}
+func TestSchemaGeoTypeStringWireFormatFixture(t *testing.T) {
+ fixtureBytes, err := os.ReadFile(filepath.Join("testdata",
geoSchemaJavaFixtureName))
+ require.NoError(t, err, "fixture missing")
+
+ var schema iceberg.Schema
+ require.NoError(t, json.Unmarshal(fixtureBytes, &schema))
+
+ freshBytes, err := json.Marshal(&schema)
+ require.NoError(t, err)
+
+ fixtureTypes := rawTopLevelFieldTypes(t, fixtureBytes)
+ freshTypes := rawTopLevelFieldTypes(t, freshBytes)
+ require.Equal(t, fixtureTypes, freshTypes)
+ assert.Equal(t, `"geometry(srid:3857)"`, freshTypes["geom"])
+ assert.Equal(t, `"geography(srid:4269, karney)"`, freshTypes["geog"])
+ assert.Equal(t, `"geography(srid:4269)"`,
freshTypes["geog_default_algo"])
+ assert.Equal(t, `"geography(OGC:CRS84, karney)"`,
freshTypes["geog_default_crs"])
+}
+
+func rawTopLevelFieldTypes(t *testing.T, data []byte) map[string]string {
+ t.Helper()
+
+ var schema struct {
+ Fields []struct {
+ Name string `json:"name"`
+ Type json.RawMessage `json:"type"`
+ } `json:"fields"`
+ }
+ require.NoError(t, json.Unmarshal(data, &schema))
+ require.NotEmpty(t, schema.Fields)
+
+ types := make(map[string]string, len(schema.Fields))
+ for _, field := range schema.Fields {
+ require.NotEmpty(t, field.Name)
+ require.NotEmptyf(t, field.Type, "field %q is missing type",
field.Name)
Review Comment:
`field.Type` is a `json.RawMessage` (`[]byte`), so a JSON `null` token is
four non-empty bytes and sails through `NotEmptyf` while being semantically
empty. If you want this guard to mean what it reads as, `require.NotEqualf(t,
"null", string(field.Type), ...)` alongside the length check would actually
catch a null type.
##########
schema_test.go:
##########
@@ -1177,6 +1199,47 @@ func TestSchemaWithGeometryGeographyTypes(t *testing.T) {
assert.True(t, schema.Equals(&unmarshaledSchema))
}
+func TestSchemaGeoTypeStringWireFormatFixture(t *testing.T) {
+ fixtureBytes, err := os.ReadFile(filepath.Join("testdata",
geoSchemaJavaFixtureName))
+ require.NoError(t, err, "fixture missing")
+
+ var schema iceberg.Schema
+ require.NoError(t, json.Unmarshal(fixtureBytes, &schema))
+
+ freshBytes, err := json.Marshal(&schema)
+ require.NoError(t, err)
+
+ fixtureTypes := rawTopLevelFieldTypes(t, fixtureBytes)
+ freshTypes := rawTopLevelFieldTypes(t, freshBytes)
+ require.Equal(t, fixtureTypes, freshTypes)
Review Comment:
This `require.Equal` is doing almost nothing once the four literal asserts
below it are in place — those already pin every `freshTypes` value against the
exact fixture string. In the realistic failure mode (Go drops the space and
emits `geography(srid:4269,karney)`) the literal asserts catch it directly and
point at the field, while the map compare just dumps a diff.
I'd either drop it and keep the literal asserts, or add a `require.Equal(t,
5, len(freshTypes))` so it earns its place by catching a silently-dropped field
on round-trip (which the literal asserts won't). The latter is probably the
more useful version.
--
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]
