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

Update the spec according to the new feedback #5

Merged
merged 2 commits into from
Oct 4, 2024
Merged

Conversation

jiayuasu
Copy link

@jiayuasu jiayuasu commented Oct 3, 2024

I've updated the PR according to the feedback from several folks.

  1. Rename EdgeInterpolation back to Edges since the explanation in comments already made it clear and we don't have to use the long name any more.
  2. Removed the Covering statistics since it is not really useful at the moment. Neither C++ and Java POC implement it.
  3. Adopted the westmost and eastmost representation of BBox when edges = spherical.
  4. Offloaded the CRS representation to Parquet file metadata fields such that multiple geometry columns can refer to the same CRS. This also makes sure that the Parquet spec does not rely on another spec for CRS.
  5. Removed the optional list<KeyValue> key_value_metadata. This makes sure that the geometry column definition is clear.

With this PR in place, the Parquet Geometry PR is nearly identical to the Iceberg Geometry PR, with the following exceptions:

  1. The Iceberg Geometry PR uses lower_bounds and upper_bounds instead of BBox statistics. But it could easily adopt westmost and eastmost representation by updating the spec explanation.
  2. The Iceberg Geometry PR offloads CRS to table properties while Parquet Geometry PR offloads it to file metadata.

If additional geometry column field like orientation is needed, we can add it to both Iceberg and Parquet Geometry PR but we should not allow list for arbitrary fields.

@wgtmac wgtmac merged commit c134c91 into wgtmac:geo Oct 4, 2024
3 checks passed
wgtmac pushed a commit that referenced this pull request Oct 13, 2024
* Update the spec according to the new feedback

* Fix typo
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.

2 participants