paleolimbot commented on code in PR #1138:
URL: https://github.com/apache/iceberg-go/pull/1138#discussion_r3413903507
##########
table/arrow_utils.go:
##########
@@ -658,20 +672,26 @@ func (c convertToArrow) VisitVariant() arrow.Field {
return arrow.Field{Type: extensions.NewDefaultVariantType()}
}
-func (c convertToArrow) VisitGeometry(iceberg.GeometryType) arrow.Field {
+func (c convertToArrow) VisitGeometry(g iceberg.GeometryType) arrow.Field {
+ meta := icebergCRSToGeoArrowMetadata(g.CRS())
if c.useLargeTypes {
- return arrow.Field{Type:
geoarrow.NewWKBType(geoarrow.WKBWithLargeBinaryStorage())}
+ return arrow.Field{Type:
geoarrow.NewWKBType(geoarrow.WKBWithLargeBinaryStorage(),
geoarrow.WKBWithMetadata(meta))}
}
- return arrow.Field{Type:
geoarrow.NewWKBType(geoarrow.WKBWithBinaryStorage())}
+ return arrow.Field{Type:
geoarrow.NewWKBType(geoarrow.WKBWithBinaryStorage(),
geoarrow.WKBWithMetadata(meta))}
}
-func (c convertToArrow) VisitGeography(iceberg.GeographyType) 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.
Review Comment:
I think this comment is misleading (planar geography is not a thing, and
this is not a hint, it affects correctness).
```suggestion
// Always add an edge to differentiate between Geography and Geometry
arrow fields.
```
##########
table/arrow_utils_test.go:
##########
@@ -430,6 +431,466 @@ func TestVariantArrowConversion(t *testing.T) {
})
}
+func assertGeoArrowWKB(t *testing.T, dt arrow.DataType, storage
arrow.DataType, want geoarrow.Metadata) {
+ t.Helper()
+
+ wkb, ok := dt.(*geoarrow.WKBType)
+ require.True(t, ok, "expected *geoarrow.WKBType, got %T", dt)
+ assert.Equal(t, "geoarrow.wkb", wkb.ExtensionName())
+ assert.True(t, arrow.TypeEqual(storage, wkb.StorageType()))
+ assert.Equal(t, want.CRS, wkb.Metadata().CRS)
+ assert.Equal(t, want.Edges, wkb.Metadata().Edges)
+}
+
+func assertGeoArrowWKBMetadataJSON(t *testing.T, dt arrow.DataType, storage
arrow.DataType, wantJSON string) {
+ t.Helper()
+
+ wkb, ok := dt.(*geoarrow.WKBType)
+ require.True(t, ok, "expected *geoarrow.WKBType, got %T", dt)
+ assert.Equal(t, "geoarrow.wkb", wkb.ExtensionName())
+ assert.True(t, arrow.TypeEqual(storage, wkb.StorageType()))
+
+ var want map[string]json.RawMessage
+ require.NoError(t, json.Unmarshal([]byte(wantJSON), &want))
+
+ meta := wkb.Metadata()
+
+ if wantCRS, ok := want["crs"]; ok {
+ assert.JSONEq(t, string(wantCRS), string(meta.CRS), "crs
mismatch")
+ } else {
+ assert.Empty(t, meta.CRS, "expected omitted crs")
+ }
+
+ if wantEdges, ok := want["edges"]; ok {
+ var edges string
+ require.NoError(t, json.Unmarshal(wantEdges, &edges))
+ assert.Equal(t, geoarrow.EdgeInterpolation(edges), meta.Edges,
"edges mismatch")
+ } else {
+ assert.Empty(t, meta.Edges, "expected omitted edges")
+ }
+}
+
+func jsonCRS(s string) json.RawMessage {
+ raw, _ := json.Marshal(s)
+
+ return raw
+}
+
+func TestIcebergGeoTypesToArrowSchema(t *testing.T) {
+ geomSRID, err := iceberg.GeometryTypeOf("srid:4326")
+ require.NoError(t, err)
+ geogKarney, err := iceberg.GeographyTypeOf("srid:4269", "karney")
+ require.NoError(t, err)
+
+ // Note that these tests below are based on arrow-rs tests
(https://github.com/apache/arrow-rs/pull/10065)
+ geomSRID0, err := iceberg.GeometryTypeOf("srid:0")
+ require.NoError(t, err)
+ geomEPSG4267, err := iceberg.GeometryTypeOf("EPSG:4267")
+ require.NoError(t, err)
+ geogSpherical, err := iceberg.GeographyTypeOf("OGC:CRS84", "spherical")
+ require.NoError(t, err)
+ geogKarneyDefaultCRS, err := iceberg.GeographyTypeOf("OGC:CRS84",
"karney")
+ require.NoError(t, err)
+ geogVincenty, err := iceberg.GeographyTypeOf("OGC:CRS84", "vincenty")
+ require.NoError(t, err)
+ geogAndoyer, err := iceberg.GeographyTypeOf("OGC:CRS84", "andoyer")
+ require.NoError(t, err)
+ geogThomas, err := iceberg.GeographyTypeOf("OGC:CRS84", "thomas")
+ require.NoError(t, err)
+ geogSRID0, err := iceberg.GeographyTypeOf("srid:0", "spherical")
+ require.NoError(t, err)
+ geogEPSG4267, err := iceberg.GeographyTypeOf("EPSG:4267", "spherical")
+ require.NoError(t, err)
+
+ defaultGeometry, err := iceberg.GeometryTypeOf("OGC:CRS84")
+ require.NoError(t, err)
+ geomPROJJSON3857, err := iceberg.GeometryTypeOf("EPSG:3857")
+ require.NoError(t, err)
+ geogEPSG4267Karney, err := iceberg.GeographyTypeOf("EPSG:4267",
"karney")
+ require.NoError(t, err)
+ geogPROJJSON4267, err := iceberg.GeographyTypeOf("EPSG:4267",
"spherical")
+ require.NoError(t, err)
Review Comment:
These are still confusingly labelled (these are not PROJJSON crses). In
arrow-rs I split these into two distinct tests:
- given an Parquet type, verify expected metadata
- given GeoArrow metadata, verify expected Parquet type
Doing that here would eliminate that confusion.
--
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]
