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

Request: Please support CurvePolygons or at least simple circles encoded as CurvePolygons? #18

Open
jamesra opened this issue May 25, 2021 · 15 comments

Comments

@jamesra
Copy link

jamesra commented May 25, 2021

I understand CurvePolygons are not supported at this time. I'm creating this issue as a request to at least support CurvePolygons that encode simple circles. Naturally the research lab I work for has an annotation database with millions of circles encoded as CurvePolygons:

CURVEPOLYGON (CIRCULARSTRING (40288.3829913593 59811.5850000016, 41716.865 61240.0670086423, 43145.3470086407 59811.5850000016, 41716.865 58383.1029913609, 40288.3829913593 59811.5850000016))

Currently I am unable to find a way to read rows with CurvePolys using EF Core+NetTopologySuite. I asked Stack Overflow as well with no answer so far.

Using a converter via Microsoft.EntityFrameworkCore.Storage.ValueConversion.ValueConverter does not work as the GeoParseException thrown for an unsupported type prevents the converter from being invoked. I've been unable to figure out how to intercept the column to convert it to an alternate representation.

My workaround will be to see if I can pull down the geometry columns as byte[], and if that doesn't work I'll have to fork a custom SqlServerBytes and use app-specific logic to workaround (Ewww...).

I'm not sure what a general solution would be but I'd love to see a way forward for future users. Unfortunately NetTopologySuite does not appear to support the circle as a geometry primitive. If it did I would argue for a special case that detects and supports CurvePolygons representing Circles as I expect they are likely the most common case.

An alternate approach would be a mechanism to invoke a handler when an unsupported type is decoded. Then a similar mechanism to encode geometry manually. Neither option feels great to me.

@bricelam
Copy link
Collaborator

bricelam commented May 26, 2021

You can return an approximation of the polygon to the client using STCurveToLine:

var entities = db.MyEntities.FromSqlRaw(@"
    SELECT Id,
           Name,
           GeoColumn.STCurveToLine() AS GeoColumn
    FROM MyTable
");

It should also be possible to map it to a CLR method so you can apply it later in the query (instead of in FromSqlRaw).

var entities = from e in db.MyEntities
               // where ...
               select new MyEntity
               {
                   Id = e.Id,
                   Name = e.Name,
                   GeoColumn = db.CurveToLine(e.GeoColumn)
               };

But obviously, this only helps with reading and not writing.

@bricelam
Copy link
Collaborator

See also NetTopologySuite/NetTopologySuite#247

@jamesra
Copy link
Author

jamesra commented May 26, 2021

Interesting... so I need to structure these queries so EF Core can inform the server to transform the column so the client never sees CurvedPolygons. At first glance it looks simpler than my workaround. After studying the EF Core and SQLServerBytes code I was heading down the road of writing a custom SQL type converter for the Geometry library I use. Thanks.

@bricelam
Copy link
Collaborator

I was thinking about this more, and I like the idea of SqlServerBytes giving you a subtype of Geometry for these types. It might even be able to let you access the shape data with methods like NumCurves and CurveN. It would obviously throw if you try to perform any spatial operations on it (until NTS fully supports curves). But if you hand it back to SqlServerBytes, it would be able to roundtrip the data.

This might even unblock a path for creating curve geometries:

command.CommandText = "SELECT geometry::Parse('CIRCULARSTRING(1 1, 2 2, 3 1, 2 0, 1 1)')";
var dataReader = command.ExecuteReader();

var circularString = geometryReader.Read(dataReader.GetSqlBytes(0).Value);

@jamesra
Copy link
Author

jamesra commented May 26, 2021

I really like that pass-through solution. If I'm understanding correctly I'd get a Geometry subtype that can expose the OGC text/binary representation. That is perfect for the server-side gRPC service code I'm working on at the moment. I just want to pull the geometry column as text/binary and copy it directly into a gRPC response for the client to deal with. Then when a client calls a write operation I add a special case adapter to translate circles into OGC SQL and have SqlServerBytes pass it through. You'd give a path forward for a lot of use cases until full support is added.

I personally would not request NumCurves or CurveN in a first pass implementation unless it is trivial to pass those through to SQL. I would like the ability to choose between text and binary encodings exposed by this Geometry subtype to control bandwidth demands as I'm streaming thousands of these to clients.

How hard is it to add a new geometry type and where would it go? Is this something I can help with or is it more involved?

@bricelam
Copy link
Collaborator

Hmm, even getting the WKT/B might be too tricky since that would require other parts of NTS to know about the new types. (Which is already half way to having full curve support.)

My idea was much more limited—the special Geometry type would be opaque to everything besides SqlServerBytes. Basically: here’s an unknown Geometry object from SQL Server, if you give it back to us we can round trip it into SQL Server again. It wouldn’t be very useful in serialization scenarios.

But if you could get the individual curves from it, you could theoretically add your own code to generate WKT/B.

Thoughts, @airbreather?

@bricelam
Copy link
Collaborator

If all you want is the WKT, you should be able to get that from the server today with EF:

var entities = db.Entities.Select(e => new { e.Id, WKT = e.Geometry.AsText() });

@airbreather
Copy link
Member

airbreather commented May 27, 2021

I think it makes sense. I would personally make the concrete subclass internal to SqlServerBytes and expose a marker interface so that the caller would check for the situation like:

var entity = db.Entities.First();
if (entity.Geometry is IGeometryPlaceholder)
{
    // figure out how to work around not having access to it
}
else
{
    // do interesting things with entity.Geometry
}

That way, if NTS adds support for this later, most callers using this pattern shouldn't need to change anything to start working with it.

@jamesra
Copy link
Author

jamesra commented May 27, 2021

With that IGeometryPlaceholder interface isolated to SQLServerBytes it seems it would a natural place to expose get/set accessor methods to expose WKT/WKB to pass through to SQL on writes. In my case I already have some adapters that translate my geometry into WKT so passing through reduces the effort to port to EF Core. I'm not sure how often other users would benefit from the same situation.

Another advantage of IGeometryPlaceholder is it allows one to use an EF value-converter to deal with these special cases in one location.

Thanks bricelam for the suggestion about updating the queries to return text. That will be my short-term fallback plan if it also can work for writes. I'm obviously not an EF Core / NTS expert. I'm looking for the most painless place to install type adapters. Ideally I'd like to use NTS types for common geometry tasks within my service.

@bricelam
Copy link
Collaborator

@jamesra The biggest blocker for you is that the only code here that knows how to generate WKT/B for CircularString, CompoundCurve, CurvePolygon, and FullGlobe is SQL Server. We basically get a stream of bytes (not WKB) from SQL Server that we can't interpret in the context of NTS. Really all we can really do without adding full support for these types is just hand them back to SQL Server.

bricelam added a commit to bricelam/NetTopologySuite.IO.SqlServerBytes that referenced this issue May 27, 2021
@jamesra
Copy link
Author

jamesra commented May 28, 2021

@bricelam Oh... I get it now. Thanks for taking the time to explain again. I shouldn't have assumed SQL would use WKB to send data. I'll look at the source again to better understand where the code that translates SQL's encoding to NTS types.

Hopefully I can configure EF Core to automatically generate queries that convert geometry columns into WKT and represent it as a string in the models. Then I can use my geometry library until NTS gets curves.

@jamesra
Copy link
Author

jamesra commented May 28, 2021

I see the pull request for unsupported types already there which is great. I'll try using that code with a value converter to move forward. Thanks for the quick fix!

@jamesra
Copy link
Author

jamesra commented May 28, 2021

@bricelam I tried your commit. It solves my case if UnsupportedGeometry is a public class instead of internal. (Not sure if that is how it was intended I detect the row didn't convert) That let me detect the unsupported case in my application logic. It worked with a value converter:

protected static object DoConvertFromProvider(object input)
{
    if (input is UnsupportedGeometry)
    {
        //TODO: App specific logic
        return "Circle";
    }
    else if(input is Geometry shape)
    {
        return shape.ToText();
    }

    throw new ArgumentException("Unexpected type passed to converter");
}

The inverse transform is what you said was hard since SQL doesn't expect WKT. In practice I may keep geometry as a backing field and have a SQL trigger clean up inserts for circles.

@airbreather
Copy link
Member

It solves my case if UnsupportedGeometry is a public class instead of internal.

My suggestion involved a (public) marker interface that you could use to detect the situation, while still leaving the Geometry subclass, UnsupportedGeometry, internal.

The marker interface could eventually gain some functionality as well, but I can't think of a more appropriate way to let you detect the situation in the first place, while also being able to automatically benefit from a future change on the NTS side that might bring in more proper support for this.

@FObermaier
Copy link
Member

Before we put too much effort in UnsupportedGeometry or IGeometryPlaceholder I invite you to collaborate on NetTopologySuite.Curved.

I don't expect it to work out of the box but I'm pretty confident that it is close. Have a look.

Areas that are not tested:

  • Everything, help with unit tests.

Areas that are not implemented at all:

  • WKTReaderEx
  • WKBWriterEx
  • WKBReaderEx
  • ...

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

No branches or pull requests

4 participants