You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "happydave1 (via GitHub)" <gi...@apache.org> on 2026/05/05 03:42:38 UTC
[PR] Feat/geo type [iceberg-go]
happydave1 opened a new pull request, #984:
URL: https://github.com/apache/iceberg-go/pull/984
*TESTING*
--
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] feat(table): Adding geometry and geography type + schema plumbing [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #984:
URL: https://github.com/apache/iceberg-go/pull/984
--
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] feat(table): Adding geometry and geography type + schema plumbing [iceberg-go]
Posted by "happydave1 (via GitHub)" <gi...@apache.org>.
happydave1 commented on PR #984:
URL: https://github.com/apache/iceberg-go/pull/984#issuecomment-4482094712
Thanks for the clear and precise review last round @laskoviymishka. Good catches on all the issues presented.
I have added `Geography` and `Geometry` cases to all 4 locations mentioned in the dialogue around the `createBoundRef` issue.
I have also fixed the canonicalization issue for both `GeographyTypeOf` and `GeometryTypeOf` - this was a good catch since we handle `algorithm` with a `strings.TrimSpace` as well in `toGeoEdgeInterpolation`.
Finally, I fixed the Substrait one-liner by making `VisitGeometry` a multi-line function. This will make it consistent with `VisitGeography`.
Please let me know what you think, any feedback would be greatly appreciated. Thanks!
--
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] feat(table): Adding geometry and geography type + schema plumbing [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #984:
URL: https://github.com/apache/iceberg-go/pull/984#issuecomment-4511244303
I believe if you rebase it should solve the integration test CI issue.
--
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] feat(table): Adding geometry and geography type + schema plumbing [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #984:
URL: https://github.com/apache/iceberg-go/pull/984#discussion_r3218034750
##########
types.go:
##########
@@ -777,6 +822,144 @@ func (UnknownType) primitive() {}
func (UnknownType) Type() string { return "unknown" }
func (UnknownType) String() string { return "unknown" }
+const defaultGeoCRS = "OGC:CRS84"
+
+type GeometryType struct {
+ crs string
+}
+
+func GeometryTypeOf(crs string) (GeometryType, error) {
+ if crs == "" {
+ return GeometryType{}, fmt.Errorf("%w: invalid CRS: (empty string)", ErrInvalidTypeString)
+ }
+ if strings.EqualFold(crs, defaultGeoCRS) {
+ return GeometryType{}, nil
+ }
+
+ return GeometryType{crs: crs}, nil
+}
+
+func (g GeometryType) CRS() string {
+ if g.crs == "" {
+ return defaultGeoCRS
+ }
+
+ return g.crs
+}
+
+func (g GeometryType) Equals(other Type) bool {
+ rhs, ok := other.(GeometryType)
+ if !ok {
+ return false
+ }
+
+ return g.crs == rhs.crs
+}
+
+func (GeometryType) primitive() {}
+func (g GeometryType) Type() string {
+ if g.crs == "" {
+ return "geometry"
+ }
+
+ return fmt.Sprintf("geometry(%s)", g.crs)
+}
+
+func (g GeometryType) String() string {
+ return g.Type()
+}
+
+const defaultGeographyAlgorithm = string(geoarrow.EdgeSpherical)
+
+func toGeoArrowEdgeInterpolation(s string) (geoarrow.EdgeInterpolation, error) {
+ algo := geoarrow.EdgeInterpolation(strings.ToLower(strings.TrimSpace(s)))
+ switch algo {
+ case geoarrow.EdgeSpherical, geoarrow.EdgeVincenty,
+ geoarrow.EdgeThomas, geoarrow.EdgeAndoyer, geoarrow.EdgeKarney,
+ geoarrow.EdgePlanar:
Review Comment:
`EdgePlanar` isn't in the Iceberg V3 spec's algorithm set (`spherical|vincenty|thomas|andoyer|karney`). With it in the allow-list a Go writer can emit `geography(crs, planar)` that Java and PyIceberg won't accept — cross-client wire incompatibility baked in at the type-construction boundary. Drop `geoarrow.EdgePlanar` from the case to stay within spec.
##########
types_test.go:
##########
@@ -479,3 +479,198 @@ func TestPropUInt(t *testing.T) {
assert.Equal(t, uint(77), iceberg.PropUInt(props, "garbage", 77), "falls back on parse error")
assert.Equal(t, uint(5), iceberg.PropUInt(props, "missing", 5), "falls back on missing key")
}
+
+func TestGeometryType(t *testing.T) {
+ t.Run("default CRS", func(t *testing.T) {
+ geom := iceberg.GeometryType{}
+ assert.Equal(t, "OGC:CRS84", geom.CRS())
+ assert.Equal(t, "geometry", geom.String())
+ assert.True(t, geom.Equals(iceberg.GeometryType{}))
+ })
+
+ t.Run("custom CRS", func(t *testing.T) {
+ geom, err := iceberg.GeometryTypeOf("srid:3857")
+ require.NoError(t, err)
+ assert.Equal(t, "srid:3857", geom.CRS())
+ assert.Equal(t, "geometry(srid:3857)", geom.String())
+ })
+
+ t.Run("CRS normalization", func(t *testing.T) {
+ geom1, err := iceberg.GeometryTypeOf("OGC:CRS84")
+ require.NoError(t, err)
+ geom2 := iceberg.GeometryType{}
+ assert.True(t, geom1.Equals(geom2))
+ assert.Equal(t, "geometry", geom1.String())
+ })
+
+ t.Run("empty CRS error", func(t *testing.T) {
+ _, err := iceberg.GeometryTypeOf("")
+ assert.ErrorContains(t, err, "invalid CRS: (empty string)")
+ })
+
+ t.Run("JSON parsing - default", func(t *testing.T) {
+ data := `{"id": 1, "name": "location", "type": "geometry", "required": false}`
+ var n iceberg.NestedField
+ require.NoError(t, json.Unmarshal([]byte(data), &n))
+ geom, ok := n.Type.(iceberg.GeometryType)
+ require.True(t, ok)
+ assert.Equal(t, "OGC:CRS84", geom.CRS())
+
+ out, err := json.Marshal(n)
+ require.NoError(t, err)
+ assert.JSONEq(t, data, string(out))
+ })
+
+ t.Run("JSON parsing - custom CRS", func(t *testing.T) {
+ data := `{"id": 1, "name": "location", "type": "geometry(srid:4326)", "required": false}`
+ var n iceberg.NestedField
+ require.NoError(t, json.Unmarshal([]byte(data), &n))
+ geom, ok := n.Type.(iceberg.GeometryType)
+ require.True(t, ok)
+ assert.Equal(t, "srid:4326", geom.CRS())
+
+ out, err := json.Marshal(n)
+ require.NoError(t, err)
+ assert.JSONEq(t, data, string(out))
+ })
+
+ t.Run("JSON parsing - case insensitive", func(t *testing.T) {
+ data := `{"id": 1, "name": "location", "type": "Geometry(SRID:4326)", "required": false}`
+ var n iceberg.NestedField
+ require.NoError(t, json.Unmarshal([]byte(data), &n))
+ geom, ok := n.Type.(iceberg.GeometryType)
+ require.True(t, ok)
+ assert.Equal(t, "SRID:4326", geom.CRS())
+ })
+
+ t.Run("JSON parsing - whitespace tolerance", func(t *testing.T) {
+ data := `{"id": 1, "name": "location", "type": "geometry( srid:4326 )", "required": false}`
+ var n iceberg.NestedField
+ require.NoError(t, json.Unmarshal([]byte(data), &n))
+ geom, ok := n.Type.(iceberg.GeometryType)
+ require.True(t, ok)
+ assert.Equal(t, "srid:4326", geom.CRS())
+ })
+
+ t.Run("rejects extra argument", func(t *testing.T) {
+ data := `{"id": 1, "name": "loc", "type": "geometry(srid:4326, extra)", "required": false}`
+ var n iceberg.NestedField
+ err := json.Unmarshal([]byte(data), &n)
+ assert.ErrorIs(t, err, iceberg.ErrInvalidTypeString)
+ })
+}
+
+func TestGeographyType(t *testing.T) {
+ t.Run("default CRS and algorithm", func(t *testing.T) {
+ geog := iceberg.GeographyType{}
+ assert.Equal(t, "OGC:CRS84", geog.CRS())
+ assert.Equal(t, "spherical", geog.Algorithm())
+ assert.Equal(t, "geography", geog.String())
+ assert.True(t, geog.Equals(iceberg.GeographyType{}))
+ })
+
+ t.Run("custom CRS only", func(t *testing.T) {
+ geog, err := iceberg.GeographyTypeOf("srid:4269", "spherical")
+ require.NoError(t, err)
+ assert.Equal(t, "srid:4269", geog.CRS())
+ assert.Equal(t, "spherical", geog.Algorithm())
+ assert.Equal(t, "geography(srid:4269)", geog.String())
+ })
+
+ t.Run("default CRS with algorithm", func(t *testing.T) {
Review Comment:
Won't block this PR on it, but I'd add a one-liner here asserting `GeographyTypeOf("OGC:CRS84", "spherical").Equals(GeographyType{})` is true. The canonicalization reads correct, but it's unproven by the current tests, and that's the contract that prevents fingerprint drift from Java.
##########
table/metadata_builder_internal_test.go:
##########
@@ -1840,6 +1844,110 @@ func TestUnknownTypeValidation(t *testing.T) {
})
}
+func TestGeometryGeographyNullOnlyDefaults(t *testing.T) {
+ testTypes := []struct {
+ name string
+ typ iceberg.Type
+ }{
+ {"geometry", iceberg.GeometryType{}},
+ {"geography", iceberg.GeographyType{}},
+ }
+
+ for _, tt := range testTypes {
+ t.Run(tt.name+" with non-null initial default", func(t *testing.T) {
+ defaultValue := "POINT(0 0)"
+ sc := iceberg.NewSchema(0,
+ iceberg.NestedField{
+ Type: tt.typ,
+ ID: 1,
+ Name: "location",
+ Required: false,
+ InitialDefault: &defaultValue,
+ },
+ )
+
+ err := checkSchemaCompatibility(sc, 3)
+ require.Error(t, err)
+ require.ErrorContains(t, err, "columns must default to null")
+ require.ErrorIs(t, err, iceberg.ErrInvalidSchema)
+ })
+
+ t.Run(tt.name+" with non-null write default", func(t *testing.T) {
+ defaultValue := "POINT(0 0)"
+ sc := iceberg.NewSchema(0,
+ iceberg.NestedField{
+ Type: tt.typ,
+ ID: 1,
+ Name: "location",
+ Required: false,
+ WriteDefault: &defaultValue,
+ },
+ )
+
+ err := checkSchemaCompatibility(sc, 3)
+ require.Error(t, err)
+ require.ErrorContains(t, err, "columns must default to null")
+ require.ErrorIs(t, err, iceberg.ErrInvalidSchema)
+ })
+
+ t.Run(tt.name+" with null defaults", func(t *testing.T) {
+ sc := iceberg.NewSchema(0,
+ iceberg.NestedField{
+ Type: tt.typ,
+ ID: 1,
+ Name: "location",
+ Required: false,
+ },
+ )
+
+ err := checkSchemaCompatibility(sc, 3)
+ require.NoError(t, err)
+ })
+
+ t.Run(tt.name+" in v2 with non-null initial default", func(t *testing.T) {
Review Comment:
Nice-to-have, not blocking. This asserts `"is not supported until v3"` but the v2 path fires both the type-unsupported error and the must-default-to-null error — `ErrorContains` happens to match the first. The new exact-count test below covers the duplicate-output concern, so the structural risk is gone. Splitting this into two subtests (v2 type-unsupported, v3 must-default-to-null) would still be cleaner, but the current shape isn't wrong.
##########
types_test.go:
##########
@@ -479,3 +479,198 @@ func TestPropUInt(t *testing.T) {
assert.Equal(t, uint(77), iceberg.PropUInt(props, "garbage", 77), "falls back on parse error")
assert.Equal(t, uint(5), iceberg.PropUInt(props, "missing", 5), "falls back on missing key")
}
+
+func TestGeometryType(t *testing.T) {
+ t.Run("default CRS", func(t *testing.T) {
+ geom := iceberg.GeometryType{}
+ assert.Equal(t, "OGC:CRS84", geom.CRS())
+ assert.Equal(t, "geometry", geom.String())
+ assert.True(t, geom.Equals(iceberg.GeometryType{}))
+ })
+
+ t.Run("custom CRS", func(t *testing.T) {
+ geom, err := iceberg.GeometryTypeOf("srid:3857")
+ require.NoError(t, err)
+ assert.Equal(t, "srid:3857", geom.CRS())
+ assert.Equal(t, "geometry(srid:3857)", geom.String())
+ })
+
+ t.Run("CRS normalization", func(t *testing.T) {
+ geom1, err := iceberg.GeometryTypeOf("OGC:CRS84")
+ require.NoError(t, err)
+ geom2 := iceberg.GeometryType{}
+ assert.True(t, geom1.Equals(geom2))
+ assert.Equal(t, "geometry", geom1.String())
+ })
+
+ t.Run("empty CRS error", func(t *testing.T) {
+ _, err := iceberg.GeometryTypeOf("")
+ assert.ErrorContains(t, err, "invalid CRS: (empty string)")
+ })
+
+ t.Run("JSON parsing - default", func(t *testing.T) {
+ data := `{"id": 1, "name": "location", "type": "geometry", "required": false}`
+ var n iceberg.NestedField
+ require.NoError(t, json.Unmarshal([]byte(data), &n))
+ geom, ok := n.Type.(iceberg.GeometryType)
+ require.True(t, ok)
+ assert.Equal(t, "OGC:CRS84", geom.CRS())
+
+ out, err := json.Marshal(n)
+ require.NoError(t, err)
+ assert.JSONEq(t, data, string(out))
+ })
+
+ t.Run("JSON parsing - custom CRS", func(t *testing.T) {
+ data := `{"id": 1, "name": "location", "type": "geometry(srid:4326)", "required": false}`
+ var n iceberg.NestedField
+ require.NoError(t, json.Unmarshal([]byte(data), &n))
+ geom, ok := n.Type.(iceberg.GeometryType)
+ require.True(t, ok)
+ assert.Equal(t, "srid:4326", geom.CRS())
+
+ out, err := json.Marshal(n)
+ require.NoError(t, err)
+ assert.JSONEq(t, data, string(out))
+ })
+
+ t.Run("JSON parsing - case insensitive", func(t *testing.T) {
+ data := `{"id": 1, "name": "location", "type": "Geometry(SRID:4326)", "required": false}`
+ var n iceberg.NestedField
+ require.NoError(t, json.Unmarshal([]byte(data), &n))
+ geom, ok := n.Type.(iceberg.GeometryType)
+ require.True(t, ok)
+ assert.Equal(t, "SRID:4326", geom.CRS())
+ })
+
+ t.Run("JSON parsing - whitespace tolerance", func(t *testing.T) {
+ data := `{"id": 1, "name": "location", "type": "geometry( srid:4326 )", "required": false}`
+ var n iceberg.NestedField
+ require.NoError(t, json.Unmarshal([]byte(data), &n))
+ geom, ok := n.Type.(iceberg.GeometryType)
+ require.True(t, ok)
+ assert.Equal(t, "srid:4326", geom.CRS())
+ })
+
+ t.Run("rejects extra argument", func(t *testing.T) {
+ data := `{"id": 1, "name": "loc", "type": "geometry(srid:4326, extra)", "required": false}`
+ var n iceberg.NestedField
+ err := json.Unmarshal([]byte(data), &n)
+ assert.ErrorIs(t, err, iceberg.ErrInvalidTypeString)
+ })
+}
+
+func TestGeographyType(t *testing.T) {
Review Comment:
Won't block this PR, but worth a follow-up: a Java-emitted JSON fixture for a geo schema with non-default CRS + algorithm, parsed here and re-marshaled byte-compared, would lock down the wire compatibility contract before #991 builds on it. The internal Go round-trips we have today guard against regressions in our own code but not against drift from Java's canonical form.
##########
table/substrait/substrait.go:
##########
@@ -169,6 +169,10 @@ func (convertToSubstrait) VisitUnknown() types.Type {
// Returning nil indicates this type cannot be converted to Substrait
return nil
}
+func (convertToSubstrait) VisitGeometry(iceberg.GeometryType) types.Type { return &types.BinaryType{} }
Review Comment:
Pure style nit, won't block on it: `VisitGeometry` is one-line, `VisitGeography` (line 173) is multi-line. Surrounding methods are uniformly one-liners.
##########
go.mod:
##########
@@ -142,6 +142,7 @@ require (
github.com/felixge/httpsnoop v1.0.4 // indirect
github.com/fsnotify/fsevents v0.2.0 // indirect
github.com/fvbommel/sortorder v1.1.0 // indirect
+ github.com/geoarrow/geoarrow-go v0.0.0-20260403143023-f54751c3e3a1 // indirect
Review Comment:
Pseudo-version pin (`v0.0.0-20260403143023-f54751c3e3a1`) is fragile — an upstream rebase or force-push would break our build.
Worth pinning to a tagged release once `geoarrow-go` cuts one.
I'm ok with that as is, @zeroshade is this fine for you?
--
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] feat(table): Adding geometry and geography type + schema plumbing [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #984:
URL: https://github.com/apache/iceberg-go/pull/984#discussion_r3228323002
##########
go.mod:
##########
@@ -142,6 +142,7 @@ require (
github.com/felixge/httpsnoop v1.0.4 // indirect
github.com/fsnotify/fsevents v0.2.0 // indirect
github.com/fvbommel/sortorder v1.1.0 // indirect
+ github.com/geoarrow/geoarrow-go v0.0.0-20260403143023-f54751c3e3a1 // indirect
Review Comment:
Why is it an indirect dep? Shouldn't it be a direct dependency?
That said, the pseudo-version is fine for now in my opinion. I'll bug the geoarrow-go repo and see if I can get them to cut a release soon. We should definitely track an issue to update to a release version when it gets cut.
--
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] feat(table): Adding geometry and geography type + schema plumbing [iceberg-go]
Posted by "happydave1 (via GitHub)" <gi...@apache.org>.
happydave1 commented on code in PR #984:
URL: https://github.com/apache/iceberg-go/pull/984#discussion_r3220678758
##########
table/substrait/substrait.go:
##########
@@ -169,6 +169,10 @@ func (convertToSubstrait) VisitUnknown() types.Type {
// Returning nil indicates this type cannot be converted to Substrait
return nil
}
+func (convertToSubstrait) VisitGeometry(iceberg.GeometryType) types.Type { return &types.BinaryType{} }
Review Comment:
I agree with this style nit but `gofmt` won't let me change `VisitGeography` into a one liner.
Is there a workaround?
--
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] feat(table): Adding geometry and geography type + schema plumbing [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #984:
URL: https://github.com/apache/iceberg-go/pull/984#discussion_r3203203273
##########
types.go:
##########
@@ -777,6 +824,145 @@ func (UnknownType) primitive() {}
func (UnknownType) Type() string { return "unknown" }
func (UnknownType) String() string { return "unknown" }
+type EdgeAlgorithm string
+
+const (
+ EdgeAlgorithmSpherical EdgeAlgorithm = "spherical"
+ EdgeAlgorithmVincenty EdgeAlgorithm = "vincenty"
+ EdgeAlgorithmThomas EdgeAlgorithm = "thomas"
+ EdgeAlgorithmAndoyer EdgeAlgorithm = "andoyer"
+ EdgeAlgorithmKarney EdgeAlgorithm = "karney"
+)
Review Comment:
these exist in geoarrow/geoarrow-go, we should use the constants from there instead of defining them here. See https://github.com/geoarrow/geoarrow-go/blob/main/metadata.go
##########
exprs.go:
##########
@@ -450,6 +450,8 @@ func createBoundRef(field NestedField, acc accessor) BoundReference {
return &boundRef[Decimal]{field: field, acc: acc}
case UUIDType:
return &boundRef[uuid.UUID]{field: field, acc: acc}
+ case GeographyType, GeometryType:
+ return &boundRef[[]byte]{field: field, acc: acc}
Review Comment:
should this be https://github.com/geoarrow/geoarrow-go/blob/main/wkb.go#L20 instead of `[]byte`?
##########
table/arrow_utils.go:
##########
@@ -630,6 +630,24 @@ func (c convertToArrow) VisitUnknown() arrow.Field {
}
}
+func (c convertToArrow) VisitGeometry(iceberg.GeometryType) arrow.Field {
+ // Passthrough binary for now, adding geoarrow-go support later
+ if c.useLargeTypes {
+ return arrow.Field{Type: arrow.BinaryTypes.LargeBinary}
+ }
+
+ return arrow.Field{Type: arrow.BinaryTypes.Binary}
+}
+
+func (c convertToArrow) VisitGeography(iceberg.GeographyType) arrow.Field {
+ // Passthrough binary for now, adding geoarrow-go support later
+ if c.useLargeTypes {
+ return arrow.Field{Type: arrow.BinaryTypes.LargeBinary}
+ }
+
+ return arrow.Field{Type: arrow.BinaryTypes.Binary}
Review Comment:
same point as above. why not just use https://github.com/geoarrow/geoarrow-go/blob/main/wkb.go#L20 right now instead of the passthrough?
##########
table/arrow_utils.go:
##########
@@ -630,6 +630,24 @@ func (c convertToArrow) VisitUnknown() arrow.Field {
}
}
+func (c convertToArrow) VisitGeometry(iceberg.GeometryType) arrow.Field {
Review Comment:
I don't see why we don't just use https://github.com/geoarrow/geoarrow-go/blob/main/wkb.go#L15 off the bat right now instead of using binary/large binary as the intermediate.
https://github.com/geoarrow/geoarrow-go/blob/main/wkb.go#L84 can be used for the large type case
--
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] feat(table): Adding geometry and geography type + schema plumbing [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #984:
URL: https://github.com/apache/iceberg-go/pull/984#discussion_r3200057360
##########
schema.go:
##########
@@ -567,6 +567,8 @@ type SchemaVisitorPerPrimitiveType[T any] interface {
VisitBinary() T
VisitUUID() T
VisitUnknown() T
+ VisitGeometry(GeometryType) T
Review Comment:
These methods get added to the interface but `visitField` (~671–720) doesn't dispatch them. `GeometryType`/`GeographyType` are `PrimitiveType`s, so they fall through to `visitor.Primitive(...)`, which both visitors panic from. `SchemaToArrowSchema` on any geo column will panic at runtime today. I'd add the two cases in the dispatcher and a regression test that runs `iceberg.Visit(geoSchema, convertToArrow{})`.
##########
types.go:
##########
@@ -777,6 +824,145 @@ func (UnknownType) primitive() {}
func (UnknownType) Type() string { return "unknown" }
func (UnknownType) String() string { return "unknown" }
+type EdgeAlgorithm string
+
+const (
+ EdgeAlgorithmSpherical EdgeAlgorithm = "spherical"
+ EdgeAlgorithmVincenty EdgeAlgorithm = "vincenty"
+ EdgeAlgorithmThomas EdgeAlgorithm = "thomas"
+ EdgeAlgorithmAndoyer EdgeAlgorithm = "andoyer"
+ EdgeAlgorithmKarney EdgeAlgorithm = "karney"
+)
+
+func ParseEdgeAlgorithm(s string) (EdgeAlgorithm, error) {
+ switch strings.ToLower(s) {
+ case "spherical":
+ return EdgeAlgorithmSpherical, nil
+ case "vincenty":
+ return EdgeAlgorithmVincenty, nil
+ case "thomas":
+ return EdgeAlgorithmThomas, nil
+ case "andoyer":
+ return EdgeAlgorithmAndoyer, nil
+ case "karney":
+ return EdgeAlgorithmKarney, nil
+ default:
+ return "", fmt.Errorf("invalid edge interpolation algorithm: %s", s)
+ }
+}
+
+func (e EdgeAlgorithm) String() string {
+ return string(e)
+}
+
+const defaultGeoCRS = "OGC:CRS84"
+
+type GeometryType struct {
+ crs string
+}
+
+func GeometryTypeOf(crs string) (GeometryType, error) {
+ if crs == "" {
+ return GeometryType{}, errors.New("invalid CRS: (empty string)")
+ }
+ if strings.EqualFold(crs, defaultGeoCRS) {
+ return GeometryType{}, nil
+ }
+
+ return GeometryType{crs: crs}, nil
+}
+
+func (g GeometryType) CRS() string {
+ if g.crs == "" {
+ return defaultGeoCRS
+ }
+
+ return g.crs
+}
+
+func (g GeometryType) Equals(other Type) bool {
+ rhs, ok := other.(GeometryType)
+ if !ok {
+ return false
+ }
+
+ return g.crs == rhs.crs
+}
+
+func (GeometryType) primitive() {}
+func (g GeometryType) Type() string {
+ if g.crs == "" {
+ return "geometry"
+ }
+
+ return fmt.Sprintf("geometry(%s)", g.crs)
+}
+
+func (g GeometryType) String() string {
+ return g.Type()
+}
+
+type GeographyType struct {
+ crs string
+ algorithm EdgeAlgorithm
+}
+
+func GeographyTypeOf(crs string, algorithm EdgeAlgorithm) (GeographyType, error) {
+ if crs == "" {
+ return GeographyType{}, errors.New("invalid CRS: (empty string)")
+ }
+
+ normalizedCRS := crs
+ if strings.EqualFold(crs, defaultGeoCRS) {
+ normalizedCRS = ""
+ }
+
+ return GeographyType{crs: normalizedCRS, algorithm: algorithm}, nil
+}
+
+func (g GeographyType) CRS() string {
+ if g.crs == "" {
+ return defaultGeoCRS
+ }
+
+ return g.crs
+}
+
+func (g GeographyType) Algorithm() EdgeAlgorithm {
+ return g.algorithm
+}
+
+func (g GeographyType) Equals(other Type) bool {
+ rhs, ok := other.(GeographyType)
+ if !ok {
+ return false
+ }
+
+ return g.crs == rhs.crs && g.algorithm == rhs.algorithm
+}
+
+func (GeographyType) primitive() {}
+func (g GeographyType) Type() string {
Review Comment:
Spec default algorithm is `spherical`. Java treats it as null internally and emits canonical `"geography"` for `geography(OGC:CRS84, spherical)`. Here we re-emit the full string, so `GeographyTypeOf("OGC:CRS84", EdgeAlgorithmSpherical)` doesn't equal `GeographyType{}` and emits different JSON than Java for the same logical type — schema fingerprints will diverge. I'd canonicalize at construction (CRS84 + spherical → zero-value) and have `Algorithm()` return `EdgeAlgorithmSpherical` for the empty internal state, mirroring `CRS()`.
##########
types.go:
##########
@@ -777,6 +824,145 @@ func (UnknownType) primitive() {}
func (UnknownType) Type() string { return "unknown" }
func (UnknownType) String() string { return "unknown" }
+type EdgeAlgorithm string
+
+const (
+ EdgeAlgorithmSpherical EdgeAlgorithm = "spherical"
+ EdgeAlgorithmVincenty EdgeAlgorithm = "vincenty"
+ EdgeAlgorithmThomas EdgeAlgorithm = "thomas"
+ EdgeAlgorithmAndoyer EdgeAlgorithm = "andoyer"
+ EdgeAlgorithmKarney EdgeAlgorithm = "karney"
+)
+
+func ParseEdgeAlgorithm(s string) (EdgeAlgorithm, error) {
+ switch strings.ToLower(s) {
+ case "spherical":
+ return EdgeAlgorithmSpherical, nil
+ case "vincenty":
+ return EdgeAlgorithmVincenty, nil
+ case "thomas":
+ return EdgeAlgorithmThomas, nil
+ case "andoyer":
+ return EdgeAlgorithmAndoyer, nil
+ case "karney":
+ return EdgeAlgorithmKarney, nil
+ default:
+ return "", fmt.Errorf("invalid edge interpolation algorithm: %s", s)
+ }
+}
+
+func (e EdgeAlgorithm) String() string {
+ return string(e)
+}
+
+const defaultGeoCRS = "OGC:CRS84"
+
+type GeometryType struct {
+ crs string
+}
+
+func GeometryTypeOf(crs string) (GeometryType, error) {
+ if crs == "" {
+ return GeometryType{}, errors.New("invalid CRS: (empty string)")
+ }
+ if strings.EqualFold(crs, defaultGeoCRS) {
+ return GeometryType{}, nil
+ }
+
+ return GeometryType{crs: crs}, nil
+}
+
+func (g GeometryType) CRS() string {
+ if g.crs == "" {
+ return defaultGeoCRS
+ }
+
+ return g.crs
+}
+
+func (g GeometryType) Equals(other Type) bool {
+ rhs, ok := other.(GeometryType)
+ if !ok {
+ return false
+ }
+
+ return g.crs == rhs.crs
+}
+
+func (GeometryType) primitive() {}
+func (g GeometryType) Type() string {
+ if g.crs == "" {
+ return "geometry"
+ }
+
+ return fmt.Sprintf("geometry(%s)", g.crs)
+}
+
+func (g GeometryType) String() string {
+ return g.Type()
+}
+
+type GeographyType struct {
+ crs string
+ algorithm EdgeAlgorithm
+}
+
+func GeographyTypeOf(crs string, algorithm EdgeAlgorithm) (GeographyType, error) {
+ if crs == "" {
+ return GeographyType{}, errors.New("invalid CRS: (empty string)")
+ }
+
+ normalizedCRS := crs
+ if strings.EqualFold(crs, defaultGeoCRS) {
+ normalizedCRS = ""
+ }
+
+ return GeographyType{crs: normalizedCRS, algorithm: algorithm}, nil
Review Comment:
Doesn't validate `algorithm` — `GeographyTypeOf("srid:4326", EdgeAlgorithm("garbage"))` succeeds and produces metadata Java/PyIceberg will reject. JSON parse goes through `ParseEdgeAlgorithm`; the constructor doesn't. I'd run it here too when `algorithm != ""`.
##########
types.go:
##########
@@ -32,6 +33,8 @@ import (
var (
regexFromBrackets = regexp.MustCompile(`^\w+\[(\d+)\]$`)
decimalRegex = regexp.MustCompile(`decimal\(\s*(\d+)\s*,\s*(\d+)\s*\)`)
+ geometryRegex = regexp.MustCompile(`(?i)^geometry\s*(?:\(\s*([^)]+?)\s*\))?$`)
+ geographyRegex = regexp.MustCompile(`(?i)^geography\s*(?:\(\s*([^,]+?)\s*(?:,\s*(\w+)\s*)?\))?$`)
)
Review Comment:
Both regexes over-accept: `geometry(srid:4326, extra)` parses with the comma absorbed into the CRS group; `geography(srid:4269 karney)` (missing comma) parses with the space absorbed. I'd tighten the CRS group (e.g. `[^),]+?` for geometry) and add a couple of negative tests.
##########
table/metadata_schema_compatibility.go:
##########
@@ -39,7 +39,17 @@ func (e ErrIncompatibleSchema) Error() string {
fmt.Fprintf(&problems, "\n- invalid type for %s: %s is not supported until v%d", f.ColName, f.Field.Type, f.UnsupportedType.MinFormatVersion)
}
if f.InvalidDefault != 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)
+ switch f.Field.Type.(type) {
+ case iceberg.GeometryType, iceberg.GeographyType:
+ if f.Field.InitialDefault != nil {
+ fmt.Fprintf(&problems, "\n- invalid initial default for %s: %s columns must default to null", f.ColName, f.Field.Type)
Review Comment:
When a field has both `InitialDefault` and `WriteDefault`, two `IncompatibleField` entries get appended (see ~line 132 below) and `Error()` re-prints both default kinds for each — four lines for two problems. Either collapse to one entry per field, or drive the printout from `f.InvalidDefault.WriteDefault` (the value captured at append time).
##########
table/metadata_builder_internal_test.go:
##########
@@ -1838,6 +1840,86 @@ func TestUnknownTypeValidation(t *testing.T) {
})
}
+func TestGeometryGeographyNullOnlyDefaults(t *testing.T) {
Review Comment:
The `v2 with non-null initial default` subtest asserts `"is not supported until v3"`. That path actually fires both the v2-unsupported error and the must-default-to-null error, and `ErrorContains` happens to match the first. If geo ever becomes allowed in v2, this silently changes meaning. Worth splitting: one v2 subtest asserting the type-unsupported message, one v3 subtest with non-null default asserting must-default-to-null.
##########
table/metadata_schema_compatibility.go:
##########
@@ -113,12 +123,30 @@ func checkSchemaCompatibility(sc *iceberg.Schema, formatVersion int) error {
})
}
- if field.InitialDefault != nil && formatVersion < defaultValuesMinFormatVersion {
- problems = append(problems, IncompatibleField{
- Field: field,
- ColName: colName,
- InvalidDefault: &InvalidDefault{MinFormatVersion: defaultValuesMinFormatVersion, WriteDefault: field.InitialDefault},
- })
+ switch field.Type.(type) {
+ case iceberg.GeometryType, iceberg.GeographyType:
+ if field.InitialDefault != nil {
+ problems = append(problems, IncompatibleField{
+ Field: field,
+ ColName: colName,
+ InvalidDefault: &InvalidDefault{MinFormatVersion: formatVersion, WriteDefault: field.InitialDefault},
Review Comment:
`MinFormatVersion = formatVersion` (current version) here, but the field's documented as the minimum *required* version where the feature became supported. `IncompatibleField` is exported, so a downstream consumer reads a misleading value. Cleaner to add a separate reason flag like `MustBeNullForType` and switch on that in `Error()` instead of re-checking `f.Field.Type`.
##########
types.go:
##########
@@ -172,6 +179,46 @@ func (t *typeIFace) UnmarshalJSON(b []byte) error {
prec, _ := strconv.Atoi(matches[1])
scale, _ := strconv.Atoi(matches[2])
t.Type = DecimalType{precision: prec, scale: scale}
+ case strings.HasPrefix(strings.ToLower(typename), "geometry"):
Review Comment:
Geo type names match case-insensitively but every other type in this switch is case-sensitive. Matches Java for geo specifically, but it's internally surprising — worth a comment noting the asymmetry is intentional.
##########
types.go:
##########
@@ -777,6 +824,145 @@ func (UnknownType) primitive() {}
func (UnknownType) Type() string { return "unknown" }
func (UnknownType) String() string { return "unknown" }
+type EdgeAlgorithm string
+
+const (
+ EdgeAlgorithmSpherical EdgeAlgorithm = "spherical"
+ EdgeAlgorithmVincenty EdgeAlgorithm = "vincenty"
+ EdgeAlgorithmThomas EdgeAlgorithm = "thomas"
+ EdgeAlgorithmAndoyer EdgeAlgorithm = "andoyer"
+ EdgeAlgorithmKarney EdgeAlgorithm = "karney"
+)
+
+func ParseEdgeAlgorithm(s string) (EdgeAlgorithm, error) {
+ switch strings.ToLower(s) {
+ case "spherical":
+ return EdgeAlgorithmSpherical, nil
+ case "vincenty":
+ return EdgeAlgorithmVincenty, nil
+ case "thomas":
+ return EdgeAlgorithmThomas, nil
+ case "andoyer":
+ return EdgeAlgorithmAndoyer, nil
+ case "karney":
+ return EdgeAlgorithmKarney, nil
+ default:
+ return "", fmt.Errorf("invalid edge interpolation algorithm: %s", s)
+ }
+}
+
+func (e EdgeAlgorithm) String() string {
+ return string(e)
+}
+
+const defaultGeoCRS = "OGC:CRS84"
+
+type GeometryType struct {
+ crs string
+}
+
+func GeometryTypeOf(crs string) (GeometryType, error) {
+ if crs == "" {
+ return GeometryType{}, errors.New("invalid CRS: (empty string)")
Review Comment:
Plain `errors.New` here. Rest of this file wraps `ErrInvalidTypeString` (lines 161/169/198) so callers can `errors.Is`. Same applies to `GeographyTypeOf` (line 912) and `ParseEdgeAlgorithm` (line 850).
##########
table/arrow_utils.go:
##########
@@ -630,6 +630,24 @@ func (c convertToArrow) VisitUnknown() arrow.Field {
}
}
+func (c convertToArrow) VisitGeometry(iceberg.GeometryType) arrow.Field {
Review Comment:
Pass-through binary is fine for now. Worth a one-liner about the GeoArrow migration plan (extension metadata over a binary buffer, no shape change for downstream readers) so we don't trap ourselves once #991 lands. Also note this method is currently unreachable due to the dispatch issue on `schema.go`.
##########
transforms.go:
##########
@@ -107,9 +107,14 @@ func (t IdentityTransform) MarshalText() ([]byte, error) {
func (IdentityTransform) String() string { return "identity" }
func (IdentityTransform) CanTransform(t Type) bool {
- _, ok := t.(PrimitiveType)
+ switch t.(type) {
Review Comment:
Style nit — the nested switch only adds a level to special-case geo. I'd flatten:
```go
switch t.(type) {
case GeometryType, GeographyType:
return false
}
_, ok := t.(PrimitiveType)
return ok
```
##########
table/substrait/substrait.go:
##########
@@ -169,6 +169,10 @@ func (convertToSubstrait) VisitUnknown() types.Type {
// Returning nil indicates this type cannot be converted to Substrait
return nil
}
+func (convertToSubstrait) VisitGeometry(iceberg.GeometryType) types.Type { return &types.BinaryType{} }
Review Comment:
Same dispatch issue as `schema.go` — these are dead code today because `visitField` doesn't route to them, and `convertToSubstrait.Primitive` panics on the fall-through. Minor: `VisitGeometry` is one-line but `VisitGeography` is multi-line; surrounding methods are uniformly one-liners.
##########
transforms_test.go:
##########
@@ -258,6 +258,8 @@ func TestCanTransform(t *testing.T) {
},
notAllowed: []iceberg.Type{
&iceberg.StructType{}, &iceberg.ListType{}, &iceberg.MapType{},
+ iceberg.GeometryType{},
Review Comment:
`Identity` and `Bucket` get geo coverage but `Void`/`Truncate`/`Year`/`Month`/`Day`/`Hour` aren't pinned. `Void` particularly — per spec it's the only transform geo is allowed for, so it's the one most likely to silently regress.
##########
table/update_schema_test.go:
##########
@@ -344,6 +344,31 @@ func TestAddColumn(t *testing.T) {
}},
}, newSchema.Fields())
})
+
+ t.Run("test update schema with add geometry and geography columns", func(t *testing.T) {
+ table := New([]string{"id"}, testMetadata, "", nil, nil)
Review Comment:
This builds against `testMetadata` (v2) and only calls `Apply()` — that path skips `checkSchemaCompatibility`, so the test passes despite geo being illegal in v2. Reads as if v2 add works. Build a v3 metadata for this case (mirror the `geoMeta` construction in the error test below) and ideally run `Commit()`/`BuildUpdates()` so the realistic path is covered.
--
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] feat(table): Adding geometry and geography type + schema plumbing [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #984:
URL: https://github.com/apache/iceberg-go/pull/984#discussion_r3235445080
##########
types.go:
##########
@@ -777,6 +822,143 @@ func (UnknownType) primitive() {}
func (UnknownType) Type() string { return "unknown" }
func (UnknownType) String() string { return "unknown" }
+const defaultGeoCRS = "OGC:CRS84"
+
+type GeometryType struct {
+ crs string
+}
+
+func GeometryTypeOf(crs string) (GeometryType, error) {
+ if crs == "" {
+ return GeometryType{}, fmt.Errorf("%w: invalid CRS: (empty string)", ErrInvalidTypeString)
+ }
+ if strings.EqualFold(crs, defaultGeoCRS) {
+ return GeometryType{}, nil
+ }
+
+ return GeometryType{crs: crs}, nil
+}
+
+func (g GeometryType) CRS() string {
+ if g.crs == "" {
+ return defaultGeoCRS
+ }
+
+ return g.crs
+}
+
+func (g GeometryType) Equals(other Type) bool {
+ rhs, ok := other.(GeometryType)
+ if !ok {
+ return false
+ }
+
+ return g.crs == rhs.crs
+}
+
+func (GeometryType) primitive() {}
+func (g GeometryType) Type() string {
+ if g.crs == "" {
+ return "geometry"
+ }
+
+ return fmt.Sprintf("geometry(%s)", g.crs)
+}
+
+func (g GeometryType) String() string {
+ return g.Type()
+}
+
+const defaultGeographyAlgorithm = string(geoarrow.EdgeSpherical)
+
+func toGeoArrowEdgeInterpolation(s string) (geoarrow.EdgeInterpolation, error) {
+ algo := geoarrow.EdgeInterpolation(strings.ToLower(strings.TrimSpace(s)))
+ switch algo {
+ case geoarrow.EdgeSpherical, geoarrow.EdgeVincenty,
+ geoarrow.EdgeThomas, geoarrow.EdgeAndoyer, geoarrow.EdgeKarney:
+ return algo, nil
+ default:
+ return "", fmt.Errorf("%w: invalid edge interpolation algorithm", ErrInvalidTypeString)
+ }
+}
+
+type GeographyType struct {
+ crs string
+ algorithm string
+}
+
+func GeographyTypeOf(crs string, algorithm string) (GeographyType, error) {
+ if crs == "" {
+ return GeographyType{}, fmt.Errorf("%w: invalid CRS: (empty string)", ErrInvalidTypeString)
+ }
+ if algorithm == "" {
+ return GeographyType{}, fmt.Errorf("%w: invalid algorithm: (empty string)", ErrInvalidTypeString)
+ }
+
+ normalizedCRS := crs
+ if strings.EqualFold(crs, defaultGeoCRS) {
+ normalizedCRS = ""
+ }
+
+ validatedAlg, err := toGeoArrowEdgeInterpolation(algorithm) // validate algorithm
+ if err != nil {
+ return GeographyType{}, fmt.Errorf("invalid algorithm: %w", err)
+ }
+ normalizedAlgorithm := string(validatedAlg)
+ if strings.EqualFold(algorithm, defaultGeographyAlgorithm) {
Review Comment:
Canonicalization branches on the raw input, not the validated form. `toGeoArrowEdgeInterpolation` above trims and lowercases, but `strings.EqualFold(algorithm, defaultGeographyAlgorithm)` here compares against the raw string — so `GeographyTypeOf("srid:4326", "spherical")` normalizes to `{algorithm: ""}` but `GeographyTypeOf("srid:4326", " spherical ")` keeps `{algorithm: "spherical"}`. Two semantically identical types whose `Equals` returns false. JSON path trims before calling so this is invisible there, but Go callers can trip it.
One-line fix — branch on `validatedAlg`, not `algorithm`:
```go
if validatedAlg == geoarrow.EdgeInterpolation(defaultGeographyAlgorithm) {
normalizedAlgorithm = ""
}
```
##########
exprs.go:
##########
@@ -450,6 +450,8 @@ func createBoundRef(field NestedField, acc accessor) BoundReference {
return &boundRef[Decimal]{field: field, acc: acc}
case UUIDType:
return &boundRef[uuid.UUID]{field: field, acc: acc}
+ case GeographyType, GeometryType:
+ return &boundRef[[]byte]{field: field, acc: acc}
Review Comment:
Found while tracing this — `createBoundRef` handles geo here, but four downstream dispatch sites that switch on the same `Type` don't, and they're inconsistent in how they fail.
`exprs.go:637` (unary predicate dispatch) — `case FixedType, BinaryType` doesn't include geo, falls through to `panic("unhandled bound reference type: ...")` at line 644. `IsNull(Ref("geom_col"))` on an optional geo column (exactly the case this PR's null-default rule enforces) crashes the process.
`exprs.go:799` (literal predicate) and `exprs.go:967` (set predicate) — same shape, friendly error on fall-through.
`visitors.go:323` (`doCmp`) — `case BinaryType, FixedType` falls through to `panic(ErrType)` at line 332. Metric-evaluation paths hit this.
Two options: (a) add `case GeometryType, GeographyType` at all four sites, treating geo as `[]byte` like Binary — most useful since `IsNull` / `NotNull` / `IsIn(null)` on geo are semantically valid; (b) reject geo in `createBoundRef` so all expression paths fail consistently at bind. I'd take (a).
##########
table/substrait/substrait.go:
##########
@@ -169,6 +169,10 @@ func (convertToSubstrait) VisitUnknown() types.Type {
// Returning nil indicates this type cannot be converted to Substrait
return nil
}
+func (convertToSubstrait) VisitGeometry(iceberg.GeometryType) types.Type { return &types.BinaryType{} }
Review Comment:
gofmt doesn't force one-line vs multi-line on single-return bodies: it preserves whatever you write. If a line-length linter is balking, easier path is to make VisitGeometry multi-line too for symmetry. Reads cleaner than either form on its own.
##########
types.go:
##########
@@ -777,6 +822,143 @@ func (UnknownType) primitive() {}
func (UnknownType) Type() string { return "unknown" }
func (UnknownType) String() string { return "unknown" }
+const defaultGeoCRS = "OGC:CRS84"
+
+type GeometryType struct {
+ crs string
+}
+
+func GeometryTypeOf(crs string) (GeometryType, error) {
+ if crs == "" {
+ return GeometryType{}, fmt.Errorf("%w: invalid CRS: (empty string)", ErrInvalidTypeString)
+ }
+ if strings.EqualFold(crs, defaultGeoCRS) {
Review Comment:
Same shape as the algorithm canonicalization in `GeographyTypeOf` below — `strings.EqualFold(crs, defaultGeoCRS)` compares the raw input, so `GeometryTypeOf("OGC:CRS84")` normalizes to the zero value but `GeometryTypeOf(" OGC:CRS84 ")` doesn't. Trim at the top of the constructor (`crs = strings.TrimSpace(crs)`) to make canonicalization tolerant.
--
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] feat(table): Adding geometry and geography type + schema plumbing [iceberg-go]
Posted by "happydave1 (via GitHub)" <gi...@apache.org>.
happydave1 commented on code in PR #984:
URL: https://github.com/apache/iceberg-go/pull/984#discussion_r3261855171
##########
exprs.go:
##########
@@ -450,6 +450,8 @@ func createBoundRef(field NestedField, acc accessor) BoundReference {
return &boundRef[Decimal]{field: field, acc: acc}
case UUIDType:
return &boundRef[uuid.UUID]{field: field, acc: acc}
+ case GeographyType, GeometryType:
+ return &boundRef[[]byte]{field: field, acc: acc}
Review Comment:
Good catch, thanks!
I took option A - it made the most sense to me. Now the downstream dispatch sites should handle the geo-types more consistently.
--
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] feat(table): Adding geometry and geography type + schema plumbing [iceberg-go]
Posted by "happydave1 (via GitHub)" <gi...@apache.org>.
happydave1 commented on PR #984:
URL: https://github.com/apache/iceberg-go/pull/984#issuecomment-4511802201
I've rebased locally but it seems that `origin/main` tests are still failing when running `make test`. Specifically `TestTableWriting` tests. Correct me if I am wrong but is #1102 maybe related to this?
--
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] feat(table): Adding geometry and geography type + schema plumbing [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on PR #984:
URL: https://github.com/apache/iceberg-go/pull/984#issuecomment-4500744419
I'll take a look at this today for a final pass. Can you resolve the conflicts in the meantime?
--
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] feat(table): Adding geometry and geography type + schema plumbing [iceberg-go]
Posted by "happydave1 (via GitHub)" <gi...@apache.org>.
happydave1 commented on code in PR #984:
URL: https://github.com/apache/iceberg-go/pull/984#discussion_r3235822656
##########
types.go:
##########
@@ -777,6 +822,143 @@ func (UnknownType) primitive() {}
func (UnknownType) Type() string { return "unknown" }
func (UnknownType) String() string { return "unknown" }
+const defaultGeoCRS = "OGC:CRS84"
+
+type GeometryType struct {
+ crs string
+}
+
+func GeometryTypeOf(crs string) (GeometryType, error) {
+ if crs == "" {
+ return GeometryType{}, fmt.Errorf("%w: invalid CRS: (empty string)", ErrInvalidTypeString)
+ }
+ if strings.EqualFold(crs, defaultGeoCRS) {
+ return GeometryType{}, nil
+ }
+
+ return GeometryType{crs: crs}, nil
+}
+
+func (g GeometryType) CRS() string {
+ if g.crs == "" {
+ return defaultGeoCRS
+ }
+
+ return g.crs
+}
+
+func (g GeometryType) Equals(other Type) bool {
+ rhs, ok := other.(GeometryType)
+ if !ok {
+ return false
+ }
+
+ return g.crs == rhs.crs
+}
+
+func (GeometryType) primitive() {}
+func (g GeometryType) Type() string {
+ if g.crs == "" {
+ return "geometry"
+ }
+
+ return fmt.Sprintf("geometry(%s)", g.crs)
+}
+
+func (g GeometryType) String() string {
+ return g.Type()
+}
+
+const defaultGeographyAlgorithm = string(geoarrow.EdgeSpherical)
+
+func toGeoArrowEdgeInterpolation(s string) (geoarrow.EdgeInterpolation, error) {
+ algo := geoarrow.EdgeInterpolation(strings.ToLower(strings.TrimSpace(s)))
+ switch algo {
+ case geoarrow.EdgeSpherical, geoarrow.EdgeVincenty,
+ geoarrow.EdgeThomas, geoarrow.EdgeAndoyer, geoarrow.EdgeKarney:
+ return algo, nil
+ default:
+ return "", fmt.Errorf("%w: invalid edge interpolation algorithm", ErrInvalidTypeString)
+ }
+}
+
+type GeographyType struct {
+ crs string
+ algorithm string
+}
+
+func GeographyTypeOf(crs string, algorithm string) (GeographyType, error) {
+ if crs == "" {
+ return GeographyType{}, fmt.Errorf("%w: invalid CRS: (empty string)", ErrInvalidTypeString)
+ }
+ if algorithm == "" {
+ return GeographyType{}, fmt.Errorf("%w: invalid algorithm: (empty string)", ErrInvalidTypeString)
+ }
+
+ normalizedCRS := crs
+ if strings.EqualFold(crs, defaultGeoCRS) {
+ normalizedCRS = ""
+ }
+
+ validatedAlg, err := toGeoArrowEdgeInterpolation(algorithm) // validate algorithm
+ if err != nil {
+ return GeographyType{}, fmt.Errorf("invalid algorithm: %w", err)
+ }
+ normalizedAlgorithm := string(validatedAlg)
+ if strings.EqualFold(algorithm, defaultGeographyAlgorithm) {
Review Comment:
Good catch! I will update this!
--
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