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

Clean-up spec and JSON Schema #131

Merged
merged 7 commits into from
Dec 5, 2022
Merged

Clean-up spec and JSON Schema #131

merged 7 commits into from
Dec 5, 2022

Conversation

m-mohr
Copy link
Collaborator

@m-mohr m-mohr commented Oct 25, 2022

Fixes #129, should include not functional changes.

Changes:

  1. Updated the datatype for the columns key from key to string in the written spec as "key" is not a datatype
  2. Added the missing null type to the crs field in the written spec (alignment with JSON Schema)
  3. Minor text and/or formatting changes without functional changes.
  4. Removed all descriptions from the JSON Schema. It seems they were not updated everytimg when changes were applied to the spec. To mitigate the risk of differences between the written spec and the schema, I'd recommend to not duplicate descriptions in the schema. They are annotations only anyway and are not used for validation.
  5. Moved required fields up so that they are located before the properties, which makes it easier to understand the requirements
  6. Keep annotation-style fields together for easier understanding.
  7. Replaced patternProperties pattern ".*" with ".+" to not allow empty column names (see Require minimum length of 1 for primary_geometry #129 #153)
  8. Removed all additionalProperties: true as they are the default anyway
  9. Replaced enums with a single value with const
  10. Don't allow additional columns that are invalid

Related PRs: #151, #153

format-specs/geoparquet.md Outdated Show resolved Hide resolved
format-specs/schema.json Outdated Show resolved Hide resolved
format-specs/schema.json Outdated Show resolved Hide resolved
format-specs/schema.json Outdated Show resolved Hide resolved
@m-mohr m-mohr mentioned this pull request Oct 25, 2022
9 tasks
Copy link
Collaborator

@kylebarron kylebarron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of good changes (the STAC experience shows!) though I think it would be easier to review/discuss if split into a few different PRs.

format-specs/schema.json Outdated Show resolved Hide resolved
@m-mohr
Copy link
Collaborator Author

m-mohr commented Oct 25, 2022

@kylebaron The issue is the indentation change for patternProperties/additionalProperties. Makes it a bit hard to compare in the underwhelming GitHub diff tool. Maybe open it in a another diff tool?

@kylebarron
Copy link
Collaborator

kylebarron commented Oct 25, 2022

@kylebaron The issue is the indentation change for patternProperties/additionalProperties.

I was referring more towards:

  • Whether to declare Z in the geom type; whether to use a regex there
  • Whether crs should allow null
  • Exactly how bbox should be defined in the schema

I think it would be clearer for each to be their own PR with their own discussion

format-specs/schema.json Outdated Show resolved Hide resolved
@m-mohr
Copy link
Collaborator Author

m-mohr commented Nov 7, 2022

I'll rebase this after #88 and #140 have been merged.

# Conflicts:
#	format-specs/geoparquet.md
#	format-specs/schema.json
@m-mohr m-mohr changed the title Update JSON Schema and data types #128, 129 Clean-up spec and JSON Schema Dec 5, 2022
@m-mohr
Copy link
Collaborator Author

m-mohr commented Dec 5, 2022

Updated to reflect the latest changes. See the initial post for a list of changes.

@m-mohr
Copy link
Collaborator Author

m-mohr commented Dec 5, 2022

Two additional changes, see also #141 (comment):

  1. Replaced patternProperties pattern ".*" with ".+" to not allow empty column names (see Require minimum length of 1 for primary_geometry #129 #153)
  2. Don't allow additional columns that are invalid

format-specs/geoparquet.md Outdated Show resolved Hide resolved
Co-authored-by: Matthias Mohr <webmaster@mamo-net.de>
@tschaub tschaub merged commit 7a7826f into opengeospatial:main Dec 5, 2022
@m-mohr
Copy link
Collaborator Author

m-mohr commented Dec 5, 2022

Wasn't this meant to wait for #141 ?

@m-mohr m-mohr deleted the json-schema branch December 5, 2022 22:24
@tschaub
Copy link
Collaborator

tschaub commented Dec 5, 2022

My bad. I thought I understood that this was going to come first. I had just read the comment here #141 (comment) and thought that the tests were going to be adapted after this was in.

@jorisvandenbossche - Apologies for complicating things if the tests don't pass with these changes.

@jorisvandenbossche
Copy link
Collaborator

No problem! The tests are also still passing, so no complication at all ;)

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 this pull request may close these issues.

Potential JSON Schema issues
5 participants