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

Add optional "geometry_type" field per column #51

Merged
merged 5 commits into from Apr 18, 2022

Conversation

jorisvandenbossche
Copy link
Collaborator

Closes #41

@cholmes
Copy link
Member

cholmes commented Apr 11, 2022

@jorisvandenbossche - what's needed to finalize this PR? It's still marked 'draft'. Would be great to get this in for 0.2.0.

@Jesus89
Copy link
Collaborator

Jesus89 commented Apr 12, 2022

In this old version of "GeoParquet" (link) there is a similar approach for storing the types. In that case, it uses geometry_types in plural so it always will be an array.

I think it's fine having both string and array but wanted to share the info, just in case.

BTW, I miss the implementation of the field in the example. Let me know if you want me to add it.

@jorisvandenbossche
Copy link
Collaborator Author

what's needed to finalize this PR?

I think mostly some more feedback on what I actually added here / the open questions in #41

@jorisvandenbossche jorisvandenbossche marked this pull request as ready for review April 15, 2022 18:42
@cholmes
Copy link
Member

cholmes commented Apr 15, 2022

I think mostly some more feedback on what I actually added here / the open questions in #41

Ah, got it. I see - this currently does have it as an optional field and Unknown as an option. I do agree with your comment that we should pick one so there's not two ways to say unknown. I lean towards required and allow unknown. Follows flatgeobuf.

Copy link
Collaborator

@alasarr alasarr left a comment

Choose a reason for hiding this comment

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

👏 Super happy to see this new field. Thank you so much @jorisvandenbossche and @rouault! I've faced this last week and a few minutes ago with multiple use cases where it's required.

Today: The tilers at CARTP need to guess the type of geometry, right now we need to add a query over the data of the user to determine the type of geometry (using samples and some heuristics... 😢 ). It's something not only for GeoParquet, it should be added as a feature in the data warehouses, i.e., given a geom column of a table have a way to get the geometry type from some metadata without exploring the data.

Copy link
Member

@cholmes cholmes left a comment

Choose a reason for hiding this comment

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

I added some edits to make it so the field is required, but unknown is allowed.

I'm also ok if we make it optional and specify that leaving it out is the same as 'unknown', but prefer required & unknown option, to nudge implementors to thinking about it.

format-specs/geoparquet.md Outdated Show resolved Hide resolved
format-specs/geoparquet.md Outdated Show resolved Hide resolved
format-specs/geoparquet.md Outdated Show resolved Hide resolved
@cholmes cholmes added this to the 0.2 milestone Apr 18, 2022
alasarr and others added 4 commits April 18, 2022 22:15
Co-authored-by: Chris Holmes <chomie@gmail.com>
Co-authored-by: Chris Holmes <chomie@gmail.com>
Co-authored-by: Chris Holmes <chomie@gmail.com>
@alasarr
Copy link
Collaborator

alasarr commented Apr 18, 2022

As @jorisvandenbossche already agreed on the idea of required & unknown and @cholmes too, I'm merging.

@alasarr alasarr merged commit ebed795 into opengeospatial:main Apr 18, 2022
@jorisvandenbossche jorisvandenbossche deleted the geometry-type branch April 19, 2022 21:21
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.

Advertizing geometry field "type" in Column metadata ?
4 participants