feat(table): Adding geometry and geography type + schema plumbing by happydave1 · Pull Request #984 · apache/iceberg-go

happydave1 · 2026-05-05T03:42:35Z

Based on #628 and addresses #990

Note that this PR sets up a PR to address #991

Signed-off-by: happydave1 <dzhao2004@gmail.com>

laskoviymishka

Direction looks good overall: JSON parsing, partition-spec rejection, null-only defaults, v3 gating, and Substrait pass-through all seem aligned with the spec.

I’d hold this before merge on two things:

SchemaVisitorPerPrimitiveType now has VisitGeometry / VisitGeography, and the visitors implement them, but visitField does not seem to dispatch to them yet. So geo columns still fall through to visitor.Primitive(...), which panics for both Arrow and Substrait conversion. That means SchemaToArrowSchema or Substrait conversion can panic on a geo column today.
The geography default algorithm canonicalization seems to differ from Java. The spec default is spherical; Java treats that as the implicit/default form and emits canonical geography for geography(OGC:CRS84, spherical). This PR appears to re-emit the full form, which may cause cross-client schema fingerprint drift for the same logical type.

The rest looks like smaller cleanups. Once these are addressed, happy to take another pass.

laskoviymishka · 2026-05-07T08:39:51Z

 	VisitBinary() T
 	VisitUUID() T
 	VisitUnknown() T
+	VisitGeometry(GeometryType) T


These methods get added to the interface but visitField (~671–720) doesn't dispatch them. GeometryType/GeographyType are PrimitiveTypes, 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{}).

laskoviymishka · 2026-05-07T08:39:51Z

+}
+
+func (GeographyType) primitive() {}
+func (g GeographyType) Type() string {


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().

laskoviymishka · 2026-05-07T08:39:51Z

+		normalizedCRS = ""
+	}
+
+	return GeographyType{crs: normalizedCRS, algorithm: algorithm}, nil


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 != "".

laskoviymishka · 2026-05-07T08:39:51Z

+
+func GeometryTypeOf(crs string) (GeometryType, error) {
+	if crs == "" {
+		return GeometryType{}, errors.New("invalid CRS: (empty string)")


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).

laskoviymishka · 2026-05-07T08:39:51Z

 	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*)?\))?$`)
 )


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.

laskoviymishka · 2026-05-07T08:39:51Z

 	// Returning nil indicates this type cannot be converted to Substrait
 	return nil
 }
+func (convertToSubstrait) VisitGeometry(iceberg.GeometryType) types.Type { return &types.BinaryType{} }


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.

laskoviymishka · 2026-05-07T08:39:51Z


 func (IdentityTransform) CanTransform(t Type) bool {
-	_, ok := t.(PrimitiveType)
+	switch t.(type) {


Style nit — the nested switch only adds a level to special-case geo. I'd flatten:

switch t.(type) { case GeometryType, GeographyType: return false } _, ok := t.(PrimitiveType) return ok

laskoviymishka · 2026-05-07T08:39:51Z

 			},
 			notAllowed: []iceberg.Type{
 				&iceberg.StructType{}, &iceberg.ListType{}, &iceberg.MapType{},
+				iceberg.GeometryType{},


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.

laskoviymishka · 2026-05-07T08:39:51Z

 	})
+
+	t.Run("test update schema with add geometry and geography columns", func(t *testing.T) {
+		table := New([]string{"id"}, testMetadata, "", nil, nil)


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.

laskoviymishka · 2026-05-07T08:39:51Z

 	})
 }

+func TestGeometryGeographyNullOnlyDefaults(t *testing.T) {


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.

zeroshade · 2026-05-07T16:51:48Z

+type EdgeAlgorithm string
+
+const (
+	EdgeAlgorithmSpherical EdgeAlgorithm = "spherical"
+	EdgeAlgorithmVincenty  EdgeAlgorithm = "vincenty"
+	EdgeAlgorithmThomas    EdgeAlgorithm = "thomas"
+	EdgeAlgorithmAndoyer   EdgeAlgorithm = "andoyer"
+	EdgeAlgorithmKarney    EdgeAlgorithm = "karney"
+)


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

zeroshade · 2026-05-07T17:03:18Z

 	case UUIDType:
 		return &boundRef[uuid.UUID]{field: field, acc: acc}
+	case GeographyType, GeometryType:
+		return &boundRef[[]byte]{field: field, acc: acc}


should this be https://github.com/geoarrow/geoarrow-go/blob/main/wkb.go#L20 instead of []byte?

zeroshade · 2026-05-07T17:11:51Z

 	}
 }

+func (c convertToArrow) VisitGeometry(iceberg.GeometryType) arrow.Field {


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

zeroshade · 2026-05-07T17:12:43Z

+	// 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}


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?

mzzz-zzm mentioned this pull request May 6, 2026

fix(occ): RowDelta uses partition-scoped conflict check instead of AlwaysTrue #983

Merged

twuebi and others added 5 commits May 6, 2026 16:14

wip: geo types

1ff08d0

rebasing

d7fef48

Signed-off-by: happydave1 <dzhao2004@gmail.com>

fixing linting

751cdb4

Signed-off-by: happydave1 <dzhao2004@gmail.com>

adding tests and arrow and substrait passthroughs

a8c1010

Signed-off-by: happydave1 <dzhao2004@gmail.com>

adding partition spec tests

813f3ba

Signed-off-by: happydave1 <dzhao2004@gmail.com>

happydave1 force-pushed the feat/geo-type branch from b223c24 to 813f3ba Compare May 6, 2026 20:16

happydave1 changed the title ~~Feat/geo type~~ feat(table): Adding geometry and geography type + schema plumbing May 6, 2026

happydave1 marked this pull request as ready for review May 6, 2026 20:30

happydave1 requested a review from zeroshade as a code owner May 6, 2026 20:30

laskoviymishka requested changes May 7, 2026

View reviewed changes

zeroshade requested changes May 7, 2026

View reviewed changes

Conversation

happydave1 commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laskoviymishka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

happydave1 commented May 5, 2026 •

edited

Loading