You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "laskoviymishka (via GitHub)" <gi...@apache.org> on 2026/05/07 08:42:59 UTC
Re: [PR] feat(table): Adding geometry and geography type + schema plumbing [iceberg-go]
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