You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Revanth14 (via GitHub)" <gi...@apache.org> on 2026/06/20 15:47:28 UTC
[PR] fix(table): harden GeoArrow CRS read parsing [iceberg-go]
Revanth14 opened a new pull request, #1239:
URL: https://github.com/apache/iceberg-go/pull/1239
closes #1231
## Summary
- Replace length-based string CRS acceptance with authority:code form matching.
- Reject PROJJSON-typed string CRS values on read.
- Return consistent WKT2:2019 unsupported errors and more specific JSON-object CRS parse errors.
## Testing
- go test ./...
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): harden GeoArrow CRS read parsing [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1239:
URL: https://github.com/apache/iceberg-go/pull/1239#discussion_r3470985237
##########
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:
Done. The internal tests now use `assert.ErrorIs` for both package-level sentinels: `errPROJJSONStringCRSNotSupported` and `errWKT2CRSNotSupported`. The external tests still use `ErrorContains` where the unexported sentinels are not visible.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): harden GeoArrow CRS read parsing [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1239:
URL: https://github.com/apache/iceberg-go/pull/1239#discussion_r3470978015
##########
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:
Done. I added an explicit `default:` arm with a comment that unset, `AuthorityCode`, `SRID`, and future `CRSType` values intentionally continue to the string-shape 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): harden GeoArrow CRS read parsing [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1239:
URL: https://github.com/apache/iceberg-go/pull/1239#discussion_r3449768639
##########
table/arrow_utils.go:
##########
@@ -1884,22 +1921,26 @@ func geoArrowCRSToIcebergCRS(meta geoarrow.Metadata) (string, error) {
return "", fmt.Errorf("invalid geoarrow CRS metadata: %w", err)
}
- if strings.EqualFold(crs, "OGC:CRS84") || strings.EqualFold(crs, "EPSG:4326") {
- return "OGC:CRS84", nil
- }
-
switch meta.CRSType {
case geoarrow.CRSTypeSRID:
return "srid:" + crs, nil
+ case geoarrow.CRSTypePROJJSON:
Review Comment:
I kept the rejection here intentionally.
`crs_type:"projjson"` says the CRS payload should be PROJJSON, so a string CRS with that annotation is invalid for this path. I agree it is a read-tolerance change from the old best-effort canonicalization, but this is one of the unsupported encodings the PR is meant to reject.
I kept that behavior covered with the `projjson` string CRS test, and I’ll call it out in the PR description so the compatibility change is explicit.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): harden GeoArrow CRS read parsing [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1239:
URL: https://github.com/apache/iceberg-go/pull/1239#discussion_r3470968771
##########
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:
Agreed, that inconsistency is fixed in the latest revision.
The canonical `OGC:CRS84` / `EPSG:4326` check now runs before the whole `crs_type` switch, so canonical strings win for `srid`, `projjson`, and `wkt2:2019` annotations alike. Non-canonical `projjson` / WKT2 strings still reject as unsupported.
I also restored the `geometry_epsg_4326_incorrect_type` canonicalizing case and added the `crs_type:"wkt2:2019"` sibling.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): harden GeoArrow CRS read parsing [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): harden GeoArrow CRS read parsing [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka merged PR #1239:
URL: https://github.com/apache/iceberg-go/pull/1239
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): harden GeoArrow CRS read parsing [iceberg-go]
Posted by "laskoviymishka (via GitHub)" <gi...@apache.org>.
laskoviymishka commented on code in PR #1239:
URL: https://github.com/apache/iceberg-go/pull/1239#discussion_r3449065763
##########
table/arrow_utils.go:
##########
@@ -1884,22 +1921,26 @@ func geoArrowCRSToIcebergCRS(meta geoarrow.Metadata) (string, error) {
return "", fmt.Errorf("invalid geoarrow CRS metadata: %w", err)
}
- if strings.EqualFold(crs, "OGC:CRS84") || strings.EqualFold(crs, "EPSG:4326") {
- return "OGC:CRS84", nil
- }
-
switch meta.CRSType {
case geoarrow.CRSTypeSRID:
return "srid:" + crs, nil
+ case geoarrow.CRSTypePROJJSON:
+ return "", errors.New("CRS type projjson not supported for string CRS")
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:
Moving this canonicalization below the `CRSType` switch quietly changes behavior for typed inputs.
Before this PR the OGC:CRS84/EPSG:4326 check ran first, so `{"crs":"EPSG:4326","crs_type":"srid"}` read back as `OGC:CRS84`. Now it hits the `CRSTypeSRID` case first and returns `srid:EPSG:4326` — an SRID-typed authority string that won't round-trip as a real SRID. Same shape for `{"crs":"OGC:CRS84","crs_type":"srid"}` -> `srid:OGC:CRS84`.
I'd put the canonical check back above the switch so it wins regardless of the annotated type. If we deliberately want the `CRSType` annotation to take precedence here, that's defensible too — but then it needs a test row asserting `srid:EPSG:4326` on purpose, since nothing covers the SRID + canonical-value combination today. wdyt?
##########
table/arrow_utils.go:
##########
@@ -1884,22 +1921,26 @@ func geoArrowCRSToIcebergCRS(meta geoarrow.Metadata) (string, error) {
return "", fmt.Errorf("invalid geoarrow CRS metadata: %w", err)
}
- if strings.EqualFold(crs, "OGC:CRS84") || strings.EqualFold(crs, "EPSG:4326") {
- return "OGC:CRS84", nil
- }
-
switch meta.CRSType {
case geoarrow.CRSTypeSRID:
return "srid:" + crs, nil
+ case geoarrow.CRSTypePROJJSON:
Review Comment:
This branch flips a previously-tolerated input into a hard read error — the same behavior the deleted `geometry_epsg_4326_incorrect_type` case used to exercise.
`{"crs":"EPSG:4326","crs_type":"projjson"}` is spec-invalid (projjson means the crs was written as PROJJSON, not as a string), but the old code accepted it best-effort via the CRS84/EPSG check and returned `OGC:CRS84`. A client that erroneously wrote that string now fails on read.
I lean toward erroring being the more correct behavior — but it's a read-tolerance regression that isn't called out in the description, and the schema-roundtrip case that pinned the old direction got deleted rather than converted. Either keep the CRS84/EPSG canonicalization ahead of this branch so canonical values still read, or keep the rejection and note it in the PR description so it's clearly intentional. Which way do you want 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): harden GeoArrow CRS read parsing [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1239:
URL: https://github.com/apache/iceberg-go/pull/1239#discussion_r3470983312
##########
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:
Done. I added a short comment above the passthrough return explaining that bare or unrecognized string CRS values are treated as opaque identifiers per the Iceberg type contract and preserved for read/write compatibility.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): harden GeoArrow CRS read parsing [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1239:
URL: https://github.com/apache/iceberg-go/pull/1239#discussion_r3470975158
##########
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:
Done. I added `COORDINATEMETADATA[`, `COORDINATEOPERATION[`, and `CRS[` to the WKT2 prefix list, and added coverage for those forms in `TestGeoArrowCRSToIcebergCRS`.
The helper comment now also calls out that this is intentionally best-effort and only used to return the clearer WKT2 unsupported error.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): harden GeoArrow CRS read parsing [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1239:
URL: https://github.com/apache/iceberg-go/pull/1239#discussion_r3470971747
##########
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:
Done, The canonical block is now immediately after the empty-string guard and before the `crs_type` switch, so it short-circuits for all annotations. I also removed the SRID special-casing comment since it’s no longer needed.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): harden GeoArrow CRS read parsing [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1239:
URL: https://github.com/apache/iceberg-go/pull/1239#discussion_r3449755160
##########
table/arrow_utils.go:
##########
@@ -1884,22 +1921,26 @@ func geoArrowCRSToIcebergCRS(meta geoarrow.Metadata) (string, error) {
return "", fmt.Errorf("invalid geoarrow CRS metadata: %w", err)
}
- if strings.EqualFold(crs, "OGC:CRS84") || strings.EqualFold(crs, "EPSG:4326") {
- return "OGC:CRS84", nil
- }
-
switch meta.CRSType {
case geoarrow.CRSTypeSRID:
return "srid:" + crs, nil
+ case geoarrow.CRSTypePROJJSON:
+ return "", errors.New("CRS type projjson not supported for string CRS")
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:
Good catch. Agreed, and addressed.
I moved SRID resolution after the canonical check, so the `OGC:CRS84` / `EPSG:4326` collapse wins even with `crs_type:"srid"`:
- `{"crs":"EPSG:4326","crs_type":"srid"}` → `OGC:CRS84`
- `{"crs":"OGC:CRS84","crs_type":"srid"}` → `OGC:CRS84`
- `{"crs":"3857","crs_type":"srid"}` → `srid:3857`
I added test coverage for both the canonical+SRID case and the numeric SRID case, and left a comment explaining why SRID is resolved last.
One intentional exception: `projjson`- and `wkt2`-typed strings still reject before canonicalization, since those encodings are unsupported by this path.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): harden GeoArrow CRS read parsing [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1239:
URL: https://github.com/apache/iceberg-go/pull/1239#discussion_r3470987610
##########
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:
Done. I restored `geometry_epsg_4326_incorrect_type` with the canonical `OGC:CRS84` expectation and added the sibling `{"crs":"EPSG:4326","crs_type":"wkt2:2019"}` case to pin the same canonical-first behavior.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): harden GeoArrow CRS read parsing [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on code in PR #1239:
URL: https://github.com/apache/iceberg-go/pull/1239#discussion_r3470986503
##########
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:
I removed the defensive `require.NoError` from `stringCRS` and now ignore the marshal error with a short `nolint` comment, since marshaling a string cannot fail.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): harden GeoArrow CRS read parsing [iceberg-go]
Posted by "Revanth14 (via GitHub)" <gi...@apache.org>.
Revanth14 commented on PR #1239:
URL: https://github.com/apache/iceberg-go/pull/1239#issuecomment-4760245858
> Nice hardening — the per-member JSON id errors are a real readability win, and the projjson-string rejection is good to have.
>
> One thing I want to make sure we've thought through: the new authority:code regex at arrow_utils.go:1939 is now the only accepted shape for a string CRS on read, but the write side doesn't enforce it. icebergCRSToGeoArrowMetadata (arrow_utils.go:2050) serializes any string, and GeometryTypeOf/GeographyTypeOf (types.go:847, :907) accept any non-empty CRS with no shape check. So something like geometry("my-custom-crs") writes fine today but fails to read back after this change — which the old `len(crs) <= 32` branch used to allow. That makes it a forward-compat break for tables already written with a free-form CRS string. Is the plan to also tighten the constructor/write path so those values can't be produced, or should read stay lenient for already-written data? Right now read and write disagree on what's valid.
@tanmayrauth thanks for the review.
Good catch. This does create a round-trip break: `geometry("my-custom-crs")` can be written today, but the new read path rejects it.
I think read should stay lenient for non-empty string CRS values, since tightening write would not help existing data and would still leave constructors more permissive than the writer. We can still reject `projjson` string CRS and WKT2, since those are unsupported.
Will wait for @laskoviymishka thoughts before updating. If that direction sounds right, I’ll add a round-trip regression test for `GeometryTypeOf("my-custom-crs")`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org