Add support for Spatialite and MySQL WKB dialects #153

Oreilles · 2023-08-09T19:43:00Z

This PR adds support for Spatialite and MySQL WKB dialects.

I'd happily discuss the implementation. Quirks of the spatialite dialect that required some refactoring:

A geometry blob ends with 0xFE. In order to know wether the polygon_end (or alike) is the last one, we keep track of the collection entity nesting level. If the nesting level is 0 when a geometry ends, it means it is the last one of the blob and the terminating byte 0xFE must be appended. Even though MULTI* geometries cannot be part of a GEOMETRYCOLLECTION in Spatialite at the moment, this was my easiest go at the problem - and it also makes it more future-proof in case that feature is added at some point.
In contrary to EWKB, the byte order and SRID info are not repeated in the nested entities headers, which means they have to be inherited from the parent entity info. In order to do that, I had to change the signature of the read_header parameter of process_wkb_geom_n so that the parent geometry info can be passed down to its children.
The "compressed" polygon and linestring geometries stores their X,Y,Z coordinates as f32 values, relative to the previous one (with the exception of the first and the last one), which required some refactoring to process_coord to avoid code duplication between the two implementations.

Side notes:

Shouldn't WkbWriter implement GeomProcessor::srid() ?
Only read support has been implemented for Compressed and TinyPoint geometries, write isn't implemented.
Spatialite header stores the envelope as [minx, miny, maxx, maxy] (like GeoJSON) whereas gpkg stores it as [minx, maxx, miny, maxy]

I also refactored the roundtrip tests so they can be shared by the 5 dialects.

pka

Great addition, thanks a lot @Oreilles !

Please do a cargo fmt and we're good to merge.

Oreilles · 2023-08-10T20:17:11Z

Thanks for the review, the latest commit fixes the formatting issues!

pka · 2023-08-10T20:56:47Z

Thanks! Now we just have to make clippy happy as well. To test locally, you just have to run cargo clippy or to be sure with all options as in the CI: cargo clippy --workspace --all-features --bins --examples --tests --benches.

Oreilles · 2023-08-10T22:00:02Z

I think it's good now!

geozero/src/wkb/wkb_common.rs

nyurik · 2023-08-10T22:05:59Z

geozero/src/wkb/wkb_reader.rs

@@ -293,17 +424,59 @@ pub(crate) fn process_wkb_geom_n<R: Read, P: GeomProcessor>(
    }
 }

+fn emit_coord<P: GeomProcessor>(


I don't think this is a good approach here - you first create coordinate tuple on line 462, and then you discover you don't even need it if multi_dim is false. I would inline this function

Fixed with latest commit

geozero/src/wkb/wkb_reader.rs

nyurik · 2023-08-10T22:11:19Z

geozero/src/wkb/wkb_common.rs

@@ -198,3 +200,12 @@ pub(crate) enum WKBByteOrder {
    Xdr = 0, // Big Endian
    Ndr = 1, // Little Endian
 }
+
+impl From<scroll::Endian> for WKBByteOrder {


good addition, thanks! I wonder if we need a reverse as well?

nyurik · 2023-08-10T22:16:47Z

geozero/src/wkb/wkb_reader.rs

-    } else {
-        scroll::LE
-    };
+    let endian = Endian::from(byte_order != 0);


sigh, Endian::from(bool) is not the most readable interface. I had to look up what true means :(

P.S. this one is not exactly actionable - just a note in case you can think of a cleaner API usage here

Yup, I agree that Endian::from(bool) is not the very explicit at first glance... If we implement the reverse of From<scroll::Endian> for WKBByteOrder as you suggested, then maybe we could also implement From<u8> for WKBByteOrder and chain the two. That's a lot of code for basically a bool check though so...

yeah, probably not worth the complexity - maybe just add an extra line to make it more readable, like this?

let is_little_endian = byte_order != 0; let endian = Endian::from(is_little_endian);

P.S. you can actually inline the second line down below

Much better, I'll do that 👍

geozero/src/wkb/wkb_reader.rs

nyurik · 2023-08-10T22:24:07Z

lol, seems like @pka and I were reviewing it at the same time...

nyurik

see inline

nyurik · 2023-08-10T23:50:17Z

geozero/src/wkb/wkb_reader.rs

-    } else {
-        scroll::LE
-    };
+    let is_little_endian = byte_order != 0;


turns out there is a much cleaner solution, but it requires a bit of research. https://docs.rs/scroll/latest/scroll/ has a lot of docs for that, but essentially instead of using raw.ioread::<u8>() you can use raw.ioread::<WKBByteOrder>. In order for that to work, you would have to implement impl IntoCtx<scroll::Endian> for WKBByteOrder { or something similar (I never used this crate, just saw some docs about cread vs gread vs pread - not sure which is which). At the end of the day, it should offer a very easy way to just do all the on-the-fly conversions cleanly

P.S. Note that you cannot do raw.ioread::<Encoding> because you can only implement a trait for the type in the same crate, and Encoding enum is not part of the geozero

P.P.S. note that we don't need to fix ALL such issues at the same time, can be done in another PR. For more docs on this - https://github.com/m4b/scroll#stdio-api

nyurik

a few minor nits, but looks almost ready to merge. Thanks for all the patience!! :)

geozero/src/wkb/wkb_reader.rs

geozero/src/wkb/wkb_writer.rs

Oreilles · 2023-08-11T19:35:52Z

Thanks for all the patience!! :)

Thanks for your time, it's very pleasing contributing to open source projects with responsive maintainers 👍

nyurik · 2023-08-11T19:51:19Z

please fix the conflict and lets merge

Oreilles · 2023-08-11T20:05:46Z

Solved

nyurik · 2023-08-11T21:45:41Z

At first i made a few minor comments here, but then just created a PR for your PR - please merge if agree -- Oreilles#1

a few minor suggestions

Oreilles · 2023-08-12T11:24:40Z

I merge the two PR!
On an unrelated side, I have a question about the processor functions xy and coordinate: since all parameter of coordinate but x and y are of type Option, would it be possible to only ever use coordinate and let the writer decide what to do with them ?
My concern is that if you parsed for example a Wkb blob with XY coordinates, and process it with a WkbWriter for which you set dims = CoordDimensions::xyzm(), then the generated WKB would be invalid: its header would indicate the presence of XYZM values, but since only processor.xy would be called by the reader (since the input doesn't have ZM), the ZM coordinates bytes wouldn't be written as they should.
Same goes if your processed a Wkb blob that has XYZM, with processor with dims = CoordDimensions::xy(). The header would indicate only XY coordinates, but XYZM would actually end up included.
Having the readers always call processor.coordinate and let the processors decide what to do with the unnecessary (or absent) coordinate values would mitigate this issue.
I picked a Wkb to Wkb example for the sake of simplicity but that behavior seems to be shared by other readers and writers as well.

pka · 2023-08-12T14:27:03Z

On an unrelated side, I have a question about the processor functions xy and coordinate: since all parameter of coordinate but x and y are of type Option, would it be possible to only ever use coordinate and let the writer decide what to do with them ?

The reason for having two methods was performance. This method is the innermost loop of any geometry operation (except for points) and passing additional optional params costs performance for the most common xy case.

My concern is that if you parsed for example a Wkb blob with XY coordinates, and process it with a WkbWriter for which you set dims = CoordDimensions::xyzm(), then the generated WKB would be invalid

Your concerns are valid, if the reader implementing the GezeroGeometry trait doesn't look at the requested dimensions.
Code example for dimension handling:

geozero/geozero/src/geojson/geojson_reader.rs

Lines 238 to 251 in ece8dfe

    
               if multi_dim { 
        
                   processor.coordinate( 
        
                       point_type[0], 
        
                       point_type[1], 
        
                       point_type.get(2).copied(), 
        
                       None, 
        
                       None, 
        
                       None, 
        
                       idx, 
        
                   ) 
        
               } else { 
        
                   processor.xy(point_type[0], point_type[1], idx) 
        
               } 
        
           }

WkbReader looks also fine, but there are maybe readers which don't respect GeomProcessor::dimensions resp. GeomProcessor::multi_dim, which would be a bug.

In your WKB xy to WKB xyzm example, I don't think that the result is invalid, you just loose the additional dimensions.

Oreilles · 2023-08-12T14:39:23Z

In your WKB xy to WKB xyzm example, I don't think that the result is invalid, you just loose the additional dimensions.

If we process a WKB that doesn't have ZM coordinates, it will only call processor.xy:

geozero/geozero/src/wkb/wkb_reader.rs

Lines 315 to 319 in ece8dfe

    
           if multi_dim { 
        
               processor.coordinate(x, y, z, m, None, None, idx) 
        
           } else { 
        
               processor.xy(x, y, idx) 
        
           }

And then only X and Y will be written:

geozero/geozero/src/wkb/wkb_writer.rs

Lines 163 to 169 in ece8dfe

    
           fn xy(&mut self, x: f64, y: f64, _idx: usize) -> Result<()> { 
        
               if self.geom_state == GeomState::MultiPointGeom { 
        
                   self.write_header(WKBGeometryType::Point)?; 
        
               } 
        
               self.out.iowrite_with(x, self.endian)?; 
        
               self.out.iowrite_with(y, self.endian)?; 
        
               Ok(())

When a parser will try to read the WKB emitted, il will try to read four f64 values for each point (the header says so), but only two would have been written.

The reason for having two methods was performance. This method is the innermost loop of any geometry operation (except for points) and passing additional optional params costs performance for the most common xy case.

You might have tested it already (I haven't) but in this case, maybe the branching caused by having to handle four different outcomes outweight the performance gain of only calling the method with the least amout of parameters:

The reader emitted XY, and the writer have to write XY
The reader emitted XY, but the writer have to write XYZM
The reader emitted XYZM, but the writer have to write XY
The reader emitted XYZM, and the writer have to write XYZM

In any case, I'm more concerned about the bug opportunities here than performance 😅

nyurik · 2023-08-12T16:09:35Z

@Oreilles i think this one is ready to go once conflict is fixed. One thing I forgot to ask about the other PR (which affect this PR as well) -- why do you need to call processor.srid(info.srid)?; everywhere? Shouldn't the processor be created with a given srid once?

Oreilles · 2023-08-12T16:31:07Z

@nyurik calling processor.srid(info.srid) allowed for inferring the EWKT geometry srid from the WKB blob without having to manually specify it. The trait ToWkt has a to_ewkt function that have an optional srid argument for when the source Wkb doesn't have one, but you still want one in the output. If the input Wkb has an srid defined, then this parameter can be omited and the one provided by the reader will be used (which is why I had to call processor.srid(info.srid). I wrote this so that the srid provided in the to_ewkt function parameter takes predominance over the one provided by the reader:

geozero/geozero/src/wkt/wkt_writer.rs

Lines 82 to 85 in 5e92027

    
           fn srid(&mut self, srid: Option<i32>) -> Result<()> { 
        
               self.srid = self.srid.or(srid); 
        
               Ok(()) 
        
           }

Done

nyurik · 2023-08-12T17:37:39Z

Awesome work, thanks!!

Oreilles added 2 commits August 9, 2023 21:40

Add support for Spatialite and MySQL WKB dialects

a82ca78

Use CoordDimensions constructors in wkb_writer tests

6824aaa

nyurik requested review from pka and nyurik August 9, 2023 23:49

pka requested changes Aug 10, 2023

View reviewed changes

Fix formatting

5b45ed0

Change const cast to template declaration

5ad1254

pka approved these changes Aug 10, 2023

View reviewed changes

nyurik reviewed Aug 10, 2023

View reviewed changes

nyurik previously requested changes Aug 10, 2023

View reviewed changes

Oreilles added 4 commits August 11, 2023 01:13

Make endian check more explicit

94140dc

Rename Spatialite to SpatiaLite

56b09e4

Inline emit_coord

0611531

Fix formatting

2555b83

nyurik reviewed Aug 10, 2023

View reviewed changes

Oreilles added 2 commits August 11, 2023 14:01

Add new dialects to Format conversion overview documentation

0899425

Revert unit test refactor

80cd7eb

nyurik reviewed Aug 11, 2023

View reviewed changes

Tidying

274a333

Oreilles force-pushed the spatialite-mysql branch from b0071c0 to 274a333 Compare August 11, 2023 19:33

nyurik self-requested a review August 11, 2023 19:50

Merge branch 'main' into spatialite-mysql

81b8245

Formatting

b0c9c98

nyurik and others added 4 commits August 12, 2023 00:08

fix CI docs issue

7a630e3

suggestions

a944752

a few more cleanups

451e0e7

Merge pull request #1 from nyurik/suggestions

64acf15

a few minor suggestions

Oreilles added 2 commits August 12, 2023 18:40

Merge remote-tracking branch 'georust/main' into spatialite-mysql

0b7abc8

Fix WktWriter constructor in tests

8a813b7

nyurik merged commit a92ef80 into georust:main Aug 12, 2023
1 check passed

pka mentioned this pull request Aug 12, 2023

Improve WkbWriter constructors #158

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Spatialite and MySQL WKB dialects #153

Add support for Spatialite and MySQL WKB dialects #153

Oreilles commented Aug 9, 2023 •

edited

Loading

pka left a comment

Oreilles commented Aug 10, 2023

pka commented Aug 10, 2023

Oreilles commented Aug 10, 2023

nyurik Aug 10, 2023

Oreilles Aug 10, 2023

nyurik Aug 10, 2023

nyurik Aug 10, 2023

nyurik Aug 10, 2023

Oreilles Aug 10, 2023

nyurik Aug 10, 2023 •

edited

Loading

Oreilles Aug 10, 2023

nyurik commented Aug 10, 2023

nyurik left a comment

nyurik Aug 10, 2023 •

edited

Loading

nyurik Aug 11, 2023

nyurik left a comment

Oreilles commented Aug 11, 2023

nyurik commented Aug 11, 2023

Oreilles commented Aug 11, 2023

nyurik commented Aug 11, 2023

Oreilles commented Aug 12, 2023 •

edited

Loading

pka commented Aug 12, 2023

Oreilles commented Aug 12, 2023 •

edited

Loading

nyurik commented Aug 12, 2023

Oreilles commented Aug 12, 2023 •

edited

Loading

nyurik commented Aug 12, 2023

Add support for Spatialite and MySQL WKB dialects #153

Add support for Spatialite and MySQL WKB dialects #153

Conversation

Oreilles commented Aug 9, 2023 • edited Loading

pka left a comment

Choose a reason for hiding this comment

Oreilles commented Aug 10, 2023

pka commented Aug 10, 2023

Oreilles commented Aug 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyurik Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyurik commented Aug 10, 2023

nyurik left a comment

Choose a reason for hiding this comment

nyurik Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyurik left a comment

Choose a reason for hiding this comment

Oreilles commented Aug 11, 2023

nyurik commented Aug 11, 2023

Oreilles commented Aug 11, 2023

nyurik commented Aug 11, 2023

Oreilles commented Aug 12, 2023 • edited Loading

pka commented Aug 12, 2023

Oreilles commented Aug 12, 2023 • edited Loading

nyurik commented Aug 12, 2023

Oreilles commented Aug 12, 2023 • edited Loading

nyurik commented Aug 12, 2023

Oreilles commented Aug 9, 2023 •

edited

Loading

nyurik Aug 10, 2023 •

edited

Loading

nyurik Aug 10, 2023 •

edited

Loading

Oreilles commented Aug 12, 2023 •

edited

Loading

Oreilles commented Aug 12, 2023 •

edited

Loading

Oreilles commented Aug 12, 2023 •

edited

Loading