twuebi commented on PR #1634:
URL: https://github.com/apache/iceberg-go/pull/1634#issuecomment-5182512360
Thanks, dug into all three.
**`"0"` vs `"srid:0"`:** we don't write a Parquet CRS field at all right
now. arrow-go v18.7.0 has no GEOMETRY/GEOGRAPHY logical type in its schema
layer (only the thrift structs in
`parquet/internal/gen-go/parquet/parquet.go`), so a geo column goes out as
plain `BYTE_ARRAY` with logical type `None`, and the CRS travels in the
GeoArrow metadata of the stored `ARROW:schema`. I verified that against the
real write path (`table/internal/parquet_files.go`, pqarrow with
`WithStoreSchema`). Storing `"srid:0"` in the GeoArrow `crs` field would also
double-prefix on our own read path — `geoArrowCRSToIcebergCRS` prepends `srid:`
for `crs_type: srid`, giving `geometry(srid:srid:0)`, which is the arrow-rs bug
you mention at the end — and it would contradict `srid:4326` →
`{"crs":"4326","crs_type":"srid"}`, which predates this PR, plus GeoArrow's
definition of `crs_type: srid` as an opaque identifier. So
`{"crs":"0","crs_type":"srid"}` is the right GeoArrow form; the prefixed
`srid:<id>
` spelling belongs to whoever emits the Parquet logical type.
I added the round-trip you asked for, at the level that exists today:
`TestGeoTypeParquetRoundTrip` writes a real Parquet file per CRS shape, pins
the GeoArrow metadata, pins that the column logical type is still `None` (so we
get a failing test to revisit the CRS spelling the moment arrow-go starts
emitting GEOMETRY), then reads back through `ArrowSchemaToIceberg` and pins the
iceberg type. `geometry(srid:0)` and `geography(srid:0)` survive.
**Back-compat:** narrower than it looks — the old `srid:0` → omitted
encoding landed in 38d0d12 and is in no release: `git tag --contains 38d0d12`
is empty and the newest tag, v0.6.0, predates it. Only builds from main in the
last few weeks can have written such files. Also, the case you describe needs
`geometry` → `geometry(srid:0)`, not `geometry(srid:0)` → `geometry`: the file
now reads as the default CRS and the table declares `srid:0`. Either direction
is a CRS promotion, which isn't in the spec's promotion list that `PromoteType`
implements, so I'd rather not add one unilaterally — it would diverge from
other engines and mask genuinely mismatched CRS. I added
`TestToRequestedSchemaGeoAbsentCRSAgainstSRID0SchemaFails` so the rejection is
deliberate rather than incidental.
**Constant:** exported `iceberg.DefaultGeoCRS` and switched both `types.go`
and `table/arrow_utils.go` to it, so the second copy is gone. I kept the
literal `"OGC:CRS84"` in the test assertions on purpose — an assertion written
against the constant under test can't catch a change to that constant.
**Read-side `srid:srid:0`:** agreed, and it turned out to be a two-line
prefix check in the same spot, so I folded it in rather than deferring: with
`crs_type: srid`, a `crs` value that already starts with `srid:` is taken
as-is. Pinned both at the CRS-mapping level (`srid:0`, `srid:3857`,
`SRID:3857`) and end to end, where `{"crs":"srid:0","crs_type":"srid"}` now
reads as `geometry(srid:0)` instead of `geometry(srid:srid:0)`.
--
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]
