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

Remove requirement that geometry columns must not be a group field #234

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

tschaub
Copy link
Collaborator

@tschaub tschaub commented Jun 20, 2024

The new native geometry encodings from #189 are incompatible with the requirement that geometry columns must not be group fields. This branch suggests removing that requirement.

Fixes #233.

@cholmes
Copy link
Member

cholmes commented Jun 20, 2024

Good catch, though do we want to completely throw out the rule? Like we probably still don't want geometries nested in a group? And we could also just say that the only groupings allowed are the native geometries...

I guess with this we'll probably need a 1.1.1?

@tschaub
Copy link
Collaborator Author

tschaub commented Jun 20, 2024

Good catch, though do we want to completely throw out the rule? Like we probably still don't want geometries nested in a group? And we could also just say that the only groupings allowed are the native geometries...

I think this might be a case where having more language in the spec is just going to create confusion.

With the change I've proposed, our one requirement related to nesting/groups is that all geometry columns must be at the root of the schema. This implies (and is the same thing as saying) that geometry columns cannot be nested within other group fields. So I wouldn't add any additional words related to that.

The encoding section already has a lot of language about how the encoding changes the structure of the geometry field, so I'm not sure more language is needed above.

Basically, I think lots of language invites lots of differing interpretations. And with fewer words we risk fewer inconsistencies/misinterpretations/bugs etc.

I guess with this we'll probably need a 1.1.1?

Yeah, I think that makes sense.

@cholmes
Copy link
Member

cholmes commented Jun 20, 2024

Cool, makes sense to me.

Let's try to get this out soon, but ideally get a few more eyes on this PR.

@jorisvandenbossche
Copy link
Collaborator

I guess with this we'll probably need a 1.1.1?

It's actually something we haven't discussed (or had to discuss) before, but do we want a new version number for something like this? Because it is not fixing something in the spec (in the sense of fixing something in implementations / files), but only fixing a wording in the spec.
And getting this fixed wording out (in the rendered spec on the website) is definitely good, but do we then actually want files with a version number of "1.1.1" in their metadata?

Copy link
Collaborator

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

The change itself certainly looks good, thanks for catching that inconsistency!

@cholmes
Copy link
Member

cholmes commented Jun 20, 2024

It's actually something we haven't discussed (or had to discuss) before, but do we want a new version number for something like this? Because it is not fixing something in the spec (in the sense of fixing something in implementations / files), but only fixing a wording in the spec.

Yeah, I think I could go either way - curious what others think. I agree it seems excessive to have implementations update just a version number when nothing really changes. On the other hand if this was written more formality like an OGC specification I do think this would be a change to the requirements / test cases / validators - indeed I think this came up since we didn't actually have any validators try to test 1.1 before we released. In the OGC version of the spec in requirement 2 it does say 'A geometry SHALL NOT be a group field or nested in a group.' So in that case it's not just wording, but it's changing one of the 'requirements'.

At this point I probably lean slightly towards finding a way to not release a new version, since we have not actually adopted the OGC style of doing things, so there's a case to be made 'it's just wording'. But I'm not sure what we actually do then? Just merge this change to 1.2.0-dev and have 1.1.0 spec not have it? Update 1.1.0 tag and the published version on https://geoparquet.org/releases/v1.1.0/ to include this change? Something else?

@cholmes
Copy link
Member

cholmes commented Jun 21, 2024

I guess one option would be to cut a 1.1.1 'release' on github (so there's a tag and the website gets updated), but keep the version the same?

Could even call it 1.1.0.1 on github / spec? Or call it 1.1.0-update or something?

@kylebarron
Copy link
Collaborator

kylebarron commented Jun 21, 2024

In Python there's also the idea of "post releases" https://packaging.python.org/en/latest/specifications/version-specifiers/#post-releases

Some projects use post-releases to address minor errors in a final release that do not affect the distributed software (for example, correcting an error in the release notes).

@cholmes
Copy link
Member

cholmes commented Jun 21, 2024

Oh nice, I like that. Good to use some precedent.

@cholmes cholmes merged commit 6a6a769 into opengeospatial:main Jun 21, 2024
2 checks passed
@cholmes
Copy link
Member

cholmes commented Jun 21, 2024

Merging in, since no matter what we need this wording in. I'm going to try to run with the 'post release' idea and make a branch to propose it, as I'd like to get this wrapped up soon.

@tschaub
Copy link
Collaborator Author

tschaub commented Jun 21, 2024

@cholmes - with Semver this would be handled with "build metadata" following the version number - so 1.1.0+P1 would be a version that would follow 1.1.0 and precede 1.2.0 (or 1.1.1 if we decided we wanted that later). The letter P is arbitrary, but we could decide that we want to use P build metadata to indicate our post-release fixes.

We could make some changes to the way the website is built to only show the most recent release considering build metadata.

Please don't follow the version naming convention from Python.

@tschaub tschaub deleted the allow-group branch June 21, 2024 20:43
@cholmes cholmes mentioned this pull request Jun 21, 2024
@tschaub
Copy link
Collaborator Author

tschaub commented Jun 21, 2024

Here is a summary of the latest tag and build metadata handling: #236 (comment)

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.

Geometry nesting requirement is incompatible with "native" encoding
5 participants