Open michaelkirk opened 2 years ago
I think, my first review comment is still relevant. Forcing the output of any FeatureCollection
to a GeometryCollection
seems not right to me.
I think, my first review comment is still relevant. Forcing the output of any FeatureCollection to a GeometryCollection seems not right to me.
What output are you expecting when mapping a feature collection to a geometry-only format like WKT?
Here's the current behavior of geo-types:
let geojson_string = serde_json::json!({
"type": "FeatureCollection",
"features": [
{
"type": "Feature",
"properties": {},
"geometry": {
"type": "Point",
"coordinates": [1,2]
}
},
{
"type": "Feature",
"properties": {},
"geometry": {
"type": "LineString",
"coordinates": [[1,2], [3, 4]]
}
},
]
}).to_string();
let geojson = crate::geojson::GeoJson(&geojson_string);
let expected = geo_types::Geometry::GeometryCollection(geo_types::GeometryCollection(vec![
point! { x: 1.0, y: 2.0 }.into(),
line_string![(x: 1.0, y: 2.0), (x: 3.0, y: 4.0)].into()
]));
assert_eq!(expected, geojson.to_geo().unwrap());
It seems reasonable to me. geo-types similarly used to behave incorrectly for multiple geometries. It confused people (https://github.com/georust/geozero/issues/11). For geo-types, the fix was: since multiple geometries cannot exist at the top-level they must be wrapped in a GeometryCollection. I'm proposing the same fix here.
The question is not whether the output format is geometry only, but whether multiple features are supported. If you e.g. use the GDAL CSV driver with WKT geometries (ogr2ogr -f CSV -lco "GEOMETRY=AS_WKT" fc.csv fc.json
) you get two records:
WKT,
"POINT (1 2)"
"LINESTRING (1 2,3 4)"
If you want to collect the geometries of all features, then a GeometryCollection
is the right thing. But when you convert a GeoJSON FeatureCollection
to e.g. FlatGeoBuf you expect the original geometry types and not a GeometryCollection
.
I'm not sure if this is controversial - and it's definitely a bit nuanced. Let me know what you think!
The goal: Allow WKT to emit a single top level item - multiple top level WKT items like
POINT(1 1), POINT(2 2)
are not parseable by e.g. postgis or georust/wkt (we removed support in https://github.com/georust/wkt/pull/72)And this change also allows for lossless roundtripping of geojson->fgb->geojson, see https://github.com/flatgeobuf/flatgeobuf/compare/master...michaelkirk:mkirk/rust-lossless-geojson-collections?expand=1