-
Notifications
You must be signed in to change notification settings - Fork 2.6k
spec: add clarification about the Geometry type calculation #13227
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
base: main
Are you sure you want to change the base?
Conversation
@rdblue @szehon-ho please review :-) |
format/spec.md
Outdated
@@ -221,7 +221,7 @@ Supported primitive types are defined in the table below. Primitive types added | |||
| | **`uuid`** | Universally unique identifiers | Should use 16-byte fixed | | |||
| | **`fixed(L)`** | Fixed-length byte array of length L | | | |||
| | **`binary`** | Arbitrary-length byte array | | | |||
| [v3](#version-3) | **`geometry(C)`** | Geospatial features from [OGC – Simple feature access][1001]. Edge-interpolation is always linear/planar. See [Appendix G](#appendix-g-geospatial-notes). Parameterized by CRS C. If not specified, C is `OGC:CRS84`. | | | |||
| [v3](#version-3) | **`geometry(C)`** | Geospatial features from [OGC – Simple feature access][1001]. Edge-interpolation is always linear/planar, and geometric calculations are performed using Cartesian mathematics along straight lines. See [Appendix G](#appendix-g-geospatial-notes). Parameterized by CRS C. If not specified, C is `OGC:CRS84`. | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion:
Edge-interpolation is always linear/planar, ie geometric calculations are performed using Cartesian mathematics along straight lines.
Then its more clear its a clarification rather than another fact
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed.
@@ -221,7 +221,7 @@ Supported primitive types are defined in the table below. Primitive types added | |||
| | **`uuid`** | Universally unique identifiers | Should use 16-byte fixed | | |||
| | **`fixed(L)`** | Fixed-length byte array of length L | | | |||
| | **`binary`** | Arbitrary-length byte array | | | |||
| [v3](#version-3) | **`geometry(C)`** | Geospatial features from [OGC – Simple feature access][1001]. Edge-interpolation is always linear/planar. See [Appendix G](#appendix-g-geospatial-notes). Parameterized by CRS C. If not specified, C is `OGC:CRS84`. | | | |||
| [v3](#version-3) | **`geometry(C)`** | Geospatial features from [OGC – Simple feature access][1001]. Edge-interpolation is always linear/planar, i.e., geometric calculations are performed using Cartesian mathematics along straight lines. See [Appendix G](#appendix-g-geospatial-notes). Parameterized by CRS C. If not specified, C is `OGC:CRS84`. | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mention that the purpose of Cartesian mathematics is to avoid the need to interpret the CRS?
Something like:
The CRS may be used to interpret points as geographic locations, but does not affect calculations, which are always Cartesian.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Fixed. Please review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, but I think @mkaravel suggests some good changes to make to clarify the bounding boxes for geometry.
@jiayuasu Thank you for driving this! I am not able to comment directly at appropriate place since there are no changes there, which is why I writing this as a high level comment in the PR. In line 684 of the existing spec (prior to this PR) we state:
I believe that in order to make the spec self-consistent it would make sense to rephrase this and specify that How about the following slight modification?
|
@mkaravel I think this is relevant to our discussion in Slack. After giving some thoughts, I think it makes sense to consider that together with the |
@jiayuasu, can you help me understand why we wouldn't make the clarifications @mkaravel suggests in this PR? If we don't expect the CRS to an effect on calculations for |
@rdblue I think it makes sense to drop On the other hand, I'd like to also add I am fine adding the sentence to disallow wraparound here and create another PR to add Please let me know what you think |
@jiayuasu I am obviously (since I have suggested it in the past) totally on board with this plan. This will make the semantics of the types very clean (geometry always Cartesian, calculations independent of the CRS) and allow geography to have an easy, practical, and meaningful way to deal with the antimeridian geometries (also very clean IMHO).
My thinking when I proposed the changes above in this PR was to make it self-consistent and self-contained regarding the @rdblue Are you okay with this? Thoughts? |
Background
Several engines including Spark/Sedona and Presto explicitly mention the calculations on geometries are done via Cartesian mathematics. It would be good if we add clarification to the Iceberg spec
Proposed change
Added the following sentence