Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Potential JSON Schema issues #129

Closed
9 tasks done
m-mohr opened this issue Oct 25, 2022 · 4 comments · Fixed by #131
Closed
9 tasks done

Potential JSON Schema issues #129

m-mohr opened this issue Oct 25, 2022 · 4 comments · Fixed by #131
Assignees
Milestone

Comments

@m-mohr
Copy link
Collaborator

m-mohr commented Oct 25, 2022

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

  • $defs keyword in the schema was only added in draft 2019-09 (see also Update JSON Schema version? #127 )
  • additionalProperties: true is the default and could be removed
  • "patternProperties": with a pattern ".*" is basically equivalent to additionalProperties and thus can be simplified a bit.
  • "enum": ["WKB"] is equivalent to "const": "WKB", the same applies to "enum": ["counterclockwise"]
  • PROJJSON has a new schema version, 0.5
  • 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
  • By the way, the spec says double for epoch, but this is not enforced in the schema and could be an int (or should this be "number" in the spec?)
  • I understand the spec that you can either specify an array of geometry types or a single geometry type as string or Unknown (so always as a string, not in an array). If this is the case, the schema doesn't check it. ["Polygon", "Unknown"] is valid.
  • The Z suffix to the geometry types is not part of the JSON Schema it seems

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

@jorisvandenbossche
Copy link
Collaborator

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 #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
Copy link
Collaborator Author

m-mohr commented Oct 25, 2022

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 cholmes added this to the 1.0.0-beta.1 milestone Nov 7, 2022
@cholmes
Copy link
Member

cholmes commented Nov 7, 2022

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
Copy link
Collaborator

tschaub commented Nov 7, 2022

#140 proposes changes to the geometry types.

cholmes added a commit that referenced this issue Dec 5, 2022
* Require minimum length of 1 for primary_geometry #129

* Update format-specs/geoparquet.md

Co-authored-by: Chris Holmes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants