You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Revanth14 (via GitHub)" <gi...@apache.org> on 2026/06/23 21:19:44 UTC
[PR] test: pin geo type JSON wire format [iceberg-go]
Revanth14 opened a new pull request, #1284:
URL: https://github.com/apache/iceberg-go/pull/1284
## Summary
Adds a cross-implementation schema JSON fixture to pin the Iceberg geo type-string wire format.
The fixture includes:
- `geometry(srid:3857)`
- `geography(srid:4269, karney)`
The new test loads the fixture, unmarshals and re-marshals it through Go, then compares the raw emitted JSON `type` string literals byte-for-byte instead of using `assert.JSONEq`.
## Why
Existing geo tests validated JSON equivalence, but did not pin the exact type strings that other Iceberg clients parse. This locks the current wire format before more geo-related work builds on it.
Fixes #1271.
## Testing
```sh
go test ./...
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] test: pin geo type JSON wire format [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1284:
URL: https://github.com/apache/iceberg-go/pull/1284#discussion_r3470772258
##########
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:
Good catch, thanks. Updated the fixture comment to cite the geo-related Iceberg commits instead of the unrelated main tip: `e1e0a740` for the original geo type support and `8de302bf` as the latest geo type-string touchpoint when the fixture was captured.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] test: pin geo type JSON wire format [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1284:
URL: https://github.com/apache/iceberg-go/pull/1284#discussion_r3464728116
##########
schema_test.go:
##########
@@ -19,6 +19,8 @@ package iceberg_test
Review Comment:
Thanks, good call. I added both single-arg geography cases to the Java fixture:
- `geography(srid:4269)` for custom CRS + default algorithm
- `geography(OGC:CRS84, karney)` for default CRS + custom algorithm
Also added byte-exact assertions for both emitted type strings.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] test: pin geo type JSON wire format [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1284:
URL: https://github.com/apache/iceberg-go/pull/1284
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] test: pin geo type JSON wire format [iceberg-go]
Posted by "tanmayrauth (via GitHub)" <gi...@apache.org>.
tanmayrauth commented on code in PR #1284:
URL: https://github.com/apache/iceberg-go/pull/1284#discussion_r3464634152
##########
schema_test.go:
##########
@@ -19,6 +19,8 @@ package iceberg_test
Review Comment:
The fixture covers the two-arg forms (geometry(srid), geography(srid, algo)) but not the single-arg geography branches — geography(crs) with the default algorithm, and geography(OGC:CRS84, algo) with the default CRS. Those are exactly the cases where Go's Type() drops the default, so they're the most valuable to pin cross-impl. Worth adding fields for them while you're capturing Java output? Totally optional.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] test: pin geo type JSON wire format [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] test: pin geo type JSON wire format [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1284:
URL: https://github.com/apache/iceberg-go/pull/1284#discussion_r3470786477
##########
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:
Done. Added plain `geom_default` and `geog_default` fields to the Java fixture and pinned their exact emitted type strings as `"geometry"` and `"geography"` in the test.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] test: pin geo type JSON wire format [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on PR #1284:
URL: https://github.com/apache/iceberg-go/pull/1284#issuecomment-4794334302
> Nice addition — this locks down the exact `geometry(...)`/`geography(...)` type-string literals that Java's `SchemaParser.toJson` emits, which is exactly the kind of thing that silently drifts when someone tweaks `String()` formatting. The four parameterized cases you picked are right.
>
> Two things make the fixture hard to trust as a reference, though, and since the whole point is to be the thing others build on, I'd fix them before merge. The cited Java commit `df6ea082...` isn't the geo-serialization commit — it's a metrics-reporter PR (#16664), so anyone tracing the provenance hits a dead end. And the comment frames this as a cross-implementation format, but PyIceberg emits _quoted_ CRS (`geometry('srid:3857')`) and won't parse the unquoted Java/Go form — so "cross-implementation" overstates what's pinned here.
>
> On the test itself: the top-level `require.Equal` is mostly carried by the four literal asserts below it, and the most common wire form — bare `geometry`/`geography` with default CRS — isn't covered. Both worth tightening.
>
> Fix the commit citation and the cross-impl note, close the default-case gap, and this is good to land.
Thanks for the detailed pass. I pushed the follow-up with the provenance fixed to the geo-related Iceberg commits, added the PyIceberg quoted-CRS note so the fixture is clearly scoped to Java's form, replaced the broad map equality with explicit per-field byte-string assertions plus length checks, and added bare `geometry` / `geography` default cases to the Java fixture.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] test: pin geo type JSON wire format [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1284:
URL: https://github.com/apache/iceberg-go/pull/1284#discussion_r3470780634
##########
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:
Agreed. I added a note that this fixture is scoped to Java's type-string form and that PyIceberg currently emits quoted CRS values, so this shouldn't be read as the canonical cross-client format.
The CRS quoting divergence is worth tracking separately. I'll open an issue for standardizing it across clients and link it here. Kept this PR focused on pinning the Java/Go fixture behavior.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] test: pin geo type JSON wire format [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1284:
URL: https://github.com/apache/iceberg-go/pull/1284#discussion_r3470783575
##########
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:
Done. Kept the existing non-empty guard and added an explicit `"null"` check so a raw JSON null type fails as missing instead of passing as non-empty bytes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] test: pin geo type JSON wire format [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1284:
URL: https://github.com/apache/iceberg-go/pull/1284#discussion_r3470782035
##########
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:
Done. I replaced the broad map equality with explicit expected type strings for each field, plus `require.Len` checks on both fixture and fresh maps so a silently dropped or extra field still fails clearly.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org