opengeospatial / geoparquet

Specification for storing geospatial vector data (point, line, polygon) in Parquet
https://geoparquet.org
Apache License 2.0
795 stars 56 forks source link

Potential JSON Schema issues #129

Closed m-mohr closed 1 year ago

m-mohr commented 1 year ago

Some potential improvements and/or issues in the JSON Schema:

Happy to work on PRs, just let me know whether all this is correctly captured.

jorisvandenbossche commented 1 year ago

I am not very familiar with JSON schema details, but that all sounds correct from the geoparquet side.

The bbox schema looks weird. Are more than 4 values allowed? Right now it would allow [0,1,2,3,"asdsad"]. Could probably also be simplified using min/maxItems... I think you were looking for something like "items": {"type": "number"}, "minItems": 4

I am currently updating this field in https://github.com/opengeospatial/geoparquet/pull/88, so your feedback there is certainly welcome! (we are basically already doing something with items and minItems now, just spelling out the 4 or 6 values explicitly)

"enum": ["WKB"] is equivalent to "const": "WKB", the same applies to "enum": ["counterclockwise"]

Although it is not the case right now, but we are planning to add more options in the future, so can maybe keep it like that? (although also the enum wouldn't allow any other value right now, so it wouldn't really matter)

m-mohr commented 1 year ago

Okay, I'll draft a PR. Any thoughts on #127 before I start? Edit: Opened PR #131

I am currently updating this field in #88, so your feedback there is certainly welcome!

Done.

"enum": ["WKB"] is equivalent to "const": "WKB", the same applies to "enum": ["counterclockwise"]

Although it is not the case right now, but we are planning to add more options in the future, so can maybe keep it like that? (although also the enum wouldn't allow any other value right now, so it wouldn't really matter)

You can always change it back to enum once you've decided to add a new option. :-)

cholmes commented 1 year ago

Call 11/7 - Merging this should block on #134, just to make sure it's doing what we think it's doing.

It'd also be good to break up #131 into multiple PR's. @tschaub will do the geometry updates in a new PR.

tschaub commented 1 year ago

140 proposes changes to the geometry types.