StringCompressedPolyLine now supports WKB for incompressible polylines #200
Conversation
lucaspcram
changed the title
lucaspcram
changed the title
There was a problem hiding this comment.
Really nice fix, thanks @lucaspcram! A few questions attached.
| */ | ||
| public static PolyLine wkb(final byte[] wkb) | ||
| { | ||
| return new WkbPolyLineConverter().backwardConvert(wkb); |
There was a problem hiding this comment.
It looks like this would not handle the single location polylines outlined below?
There was a problem hiding this comment.
Does it not already? This method is just a carbon copy of the already existing wkt method. The WkbPolyLineConverter.backwardConvert method used by wkb will just end up creating a singleton list to initialize the polyline.
If you take a look at the toWkb method I added (line 1126), you'll notice it does handle the case of a polyline with a single point (following the example set by toWkt). In fact I was actually wondering why this is necessary. Both of those functions call WkXPolyLineConverter.convert, and it looks like that method also handles the special case of a single point PolyLine. So is this check redundant, or am I missing something?
| /* | ||
| * If the first byte of the encoding array is this sentinel value, then the following encoding | ||
| * is WKB and not string-compressed. We use '0' as the sentinel value since the string | ||
| * compression algorithm will always use printable ASCII characters. There will never be a 0 |
| { | ||
| logger.warn("Unable to use string compression for {}", points.toWkt()); |
There was a problem hiding this comment.
Isn't that logging statement redundant if the below exception is thrown?
There was a problem hiding this comment.
Normally it would be but this specific exception is always caught on line 77 of StringCompressedPolyLine (which triggers the fallback), so I thought it would be worth logging. I believe catching the exception explicitly hides it from log output (since it is a checked exception and we are not dumping the stack trace in the catch block).
Please correct me if I am wrong, and I can remove the log statement.
| { | ||
| final byte[] wkbEncoding = polyLine.toWkb(); | ||
| final byte[] finalEncoding = new byte[1 + wkbEncoding.length]; | ||
| finalEncoding[0] = WKB_SENTINEL; |
There was a problem hiding this comment.
Such a waste of bytes!! ;p
Description:
In the case that a user attempted to create a compressed polyline in which two consecutive points had a longitudinal difference of > 180, the algorithm would throw up an exception and fail loudly.
In the alternate case that a user attempted to create a compressed polyline in which two consecutive points had a longitudinal difference of < -180, the algorithm would fail to catch this error condition. Instead, it would happily create a bogus polyline - which would have an undefined value when decompressed.
This PR applies the following fix strategy: an adaptable format, using string compression for closely packed polylines and regular WKB for incompressible polylines.
Potential Impact:
This PR effectively changes the atlas file format. The changes are self contained within
StringCompressedPolyLine, so as far as the Java codebase is concerned, there is no discernible difference. But, if there is any code out there using the protobuf atlas (thinkpyatlas) that does not expect WKB polylines, it is possible that decompression errors could occur. This is due to the fact that atlas generation with > 180 degree polylines no longer fails, but produces an atlas file with a WKB polyline.Unit Test Approach:
I have added
testCompressionWkbFallbackwhich is designed to encode polylines using the WKB fallback. It then tests that theirtoStringrepresentation correctly matches their WKT, which only occurs if string compression failed. In the case that string compression succeeded,toStringinstead would have return the encoded string.Test Results:
All unit tests continue to pass. I also ran the full atlas integration test suite, which continues to pass. I think it is worth discussing potential downstream impact before merging.
In doubt: Contributing Guidelines