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: [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]
