laskoviymishka commented on code in PR #1634:
URL: https://github.com/apache/iceberg-go/pull/1634#discussion_r3714517959
##########
table/arrow_utils_test.go:
##########
@@ -2756,3 +2774,244 @@ func
TestToRequestedSchemaMismatchedExtensionNameNotRewrapped(t *testing.T) {
// silently rewrapped.
require.Error(t, err)
}
+
+// absentCRSWKBType returns the geoarrow.wkb type a reader reconstructs for a
+// column whose stored GeoArrow metadata carries no CRS. Omitted edges mean
+// planar, so the geography cases must state their edge algorithm.
+func absentCRSWKBType(edges geoarrow.EdgeInterpolation) arrow.ExtensionType {
+ return geoarrow.NewWKBType(geoarrow.WKBWithBinaryStorage(),
+ geoarrow.WKBWithMetadata(geoarrow.Metadata{Edges: edges}))
+}
+
+func TestArrowGeoAbsentCRSMetadataToIcebergDefaults(t *testing.T) {
+ tests := []struct {
+ name string
+ edges geoarrow.EdgeInterpolation
+ want iceberg.Type
+ wantType string
+ }{
+ {
+ name: "geometry_crs_omitted",
+ want: iceberg.GeometryType{},
+ wantType: "geometry",
+ },
+ {
+ name: "geography_crs_omitted",
+ edges: geoarrow.EdgeSpherical,
+ want: iceberg.GeographyType{},
+ wantType: "geography",
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ dt := absentCRSWKBType(tt.edges)
+
+ sc, err :=
table.ArrowSchemaToIceberg(arrow.NewSchema([]arrow.Field{{
+ Name: "geom", Type: dt, Nullable: true,
Metadata: fieldIDMeta("1"),
+ }}, nil), false, nil)
+ require.NoError(t, err)
+
+ field, ok := sc.FindFieldByID(1)
+ require.True(t, ok)
+ assert.Equal(t, tt.wantType, field.Type.String())
+ assert.True(t, tt.want.Equals(field.Type), "expected
%s, got %s", tt.want, field.Type)
+ })
+ }
+}
+
+func TestToRequestedSchemaGeoAbsentCRSProjection(t *testing.T) {
+ values := wkbPointRows()
+
+ tests := []struct {
+ name string
+ edges geoarrow.EdgeInterpolation
+ icebergType iceberg.Type
+ }{
+ {
+ name: "geometry_crs_omitted",
+ icebergType: iceberg.GeometryType{},
+ },
+ {
+ name: "geography_crs_omitted",
+ edges: geoarrow.EdgeSpherical,
+ icebergType: iceberg.GeographyType{},
+ },
+ }
+
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ mem :=
memory.NewCheckedAllocator(memory.NewGoAllocator())
+ defer mem.AssertSize(t, 0)
+
+ sourceType := absentCRSWKBType(tt.edges)
+ source := newExtensionArrayOverBinary(t, mem,
sourceType, values)
+ defer source.Release()
+
+ rec := singleColumnRecord(arrow.Field{
+ Name: "geom", Type: sourceType, Nullable: true,
Metadata: fieldIDMeta("1"),
+ }, source)
+ defer rec.Release()
+
+ fileSchema, err :=
table.ArrowSchemaToIceberg(rec.Schema(), false, nil)
+ require.NoError(t, err)
+
+ requested := iceberg.NewSchema(0, iceberg.NestedField{
+ ID: 1, Name: "geom", Type: tt.icebergType,
Required: false,
+ })
+
+ ctx := compute.WithAllocator(context.Background(), mem)
+ out, err := table.ToRequestedSchema(ctx, requested,
fileSchema, rec, table.SchemaOptions{IncludeFieldIDs: true})
+ require.NoError(t, err)
+ defer out.Release()
+
+ want, err := table.TypeToArrowType(tt.icebergType,
true, false)
+ require.NoError(t, err)
+
+ got, ok := out.Column(0).(array.ExtensionArray)
+ require.True(t, ok, "expected extension array, got %T",
out.Column(0))
+ assert.True(t, arrow.TypeEqual(want, got.DataType()),
"expected: %s\ngot: %s", want, got.DataType())
+
+ storage := got.Storage()
+ require.Equal(t, len(values), storage.Len())
+
+ for i, v := range values {
+ if v == nil {
+ assert.True(t, storage.IsNull(i), "row
%d should be null", i)
+
+ continue
+ }
+ require.False(t, storage.IsNull(i), "row %d
should not be null", i)
+ assert.Equal(t, v, binaryValueAt(t, storage, i))
+ }
+ })
+ }
+}
+
+// roundTripWKBType returns the Arrow type a reader reconstructs for a column
+// written from icebergType, i.e. what a table sees reading its own files.
+func roundTripWKBType(t *testing.T, icebergType iceberg.Type)
arrow.ExtensionType {
+ t.Helper()
+
+ written, err := table.TypeToArrowType(icebergType, true, false)
+ require.NoError(t, err)
+
+ wkb, ok := written.(*geoarrow.WKBType)
+ require.True(t, ok, "expected geoarrow.wkb, got %T", written)
+
+ dt, err := geoarrow.NewWKBType().Deserialize(wkb.StorageType(),
wkb.Serialize())
+ require.NoError(t, err)
+
+ return dt
+}
+
+func TestToRequestedSchemaGeoSRID0Projection(t *testing.T) {
Review Comment:
This covers `srid:0` written by the new code and read straight back, which
is the easy direction. The case that regresses is the other one: a file with
absent CRS (what old iceberg-go wrote for a `srid:0` column) projected against
a `geometry(srid:0)` schema. I'd add a test pinning that path, either asserting
the promotion now succeeds (if we allow it) or documenting the failure, so the
back-compat behavior is deliberate rather than incidental.
##########
table/arrow_utils.go:
##########
@@ -2340,10 +2347,6 @@ func icebergCRSToGeoArrowMetadata(crs string, props
iceberg.Properties) (geoarro
if strings.HasPrefix(lowerCRS, "srid:") {
id := crs[len("srid:"):]
- if id == "0" {
- return geoarrow.NewMetadata(), nil // srid:0 maps to
omitted GeoArrow CRS
- }
-
raw, _ := json.Marshal(id) //nolint:errcheck // Marshalling a
string can't fail
Review Comment:
I think there's a subtle issue with what actually lands in the Parquet CRS
field here. Dropping the `srid:0` special case means
`icebergCRSToGeoArrowMetadata("srid:0")` now emits GeoArrow crs `"0"` with
crs_type srid, and when that serializes out, the Parquet GEOMETRY logical type
carries the bare string `"0"`.
The Parquet geospatial spec wants the full `"srid:0"` form with the prefix,
and arrow-rs keys its unknown-CRS sentinel off exactly `"srid:0"`: it won't
read `"0"` as SRID 0, it'll treat it as an opaque authority code. So we'd be
writing `srid:0` geometry other engines can't round-trip. I'd store the full
`"srid:<id>"` string in the GeoArrow CRS field so `ParquetCRS()` emits
`"srid:0"`. Can we confirm what `ParquetCRS()` produces here and pin it with a
Parquet-level round-trip?
##########
table/arrow_utils.go:
##########
@@ -2183,9 +2183,16 @@ func isWKT2CRSString(crs string) bool {
return false
}
+// defaultGeoCRS is the CRS assumed when a Parquet GEOMETRY or GEOGRAPHY
logical
+// type carries no CRS; it is also the default CRS of the Iceberg geo types.
+const defaultGeoCRS = "OGC:CRS84"
Review Comment:
iceberg already has an unexported `defaultGeoCRS = "OGC:CRS84"` over in
types.go, so this is a second copy of the same magic string with no compiler
link between them. I'd export `iceberg.DefaultGeoCRS` and use it here; the new
internal-test assertions hard-code `"OGC:CRS84"` too, same drift risk. Minor,
but it's the kind of thing that silently diverges.
##########
table/arrow_utils.go:
##########
@@ -2183,9 +2183,16 @@ func isWKT2CRSString(crs string) bool {
return false
}
+// defaultGeoCRS is the CRS assumed when a Parquet GEOMETRY or GEOGRAPHY
logical
+// type carries no CRS; it is also the default CRS of the Iceberg geo types.
+const defaultGeoCRS = "OGC:CRS84"
+
+// geoArrowCRSToIcebergCRS maps GeoArrow CRS metadata to an Iceberg CRS string.
+// Absent CRS metadata means the default CRS OGC:CRS84, matching the Parquet
+// geospatial spec and Iceberg's default geometry/geography types.
func geoArrowCRSToIcebergCRS(meta geoarrow.Metadata) (string, error) {
if len(meta.CRS) == 0 {
- return "srid:0", nil
+ return defaultGeoCRS, nil
Review Comment:
I think this changes the read result for files old iceberg-go already wrote.
Before, a `geometry(srid:0)` column serialized to absent GeoArrow CRS (via the
`srid:0` to omitted special case this PR also removes), and absent CRS read
back as `srid:0`, internally consistent. After this change, absent CRS reads as
plain `geometry`, so projecting one of those files against a `geometry(srid:0)`
table schema hits PromoteType's default branch and fails with `cannot promote
geometry to geometry(srid:0)`.
Blast radius is bounded, since that write path only shipped a few weeks ago,
but it's a silent read regression for anyone who wrote `srid:0` geometry in
that window. I'd allow `geometry(srid:0)` → `geometry` (and `geography(srid:0,
spherical)` → `geography`) as permitted promotions, since old iceberg-go used
`srid:0` as the stand-in for absent CRS anyway; at minimum I'd document the
migration. wdyt?
##########
table/arrow_utils_internal_test.go:
##########
@@ -495,11 +496,16 @@ func TestIcebergCRSToGeoArrowMetadata(t *testing.T) {
}
})
- t.Run("srid:0 maps to an omitted CRS", func(t *testing.T) {
+ // An omitted CRS means OGC:CRS84, so the unknown CRS srid:0 must be
explicit.
+ t.Run("srid:0 round trips as an explicit srid CRS", func(t *testing.T) {
meta, err := icebergCRSToGeoArrowMetadata("srid:0", nil)
require.NoError(t, err)
- assert.Empty(t, meta.CRS)
- assert.Empty(t, meta.CRSType)
+ assert.JSONEq(t, `"0"`, string(meta.CRS))
+ assert.Equal(t, geoarrow.CRSTypeSRID, meta.CRSType)
+
+ crs, err := geoArrowCRSToIcebergCRS(meta)
+ require.NoError(t, err)
+ assert.Equal(t, "srid:0", crs)
Review Comment:
This round-trips through the GeoArrow metadata but stops there: it never
serializes to a Parquet CRS string and reads it back, which is exactly where
the `"0"` vs `"srid:0"` question bites. It'd still pass even if what we write
to Parquet is `"0"` rather than the spec's `"srid:0"`. A round-trip that goes
iceberg type → Parquet CRS → iceberg type would pin the wire format we actually
care about.
--
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]
