laskoviymishka commented on code in PR #1239:
URL: https://github.com/apache/iceberg-go/pull/1239#discussion_r3461414613
##########
table/arrow_utils.go:
##########
@@ -1871,6 +1871,46 @@ func checkCRSJSON(rawCrs json.RawMessage) bool {
return len(b) > 0 && b[0] == '{'
}
+// errWKT2CRSNotSupported is returned for WKT2:2019 CRS definitions, which this
+// package does not support yet.
+var errWKT2CRSNotSupported = errors.New("CRS type wkt2:2019 not supported")
+
+// errPROJJSONStringCRSNotSupported is returned for projjson-typed string CRS
+// values, which this package does not support yet.
+var errPROJJSONStringCRSNotSupported = errors.New("CRS type projjson not
supported for string CRS")
+
+// isWKT2CRSString reports whether crs looks like a WKT2 (ISO 19162) CRS
+// definition. It is a best-effort heuristic used only to surface the clearer
+// errWKT2CRSNotSupported message. The list covers the WKT2:2019 CRS keywords
+// and their long forms.
+func isWKT2CRSString(crs string) bool {
+ upper := strings.ToUpper(strings.TrimSpace(crs))
+
+ for _, prefix := range []string{
+ "BOUNDCRS[",
+ "COMPOUNDCRS[",
+ "DERIVEDPROJCRS[",
+ "ENGCRS[",
+ "ENGINEERINGCRS[",
+ "GEODCRS[",
+ "GEODETICCRS[",
+ "GEOGCRS[",
Review Comment:
The keyword set is missing a few valid WKT2:2019 top-level forms —
`COORDINATEMETADATA[`, `COORDINATEOPERATION[`, and the abstract `CRS[` wrapper
— so a string wrapped in those passes the heuristic and gets stored as an
opaque CRS instead of hitting the clearer error. I'd add them.
While we're here: this is a best-effort prefix match, so it'll also fire on
a non-WKT2 authority string that happens to start with one of these tokens —
worth a one-line comment that it's intentionally heuristic and only used to
pick the friendlier error.
##########
table/arrow_utils.go:
##########
@@ -1883,23 +1923,32 @@ func geoArrowCRSToIcebergCRS(meta geoarrow.Metadata)
(string, error) {
if err := json.Unmarshal(meta.CRS, &crs); err != nil {
return "", fmt.Errorf("invalid geoarrow CRS metadata:
%w", err)
}
-
- if strings.EqualFold(crs, "OGC:CRS84") ||
strings.EqualFold(crs, "EPSG:4326") {
- return "OGC:CRS84", nil
+ if crs == "" {
+ return "", errors.New("unsupported CRS: empty string
CRS")
}
switch meta.CRSType {
- case geoarrow.CRSTypeSRID:
- return "srid:" + crs, nil
+ case geoarrow.CRSTypePROJJSON:
Review Comment:
I think there's a subtle inconsistency here. The SRID arm was deliberately
moved below the canonical check (with the comment a few lines down) so that
`EPSG:4326 + crs_type=srid` still canonicalizes to `OGC:CRS84` rather than
becoming `srid:EPSG:4326`. But projjson and wkt2:2019 fire *before* the
canonical check, so `{"crs":"EPSG:4326","crs_type":"projjson"}` and
`{"crs":"EPSG:4326","crs_type":"wkt2:2019"}` both error — that's exactly the
case the deleted `geometry_epsg_4326_incorrect_type` test used to cover.
A canonical authority code is unambiguous regardless of a bogus `crs_type`,
so I'd apply the same rule you already applied to SRID: run the
`OGC:CRS84`/`EPSG:4326` check first, then let projjson/wkt2:2019 reject only
non-canonical strings. That makes all four arms consistent. wdyt?
##########
table/arrow_utils.go:
##########
@@ -1883,23 +1923,32 @@ func geoArrowCRSToIcebergCRS(meta geoarrow.Metadata)
(string, error) {
if err := json.Unmarshal(meta.CRS, &crs); err != nil {
return "", fmt.Errorf("invalid geoarrow CRS metadata:
%w", err)
}
-
- if strings.EqualFold(crs, "OGC:CRS84") ||
strings.EqualFold(crs, "EPSG:4326") {
- return "OGC:CRS84", nil
+ if crs == "" {
+ return "", errors.New("unsupported CRS: empty string
CRS")
}
switch meta.CRSType {
- case geoarrow.CRSTypeSRID:
- return "srid:" + crs, nil
+ case geoarrow.CRSTypePROJJSON:
+ return "", errPROJJSONStringCRSNotSupported
case geoarrow.CRSTypeWKT22019:
- return "", errors.New("CRS type wkt2:2019 not
supported")
- default:
- if len(crs) <= 32 {
- return crs, nil
- }
+ return "", errWKT2CRSNotSupported
+ }
- return "", errors.New("crs length too long")
+ if strings.EqualFold(crs, "OGC:CRS84") ||
strings.EqualFold(crs, "EPSG:4326") {
Review Comment:
This is the move I'd make to fix the ordering above: lift this canonical
block to the very top of the string case, right after the empty-string guard
and before `switch meta.CRSType`. Once it short-circuits first, the projjson
and wkt2:2019 arms can stay as-is and the SRID special-casing comment below
becomes unnecessary, since canonical will already have returned.
##########
table/arrow_utils_internal_test.go:
##########
@@ -491,3 +492,128 @@ func TestIcebergCRSToGeoArrowMetadata(t *testing.T) {
assert.Empty(t, meta.CRSType)
})
}
+
+func TestGeoArrowCRSToIcebergCRS(t *testing.T) {
+ stringCRS := func(s string) json.RawMessage {
Review Comment:
`stringCRS` closes over the outer `t`, so a `require.NoError` failure inside
a subtest gets reported against the parent. `json.Marshal` of a string can't
fail, so I'd just drop the error check (or take a `testing.TB` and call
`tb.Helper()` if you want to keep it defensive).
##########
table/arrow_utils_internal_test.go:
##########
@@ -491,3 +492,128 @@ func TestIcebergCRSToGeoArrowMetadata(t *testing.T) {
assert.Empty(t, meta.CRSType)
})
}
+
+func TestGeoArrowCRSToIcebergCRS(t *testing.T) {
+ stringCRS := func(s string) json.RawMessage {
+ raw, err := json.Marshal(s)
+ require.NoError(t, err)
+
+ return raw
+ }
+
+ t.Run("accepts long authority code string", func(t *testing.T) {
+ const crs = "EPSGAuthorityLongName:CODE-12345678901234567890"
+
+ got, err := geoArrowCRSToIcebergCRS(geoarrow.Metadata{CRS:
stringCRS(crs)})
+ require.NoError(t, err)
+ assert.Equal(t, crs, got)
+ })
+
+ t.Run("accepts arbitrary string CRS", func(t *testing.T) {
+ got, err := geoArrowCRSToIcebergCRS(geoarrow.Metadata{CRS:
stringCRS("custom")})
+ require.NoError(t, err)
+ assert.Equal(t, "custom", got)
+ })
+
+ t.Run("canonical string CRS wins over srid type annotation", func(t
*testing.T) {
+ for _, crs := range []string{"EPSG:4326", "OGC:CRS84"} {
+ t.Run(crs, func(t *testing.T) {
+ got, err :=
geoArrowCRSToIcebergCRS(geoarrow.Metadata{
+ CRS: stringCRS(crs),
+ CRSType: geoarrow.CRSTypeSRID,
+ })
+ require.NoError(t, err)
+ assert.Equal(t, "OGC:CRS84", got)
+ })
+ }
+ })
+
+ t.Run("numeric srid type annotation maps to srid", func(t *testing.T) {
+ got, err := geoArrowCRSToIcebergCRS(geoarrow.Metadata{
+ CRS: stringCRS("3857"),
+ CRSType: geoarrow.CRSTypeSRID,
+ })
+ require.NoError(t, err)
+ assert.Equal(t, "srid:3857", got)
+ })
+
+ t.Run("rejects empty string CRS", func(t *testing.T) {
+ _, err := geoArrowCRSToIcebergCRS(geoarrow.Metadata{CRS:
stringCRS("")})
+ require.Error(t, err)
+ assert.ErrorContains(t, err, "unsupported CRS: empty string
CRS")
+ })
+
+ t.Run("rejects projjson string CRS", func(t *testing.T) {
+ _, err := geoArrowCRSToIcebergCRS(geoarrow.Metadata{
+ CRS: stringCRS("EPSG:4326"),
+ CRSType: geoarrow.CRSTypePROJJSON,
+ })
+ require.Error(t, err)
+ assert.ErrorContains(t, err, "CRS type projjson not supported
for string CRS")
Review Comment:
These two are package-level sentinels (`errPROJJSONStringCRSNotSupported`
here and `errWKT2CRSNotSupported` just below), and this is the internal test,
so it can reach them — I'd assert with `assert.ErrorIs(t, err,
errPROJJSONStringCRSNotSupported)` rather than matching the message text. Keeps
the test from breaking on wording changes and actually pins the sentinel. (The
external `arrow_utils_test.go` can't see the unexported vars, so
`ErrorContains` is correct there.)
##########
table/arrow_utils.go:
##########
@@ -1883,23 +1923,32 @@ func geoArrowCRSToIcebergCRS(meta geoarrow.Metadata)
(string, error) {
if err := json.Unmarshal(meta.CRS, &crs); err != nil {
return "", fmt.Errorf("invalid geoarrow CRS metadata:
%w", err)
}
-
- if strings.EqualFold(crs, "OGC:CRS84") ||
strings.EqualFold(crs, "EPSG:4326") {
- return "OGC:CRS84", nil
+ if crs == "" {
+ return "", errors.New("unsupported CRS: empty string
CRS")
}
switch meta.CRSType {
- case geoarrow.CRSTypeSRID:
- return "srid:" + crs, nil
+ case geoarrow.CRSTypePROJJSON:
+ return "", errPROJJSONStringCRSNotSupported
case geoarrow.CRSTypeWKT22019:
- return "", errors.New("CRS type wkt2:2019 not
supported")
- default:
- if len(crs) <= 32 {
- return crs, nil
- }
+ return "", errWKT2CRSNotSupported
+ }
- return "", errors.New("crs length too long")
+ if strings.EqualFold(crs, "OGC:CRS84") ||
strings.EqualFold(crs, "EPSG:4326") {
+ return "OGC:CRS84", nil
+ }
+ if isWKT2CRSString(crs) {
+ return "", errWKT2CRSNotSupported
}
+ // SRID is resolved after the canonical and WKT2 checks so a
+ // contradictory crs_type=srid on a value like "EPSG:4326" still
+ // canonicalizes rather than producing "srid:EPSG:4326". Real
numeric
+ // SRIDs match none of the checks above and fall through to
here.
+ if meta.CRSType == geoarrow.CRSTypeSRID {
+ return "srid:" + crs, nil
+ }
+
+ return crs, nil
Review Comment:
This final `return crs, nil` is the catch-all passthrough for AuthorityCode
and unset types, but that policy is implicit. One line — something like
"unrecognized type or bare string: accept as opaque CRS per spec (any unique
identifier is valid)" — would save the next reader the trace-through.
##########
table/arrow_utils_test.go:
##########
@@ -608,11 +615,6 @@ func TestIcebergGeoTypesToArrowSchema(t *testing.T) {
ice: defaultGeometry,
geoarrowMetaJSON: `{"crs":"epsg:4326"}`,
},
- {
- name: "geometry_epsg_4326_incorrect_type",
Review Comment:
Once the canonical-first reordering lands, this case should come back with
its original canonicalizing assertion (`OGC:CRS84`) — it's the regression guard
for the exact ordering bug. I'd also add the sibling
`{"crs":"EPSG:4326","crs_type":"wkt2:2019"}` case, since that path is currently
untested and is where the expected behavior is easiest to get wrong.
##########
table/arrow_utils.go:
##########
@@ -1883,23 +1923,32 @@ func geoArrowCRSToIcebergCRS(meta geoarrow.Metadata)
(string, error) {
if err := json.Unmarshal(meta.CRS, &crs); err != nil {
return "", fmt.Errorf("invalid geoarrow CRS metadata:
%w", err)
}
-
- if strings.EqualFold(crs, "OGC:CRS84") ||
strings.EqualFold(crs, "EPSG:4326") {
- return "OGC:CRS84", nil
+ if crs == "" {
+ return "", errors.New("unsupported CRS: empty string
CRS")
}
switch meta.CRSType {
- case geoarrow.CRSTypeSRID:
- return "srid:" + crs, nil
+ case geoarrow.CRSTypePROJJSON:
+ return "", errPROJJSONStringCRSNotSupported
case geoarrow.CRSTypeWKT22019:
- return "", errors.New("CRS type wkt2:2019 not
supported")
- default:
- if len(crs) <= 32 {
- return crs, nil
- }
+ return "", errWKT2CRSNotSupported
+ }
Review Comment:
This switch handles two of the four `CRSType` constants with no `default`,
which the exhaustive linter will flag, and means a future fifth constant
silently falls through to `return crs, nil`. I'd add an explicit `default:`
with a short comment that SRID/AuthorityCode/unset are intentionally handled by
the checks below.
--
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]
