Adds Ewkt dialect #155

Oreilles · 2023-08-10T00:12:33Z

This PR adds:

enum WktDialect with options WktDialect::Wkt and WktDialect::Ewkt to wkt module
struct EwktString(pub String)
fn to_ewkt(&self, srid: Option<i32>) -> Result<String> to trait ToWkt
fn to_wkt_dialect(&self, dialect: WktDialect, dims: CoordDimensions, srid: Option<i32>) -> Result<String> to trait ToWkt

The trait FromWkb is implemented for EwktString so that the srid is inferred from the Wkb stream if available. The later is made possible by implementing GeomProcessor::srid for WktWriter and calling processor.srid(info.srid) in process_wkb_geom.

Side notes:

This PR only implements Ewkt write. Ewkt read should probably be implemented in the wkt crate.
Should we add ProcessToCsv::to_csv_ewkt and/or ProcessToCsv::to_csv_dialect?

nyurik · 2023-08-10T01:37:40Z

TBH, I am not too happy about changing all of the APIs to force the user to always be WKT dialect aware. I would much rather keep the existing API, plus perhaps add a new constructor, e.g. keep the WktWriter::new(w: impl Write), and add another one like WktWriter::with_dialect(w: impl Write, dialect: WktDialect)?

Oreilles · 2023-08-10T01:50:57Z

Sure, I changed the signature of the constructor for the sake of consistency with WkbWriter but I would definetly understand that an API change would not be acceptable.

nyurik · 2023-08-10T01:56:37Z

oh, i didn't realize we already did it that way with WkbWriter... I'm a bit on a fence on this one then - ideally I would want Wkb to also be done the same way - WkbWriter::with_dialect(...), but I will let @pka to chime in on this one

pka

I'm fine with changing the WktWriter-API. The ToWkt trait API should be kept compatible if possible, which is the case here.

Comments:

We should use the opportunity to drop the lifetime in ẀktWriter, like in RFC: Drop lifetime parameter of GeoJsonReader #147
Run cargo fmt

Oreilles · 2023-08-10T20:15:08Z

Thanks for the review, I ran fmt and took example on the linked PR to remove the WktReader lifetime 👍

nyurik · 2023-08-10T20:28:00Z

@Oreilles thx! Do you need help merging main branch into your PRs?

Oreilles · 2023-08-10T20:53:52Z

I just rebased onto main and solved the few conflicts.

pka · 2023-08-10T21:05:53Z

After discussing the PR with @nyurik, I'm also for keeping the WktWriter::new signature and adding a second method WktWriter::with_dialect. For WkbWriter the dialect is essential, but WKT users usually don't care and I don't want to force them to think about it. What's your use case of using EWKT? Can't remember anyone mentioning it for a long time.

nyurik

I made a few suggestions in code. I think we shouldn't be removing the lifetime in the same PR - lets just figure out the dialects first, as it is not certain?

geozero/src/gdal/gdal_reader.rs

nyurik · 2023-08-10T21:02:11Z

geozero/src/wkt/mod.rs

        /// Convert to WKT String with dimensions.
        fn to_wkt_ndim(&self, dims: CoordDimensions) -> Result<String>;
+        /// Convert to WKT String with srid, dimensions and dialect.
+        fn to_wkt_dialect(


Technically this is more of a to_wkt_with_options or to_wkt_extended method, not just dialect... Naming is hard, we may want to bikeshed this one.... (note that I am still kinda uneasy about the new constructor now requiring an additional dialect param even though most of the users probably won't ever care about it, and plus it doesn't allow us to have a default

Actually maybe we don't want a to_wkt_with_options ? Then to_ewkt would leave you no choice over the output dimensions, e.g. taking all those available. I hardly think of a use case where you'd want an EWKT output with less information. Maybe we can just remove to_wkt_dialect, and just leave to_wkt, to_wkt_ndim and to_ewkt.

I just hope the number of dialects don't explode :) to_ewkt and to_ewkt_ndim sounds fine.

Oh, but since there is now three to_wkt flavour, that would imply a lot of code duplicate to remove to_wkt_dialect. So we might as well just find an appropriate name for it as you suggested. About the number of Wkt dialect... I discovered while working on this that Spatialite rejects POINT(1 2 3 4) as valid Wkt and will only accept POINTZM(1 2 3 4) so maybe a next PR to come ? 😅

sigh :) I think we may as well just use to_wkt_with_opts, and provide all those detalis. We may even want to get rid of the to_wkt_ndims to reduce the api surface (TBD)

I just really don't like the to_wkt_dialect because it actually has many more params than just the dialect one, making it confusing

I agree, not sure what the best solution is here so I'll that one up to you..!

geozero/src/wkt/mod.rs

Oreilles · 2023-08-10T21:15:07Z

After discussing the PR with @nyurik, I'm also for keeping the WktWriter::new signature and adding a second method WktWriter::with_dialect. For WkbWriter the dialect is essential, but WKT users usually don't care and I don't want to force them to think about it.

That's understandable, I'll do the modification 👍

What's your use case of using EWKT ?

Even though it's not a standard, thanks to PostGIS success it's still quite common and (AFAIAA) is the only human-friendly geometry format that allows SRID declaration. It will be used as a user input format in a product I'm working on.

Co-authored-by: Yuri Astrakhan <yuriastrakhan@gmail.com>

This reverts commit c670618.

geozero/src/gdal/gdal_reader.rs

nyurik · 2023-08-10T22:29:20Z

geozero/src/wkt/mod.rs

        /// Convert to WKT String with dimensions.
        fn to_wkt_ndim(&self, dims: CoordDimensions) -> Result<String>;
+        /// Convert to WKT String with srid, dimensions and dialect.
+        fn to_wkt_dialect(


I just hope the number of dialects don't explode :) to_ewkt and to_ewkt_ndim sounds fine.

geozero/src/wkt/mod.rs

nyurik · 2023-08-10T22:33:48Z

geozero/src/wkt/mod.rs

+            let mut out: Vec<u8> = Vec::new();
+            let mut writer = WktWriter::with_dialect(&mut out, WktDialect::Ewkt);
+            writer.dims = CoordDimensions::xyzm();
+            crate::wkb::process_wkb_type_geom(rdr, &mut writer, dialect)?;


not sure if actionable, just checking if this is correct: both the writer and the processor need a dialect? And when reading, the processor cannot auto-detect the dialect, or can it?

So an Ewkt string can be parsed from several Wkb dialects, which is why we need to pass it as a parameter. In the current implementation, no detection of the Wkb dialect is implemented; I guess it would be pretty tricky to do so, as they all have their own quirks.

nyurik

seems like this one is ready to go once we settle on the to_wkt_X name... I think to_wkt_with_opts is the cleanest?

nyurik · 2023-08-11T17:14:18Z

geozero/src/wkt/mod.rs

        /// Convert to WKT String with dimensions.
        fn to_wkt_ndim(&self, dims: CoordDimensions) -> Result<String>;
+        /// Convert to WKT String with srid, dimensions and dialect.
+        fn to_wkt_dialect(


sigh :) I think we may as well just use to_wkt_with_opts, and provide all those detalis. We may even want to get rid of the to_wkt_ndims to reduce the api surface (TBD)

I just really don't like the to_wkt_dialect because it actually has many more params than just the dialect one, making it confusing

nyurik · 2023-08-11T22:35:14Z

I made a few more suggestions for this PR in Oreilles#2

rename to_wkt_dialect -> to_wkt_with_opts
make WktWriter content private! It's ridiculous to have write access to a writer from outside
introduce WktWriter::with_dims to allow creation of a writer with the needed params

* rename `to_wkt_dialect` -> to_wkt_with_opts` * make `WktWriter` content private! It's ridiculous to have write access to a writer from outside * introduce `WktWriter::with_dims` to allow creation of a writer with the needed params

ewkt PR - a few suggestions

Oreilles force-pushed the ewkt branch from 9ecd509 to 51b682e Compare August 10, 2023 00:13

nyurik requested review from nyurik and pka August 10, 2023 06:11

pka requested changes Aug 10, 2023

View reviewed changes

Oreilles added 3 commits August 10, 2023 22:49

Add Wkt and Ewkt dialects

048dc6f

Fix formatting

fbde937

Remove WktReader lifetime

c670618

Oreilles force-pushed the ewkt branch from a9d2ac2 to c670618 Compare August 10, 2023 20:53

nyurik requested a review from pka August 10, 2023 20:55

nyurik reviewed Aug 10, 2023

View reviewed changes

Oreilles and others added 4 commits August 10, 2023 23:17

Simplify WktDialect default declaration

be0534e

Co-authored-by: Yuri Astrakhan <yuriastrakhan@gmail.com>

Revert WktWriter::new signature and addsWktWriter::with_dialect

fd840fc

Revert "Remove WktReader lifetime"

c6c37a2

This reverts commit c670618.

Remove unused imports

06ee5f4

nyurik reviewed Aug 10, 2023

View reviewed changes

Oreilles added 3 commits August 11, 2023 00:54

Revert gdal line_3d test

8b2594f

Revert WktString::from_wkb writer declaration

6a8b966

Add new dialects to Format conversion overview documentation

f4e32cc

nyurik reviewed Aug 11, 2023

View reviewed changes

nyurik added 2 commits August 12, 2023 00:04

fix build

24eca0d

fix CI build

116f438

nyurik and others added 3 commits August 12, 2023 00:38

fix ewkt - take 2

4f2c505

a few suggestions

36dee84

* rename `to_wkt_dialect` -> to_wkt_with_opts` * make `WktWriter` content private! It's ridiculous to have write access to a writer from outside * introduce `WktWriter::with_dims` to allow creation of a writer with the needed params

Merge pull request #2 from nyurik/ewkt-suggestions

66b5c1e

ewkt PR - a few suggestions

pka approved these changes Aug 12, 2023

View reviewed changes

pka merged commit 5e92027 into georust:main Aug 12, 2023
1 check passed

This was referenced Aug 12, 2023

Make srid & dim fields of all writers private, and add ctor #156

Open

Fix invalid WKT empty geometries #160

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Ewkt dialect #155

Adds Ewkt dialect #155

Oreilles commented Aug 10, 2023 •

edited

Loading

nyurik commented Aug 10, 2023

Oreilles commented Aug 10, 2023

nyurik commented Aug 10, 2023 •

edited

Loading

pka left a comment

Oreilles commented Aug 10, 2023

nyurik commented Aug 10, 2023

Oreilles commented Aug 10, 2023

pka commented Aug 10, 2023 •

edited by nyurik

Loading

nyurik left a comment

nyurik Aug 10, 2023

Oreilles Aug 10, 2023

nyurik Aug 10, 2023

Oreilles Aug 10, 2023

nyurik Aug 11, 2023

Oreilles Aug 11, 2023

Oreilles commented Aug 10, 2023

nyurik Aug 10, 2023

nyurik Aug 10, 2023

Oreilles Aug 10, 2023

nyurik left a comment

nyurik Aug 11, 2023

nyurik commented Aug 11, 2023

Adds Ewkt dialect #155

Adds Ewkt dialect #155

Conversation

Oreilles commented Aug 10, 2023 • edited Loading

nyurik commented Aug 10, 2023

Oreilles commented Aug 10, 2023

nyurik commented Aug 10, 2023 • edited Loading

pka left a comment

Choose a reason for hiding this comment

Oreilles commented Aug 10, 2023

nyurik commented Aug 10, 2023

Oreilles commented Aug 10, 2023

pka commented Aug 10, 2023 • edited by nyurik Loading

nyurik left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

nyurik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nyurik commented Aug 11, 2023

Oreilles commented Aug 10, 2023 •

edited

Loading

nyurik commented Aug 10, 2023 •

edited

Loading

pka commented Aug 10, 2023 •

edited by nyurik

Loading