laskoviymishka commented on code in PR #1235:
URL: https://github.com/apache/iceberg-go/pull/1235#discussion_r3443416422
##########
table/arrow_utils.go:
##########
@@ -1944,6 +1944,9 @@ func geoArrowMetadataToIcebergType(meta
geoarrow.Metadata) (iceberg.Type, error)
return nil, err
}
+ // Missing GeoArrow edges use the planar default, which reads back as
Review Comment:
The arrow-rs half checks out (arrow-rs#10065 reads no-edges as Geometry),
but the PyIceberg framing isn't quite right — PyIceberg has no Arrow→Iceberg
geo path that inspects GeoArrow extension metadata; it goes off the Iceberg
schema in file metadata. There's a sharper point hiding here too: PyIceberg
writes Geography(vincenty/karney/thomas/andoyer) *without* an edges member, and
this branch decodes those as Geometry — a silent downgrade. I'd cite arrow-rs
for the planar default and name the PyIceberg case as a known interop gap
rather than a shared default (and maybe file a follow-up for the downgrade
itself).
##########
table/internal/parquet_files_test.go:
##########
@@ -658,26 +713,42 @@ func TestParquetGeoArrowExtensionMetadataRoundTrip(t
*testing.T) {
roundTripSchema := tbl.Schema()
- origGeomType := arrowSchema.Field(1).Type.(*geoarrow.WKBType)
- origGeogType := arrowSchema.Field(2).Type.(*geoarrow.WKBType)
- rtGeomField := roundTripSchema.Field(1)
- rtGeogField := roundTripSchema.Field(2)
-
- rtGeomExt := rtGeomField.Type.(arrow.ExtensionType)
- rtGeogExt := rtGeogField.Type.(arrow.ExtensionType)
- rtGeomType := rtGeomExt.(*geoarrow.WKBType)
- rtGeogType := rtGeogExt.(*geoarrow.WKBType)
-
- assert.Equal(t, origGeomType.ExtensionName(), rtGeomExt.ExtensionName())
- assert.Equal(t, origGeogType.ExtensionName(), rtGeogExt.ExtensionName())
- assert.True(t, arrow.TypeEqual(origGeomType.StorageType(),
rtGeomType.StorageType()))
- assert.True(t, arrow.TypeEqual(origGeogType.StorageType(),
rtGeogType.StorageType()))
- assert.Equal(t, origGeomType.Metadata(), rtGeomType.Metadata())
- assert.Equal(t, origGeogType.Metadata(), rtGeogType.Metadata())
- assert.Equal(t, geoarrow.CRSTypeSRID, rtGeomType.Metadata().CRSType)
- assert.Equal(t, geoarrow.EdgePlanar, rtGeomType.Metadata().Edges)
- assert.Equal(t, geoarrow.CRSTypeSRID, rtGeogType.Metadata().CRSType)
- assert.Equal(t, geoarrow.EdgeVincenty, rtGeogType.Metadata().Edges)
+ tests := []struct {
+ name string
+ idx int
+ wantMeta geoarrow.Metadata
+ wantType geoarrow.EdgeInterpolation
+ }{
+ {
+ name: "explicit_srid_geometry", idx: 1,
+ wantMeta: geoarrow.Metadata{CRS: geoArrowCRS("4326"),
Edges: geoarrow.EdgePlanar},
+ wantType: geoarrow.EdgePlanar,
+ },
+ {
+ name: "explicit_srid_geography", idx: 2,
+ wantMeta: geoarrow.Metadata{CRS: geoArrowCRS("4326"),
Edges: geoarrow.EdgeVincenty},
+ wantType: geoarrow.EdgeVincenty,
+ },
+ {
+ name: "default_crs_geometry", idx: 3,
+ wantMeta: geoarrow.Metadata{CRS:
geoArrowCRS("OGC:CRS84"), Edges: geoarrow.EdgePlanar},
+ wantType: geoarrow.EdgePlanar,
+ },
+ {
+ name: "default_crs_geography", idx: 4,
+ wantMeta: geoarrow.Metadata{CRS:
geoArrowCRS("OGC:CRS84"), Edges: geoarrow.EdgeSpherical},
+ wantType: geoarrow.EdgeSpherical,
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run("metadata/"+tt.name, func(t *testing.T) {
+ origType := assertGeoArrowWKBType(t,
arrowSchema.Field(tt.idx).Type, arrow.BinaryTypes.Binary, tt.wantMeta)
+ rtType := assertGeoArrowWKBType(t,
roundTripSchema.Field(tt.idx).Type, arrow.BinaryTypes.Binary, tt.wantMeta)
+ assert.Equal(t, origType.Metadata(), rtType.Metadata())
+ assert.Equal(t, tt.wantType, rtType.Metadata().Edges)
Review Comment:
`tt.wantType` is always equal to `tt.wantMeta.Edges`, and
`assertGeoArrowWKBType` already asserts that value against
`rtType.Metadata().Edges` — so this line is a duplicate assertion. It also
implies the two can vary independently, which invites a future editor to update
one and not the other. I'd drop the `wantType` field and this assert.
##########
table/internal/parquet_files_test.go:
##########
@@ -658,26 +713,42 @@ func TestParquetGeoArrowExtensionMetadataRoundTrip(t
*testing.T) {
roundTripSchema := tbl.Schema()
- origGeomType := arrowSchema.Field(1).Type.(*geoarrow.WKBType)
- origGeogType := arrowSchema.Field(2).Type.(*geoarrow.WKBType)
- rtGeomField := roundTripSchema.Field(1)
- rtGeogField := roundTripSchema.Field(2)
-
- rtGeomExt := rtGeomField.Type.(arrow.ExtensionType)
- rtGeogExt := rtGeogField.Type.(arrow.ExtensionType)
- rtGeomType := rtGeomExt.(*geoarrow.WKBType)
- rtGeogType := rtGeogExt.(*geoarrow.WKBType)
-
- assert.Equal(t, origGeomType.ExtensionName(), rtGeomExt.ExtensionName())
- assert.Equal(t, origGeogType.ExtensionName(), rtGeogExt.ExtensionName())
- assert.True(t, arrow.TypeEqual(origGeomType.StorageType(),
rtGeomType.StorageType()))
- assert.True(t, arrow.TypeEqual(origGeogType.StorageType(),
rtGeogType.StorageType()))
- assert.Equal(t, origGeomType.Metadata(), rtGeomType.Metadata())
- assert.Equal(t, origGeogType.Metadata(), rtGeogType.Metadata())
- assert.Equal(t, geoarrow.CRSTypeSRID, rtGeomType.Metadata().CRSType)
- assert.Equal(t, geoarrow.EdgePlanar, rtGeomType.Metadata().Edges)
- assert.Equal(t, geoarrow.CRSTypeSRID, rtGeogType.Metadata().CRSType)
- assert.Equal(t, geoarrow.EdgeVincenty, rtGeogType.Metadata().Edges)
+ tests := []struct {
+ name string
+ idx int
+ wantMeta geoarrow.Metadata
+ wantType geoarrow.EdgeInterpolation
+ }{
+ {
+ name: "explicit_srid_geometry", idx: 1,
+ wantMeta: geoarrow.Metadata{CRS: geoArrowCRS("4326"),
Edges: geoarrow.EdgePlanar},
+ wantType: geoarrow.EdgePlanar,
+ },
+ {
+ name: "explicit_srid_geography", idx: 2,
+ wantMeta: geoarrow.Metadata{CRS: geoArrowCRS("4326"),
Edges: geoarrow.EdgeVincenty},
+ wantType: geoarrow.EdgeVincenty,
+ },
+ {
+ name: "default_crs_geometry", idx: 3,
+ wantMeta: geoarrow.Metadata{CRS:
geoArrowCRS("OGC:CRS84"), Edges: geoarrow.EdgePlanar},
+ wantType: geoarrow.EdgePlanar,
+ },
+ {
+ name: "default_crs_geography", idx: 4,
+ wantMeta: geoarrow.Metadata{CRS:
geoArrowCRS("OGC:CRS84"), Edges: geoarrow.EdgeSpherical},
+ wantType: geoarrow.EdgeSpherical,
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run("metadata/"+tt.name, func(t *testing.T) {
+ origType := assertGeoArrowWKBType(t,
arrowSchema.Field(tt.idx).Type, arrow.BinaryTypes.Binary, tt.wantMeta)
+ rtType := assertGeoArrowWKBType(t,
roundTripSchema.Field(tt.idx).Type, arrow.BinaryTypes.Binary, tt.wantMeta)
+ assert.Equal(t, origType.Metadata(), rtType.Metadata())
Review Comment:
`assertGeoArrowWKBType` only checks CRS and Edges, never CRSType, so the
table doesn't pin it anywhere. The old code asserted `CRSTypeSRID` for the
geom/geog SRID cases; that's gone, and the two new default-CRS columns expect
`CRSTypeAuthorityCode` with nothing verifying it. This orig-vs-rt equality only
compares the two computed sides, so a regression that corrupts CRSType on both
sides slips through. I'd thread `want.CRSType` through the helper (assert when
non-empty) and set it on all four `wantMeta` structs.
##########
table/internal/parquet_files_test.go:
##########
@@ -604,6 +605,42 @@ func TestDecimalPhysicalTypes(t *testing.T) {
}
}
+func geoArrowCRS(s string) json.RawMessage {
+ raw, _ := json.Marshal(s)
+
+ return raw
+}
+
+func assertGeoArrowWKBType(t *testing.T, dt arrow.DataType, storage
arrow.DataType, want geoarrow.Metadata) *geoarrow.WKBType {
+ t.Helper()
+
+ wkbType, ok := dt.(*geoarrow.WKBType)
+ require.True(t, ok, "expected *geoarrow.WKBType, got %T", dt)
+ assert.Equal(t, "geoarrow.wkb", wkbType.ExtensionName())
+ assert.True(t, arrow.TypeEqual(storage, wkbType.StorageType()))
+ assert.Equal(t, want.CRS, wkbType.Metadata().CRS)
+ assert.Equal(t, want.Edges, wkbType.Metadata().Edges)
Review Comment:
`EdgePlanar` is the zero value (`""`), so the geometry-side Edges assertions
pass even if a refactor stopped setting Edges explicitly. Pre-existing property
of the library, not something you introduced — but the new assertions inherit
it. Asserting the inverse for the geometry cases (e.g. `meta.Edges !=
EdgeSpherical`) would make it actually catch a regression. Non-blocking.
##########
table/arrow_utils.go:
##########
@@ -683,8 +683,8 @@ func (c convertToArrow) VisitGeometry(g
iceberg.GeometryType) arrow.Field {
func (c convertToArrow) VisitGeography(g iceberg.GeographyType) arrow.Field {
meta := icebergCRSToGeoArrowMetadata(g.CRS())
- // Always add an edge to differentiate between Geography and Geometry
arrow fields.
- // Note that the edge convention is a best-effort hint and planar
geography from other clients won't round-trip through Arrow alone.
+ // Always add an edge algorithm so Arrow metadata can distinguish
Review Comment:
The new wording reads like the edge convention guarantees a correct round
trip, but that only holds when iceberg-go is the writer. The old caveat was
load-bearing — for files written by other clients that omit edges for
non-spherical geography, the round trip through Arrow alone is lossy. I'd keep
a trimmed version of that note rather than drop it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
