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

Switch to compact-SRID WKB format #664

Merged
merged 2 commits into from
Jan 24, 2021

Conversation

hisener
Copy link
Contributor

@hisener hisener commented Jan 15, 2021

Fixes #663.

I have ported GEOS WKBWriter and changed WKBReader to use the parent's SRID in sub geometries.

Added tests to verify that

  • WKBWriter writes compact WKB format
  • WKBReader can read from both format types
  • Parsed sub geometry types have the SRID of the parent in case the given WKB is compact.

@@ -202,7 +202,6 @@ else if(isStrict)

// determine if SRIDs are present
hasSRID = (typeInt & 0x20000000) != 0;
int SRID = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that we use the class field. This might be problematic in case a geometry collection contains mixed SRIDs as @dr-jts pointed here.

Copy link
Contributor

@dr-jts dr-jts Jan 15, 2021

Choose a reason for hiding this comment

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

Yes, definitely problematic. This is a bug in the GEOS implementation IMO.

This should just use the SRID of the top-most geometry (or 0 if not set). JTS does not support mixed SRIDs in collections.

Copy link
Contributor Author

@hisener hisener Jan 15, 2021

Choose a reason for hiding this comment

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

Shouldn't we throw a ParseException in that case instead of using the SRID of top-most geometry?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's nice to allow reading as many WKB inputs as possible. Even if they are invalid, they can only be fixed if they are able to be read.

To preserve the most information, the SRIDs should always be read, and then applied to any missing SRIDs in child geometries. This will require passing the most recent SRID down the call stack as a parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a commit, PTAL.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good.

Signed-off-by: Halil İbrahim Şener <hisener@yahoo.com>
Signed-off-by: Halil İbrahim Şener <hisener@yahoo.com>
Copy link
Contributor

@dr-jts dr-jts left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@hisener
Copy link
Contributor Author

hisener commented Jan 23, 2021

Can we include this change in the next release, and is there a planned date for it?

@dr-jts
Copy link
Contributor

dr-jts commented Jan 24, 2021

Yes, I will commit this.

There should be a new release by the middle of 2021, I think. Is that soon enough for you?

@dr-jts dr-jts merged commit b8bcad8 into locationtech:master Jan 24, 2021
@dr-jts dr-jts changed the title Switch to compact WKB format Switch to compact-SRID WKB format Jan 24, 2021
@hisener hisener deleted the hsener/gh-663 branch January 25, 2021 06:57
@hisener
Copy link
Contributor Author

hisener commented Jan 25, 2021

Is it going to be part of the next major release? I was hoping for a minor in the next couple of weeks. 🙈

@dr-jts
Copy link
Contributor

dr-jts commented Jan 25, 2021

We might be able to get out a minor release this quarter. A minor release takes just as much work as a major release.

@hisener
Copy link
Contributor Author

hisener commented Jan 25, 2021

Ack! This quarter would be nice. Thanks! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question: Is there a way to use PostGIS WKB format in WKBWriter?
2 participants