SqlServer: Support spatial data via NTS #13115

bricelam · 2018-08-24T23:11:13Z

There's still a lot to do (e.g. support mapping to GEOGRAPHY), bit it's at a point where we can start gathering feedback while we continue iterating on it.

To get started, install the Microsoft.EntityFrameworkCore.SqlServer.NetTopologySuite package, call .UseSqlServer(..., x => x.UseNetTopologySuite()), and add IGeometry properties to your model.

Part of #1100

bricelam · 2018-08-27T22:28:05Z

Doh, compile error on net461. Looks like I need to change my awesome use of Span<T>.

bricelam · 2018-08-27T22:40:17Z

src/EFCore.SqlServer.NTS/Extensions/SqlServerNTSServiceCollectionExtensions.cs

+            Check.NotNull(serviceCollection, nameof(serviceCollection));
+
+            // TODO: Use EF infrastructure?
+            serviceCollection.TryAddSingleton(NtsGeometryServices.Instance);


@ajcvickers Worth creating a new services builder?

Probably not worth it for just this service.

bricelam · 2018-08-27T22:41:20Z

src/EFCore.SqlServer.NTS/NetTopologySuite.IO/SqlServerSpatialReader.cs

+                // TODO: Remove when fixed in NTS
+                if (_sequenceFactory is PackedCoordinateSequenceFactory packedFactory)
+                {
+                    ordinates = OrdinatesUtility.DimensionToOrdinates(packedFactory.Dimension);


Submitted NetTopologySuite/NetTopologySuite#255

bricelam · 2018-08-27T22:44:07Z

src/EFCore.SqlServer.NTS/Storage/Internal/SqlServerGeometryTypeMapping.cs

+        protected override string GenerateNonNullSqlLiteral(object value)
+        {
+            // TODO: Can we avoid the conversion in the first place? Should we just inline the BLOB?
+            var geometry = _reader.Read(((SqlBytes)value).Value);


@ajcvickers Have we considered adding a hook before the value gets converted?

airbreather

I just had a few notes / suggestions as I was reading through the NTS parts of this. I've never used EF itself, so I obviously can't comment much on those bits, heh.

airbreather · 2018-08-28T11:59:34Z

src/EFCore.SqlServer.NTS/NetTopologySuite.IO/SqlServerSpatialReader.cs

+                }
+                catch (EndOfStreamException)
+                {
+                    throw new FormatException();


Might it make more sense to provide a more specific message, and/or move this into Geography.ReadFrom?

Definitely belongs in Geography.ReadFrom. The default message isn't very useful, is it:

One of the identified items was in an invalid format.

airbreather · 2018-08-28T12:05:09Z

src/EFCore.SqlServer.NTS/NetTopologySuite.IO/SqlServerSpatialWriter.cs

+            var geometries = new Queue<(IGeometry, int)>();
+            geometries.Enqueue((geometry, -1));
+
+            // TODO: For geography (ellipsoidal) data, set IsLargerThanAHemisphere. Can this be represented in NTS?


Can this be represented in NTS?

Not directly / separately, but I think the logic would be:

bool IsLargerThanAHemisphere(IGeometry someGeometry) { return someGeometry.EnvelopeInternal.Width > 180 || someGeometry.EnvelopeInternal.Height > 90; }

then again, this would often do the completely wrong thing if the geometry wraps around the antimeridian (e.g., Alaska including Aleutians), so it would need to be a bit more sophisticated than this sample code.

Thanks! I also wanted to dig into why SQL Server even needs this. I think it's related to how the external rings of geography polygons are handled...

airbreather · 2018-08-28T12:16:43Z

src/EFCore.SqlServer.NTS/NetTopologySuite.IO/SqlServerSpatialWriter.cs

+                    var pointOffset = geography.Points.Count;
+                    var pointsAdded = false;
+
+                    foreach (var coordinate in g.Coordinates)


PERF: I'd typically do this using a GeoAPI.Geometries.ICoordinateSequenceFilter implementation (Done = false, GeometryChanged = false), to avoid lots of extra allocations and copying all the data around.

Then this would be something like:

var pointOffset = geography.Points.Count; var filter = new MyGeographyPopulatingFilter(geography, _emitZ, _emitM); g.Apply(filter); if (filter.PointsAdded) { geography.Figures.Add( new Figure { FigureAttribute = figureAttribute, PointOffset = pointOffset, }); } return filter.PointsAdded;

Thanks! There's a lot of unnecessary allocating here with the enumerators too. We should do some profiling before we RTM.

Filed #13144

airbreather · 2018-08-28T12:25:09Z

src/EFCore.SqlServer.NTS/Storage/Internal/SqlServerGeometryTypeMapping.cs

+            var geometry = _reader.Read(((SqlBytes)value).Value);
+            var srid = geometry.SRID;
+
+            // TODO: This won't emit M


note: NetTopologySuite/NetTopologySuite#156 is the reference number

ajcvickers · 2018-08-29T16:44:59Z

src/EFCore.Relational/Query/ExpressionTranslators/IMemberTranslatorPlugin.cs

+    /// <summary>
+    ///     Represents plugin member translators.
+    /// </summary>
+    public interface IMemberTranslatorPlugin


@divega We should probably call out these plugin mechanisms explicitly in the preview2 blog post because they are generally useful outside of just spatial.

ajcvickers · 2018-08-29T16:47:56Z

src/EFCore.Relational/Storage/RelationalTypeMappingSource.cs

@@ -58,7 +58,19 @@ public abstract class RelationalTypeMappingSource : TypeMappingSourceBase, IRela
        /// </summary>
        /// <param name="mappingInfo"> The mapping info to use to create the mapping. </param>
        /// <returns> The type mapping, or <c>null</c> if none could be found. </returns>
-        protected abstract RelationalTypeMapping FindMapping(in RelationalTypeMappingInfo mappingInfo);
+        protected virtual RelationalTypeMapping FindMapping(in RelationalTypeMappingInfo mappingInfo)


Add to the provider log that base.FindMapping should be called?

ajcvickers · 2018-08-29T16:53:27Z

src/EFCore.SqlServer.NTS/NetTopologySuite.IO/Serialization/Figure.cs

+
+namespace NetTopologySuite.IO.Serialization
+{
+    internal class Figure


Internal because these move to NTS before RTM?

ajcvickers · 2018-08-29T17:01:02Z

src/EFCore.SqlServer.NTS/NetTopologySuite.IO/SqlServerSpatialReader.cs

+        public virtual bool HandleSRID
+        {
+            get => true;
+            set { }


Add API doc that setting is a no-op and why. Also for RepairRings.

ajcvickers · 2018-08-29T17:03:00Z

src/EFCore.SqlServer.NTS/NetTopologySuite.IO/SqlServerSpatialWriter.cs

+        /// <summary>
+        ///     Gets or sets whether the SpatialReference ID must be handled.
+        /// </summary>
+        public virtual bool HandleSRID


Likewise in this class

ajcvickers · 2018-08-29T17:04:19Z

src/EFCore.SqlServer.NTS/Properties/InternalsVisibleTo.cs

+
+using System.Runtime.CompilerServices;
+
+[assembly: InternalsVisibleTo("Microsoft.EntityFrameworkCore.SqlServer.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]


Probably not needed. I'll remove.

ajcvickers

Very nice!

bricelam · 2018-08-29T23:17:16Z

Looks like GeometryValueComparer isn't getting used. Need to dig into that before merging...

There's still a lot to do (e.g. support mapping to GEOGRAPHY), bit it's at a point where we can start gathering feedback while we continue iterating on it. To get started, install the `Microsoft.EntityFrameworkCore.SqlServer.NetTopologySuite` package, call `.UseSqlServer(..., x => x.UseNetTopologySuite())`, and add `Geometry` properties to your model. Part of #1100

bricelam mentioned this pull request Aug 24, 2018

Allow type mappings and member translators outside of the provider #13035

Closed

bricelam commented Aug 27, 2018

View reviewed changes

airbreather approved these changes Aug 28, 2018

View reviewed changes

ajcvickers reviewed Aug 29, 2018

View reviewed changes

ajcvickers approved these changes Aug 29, 2018

View reviewed changes

bricelam added the providers-fyi label Aug 29, 2018

bricelam mentioned this pull request Aug 30, 2018

Add Microsoft.EntityFrameworkCore.SqlServer.NetTopologySuite aspnet/Universe#1362

Merged

bricelam merged commit da777ce into dotnet:release/2.2 Aug 30, 2018

bricelam deleted the nts2 branch August 30, 2018 21:26

bricelam mentioned this pull request Oct 12, 2018

Make it easier for libraries to add IMemberTranslator implementations #11722

Closed

ajcvickers added the providers-beware label May 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SqlServer: Support spatial data via NTS #13115

SqlServer: Support spatial data via NTS #13115

bricelam commented Aug 24, 2018

bricelam commented Aug 27, 2018

bricelam Aug 27, 2018

ajcvickers Aug 29, 2018

bricelam Aug 27, 2018

bricelam Aug 27, 2018

airbreather left a comment

airbreather Aug 28, 2018

bricelam Aug 28, 2018

airbreather Aug 28, 2018

airbreather Aug 28, 2018 •

edited

Loading

bricelam Aug 28, 2018

airbreather Aug 28, 2018 •

edited

Loading

bricelam Aug 28, 2018

bricelam Aug 28, 2018

airbreather Aug 28, 2018

ajcvickers Aug 29, 2018

ajcvickers Aug 29, 2018

ajcvickers Aug 29, 2018

bricelam Aug 29, 2018

ajcvickers Aug 29, 2018

ajcvickers Aug 29, 2018

ajcvickers Aug 29, 2018

bricelam Aug 29, 2018

ajcvickers left a comment

bricelam commented Aug 29, 2018


		using System.Runtime.CompilerServices;

		[assembly: InternalsVisibleTo("Microsoft.EntityFrameworkCore.SqlServer.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100f33a29044fa9d740c9b3213a93e57c84b472c84e0b8a0e1ae48e67a9f8f6de9d5f7f3d52ac23e48ac51801f1dc950abe901da34d2a9e3baadb141a17c77ef3c565dd5ee5054b91cf63bb3c6ab83f72ab3aafe93d0fc3c2348b764fafb0b1c0733de51459aeab46580384bf9d74c4e28164b7cde247f891ba07891c9d872ad2bb")]

SqlServer: Support spatial data via NTS #13115

SqlServer: Support spatial data via NTS #13115

Conversation

bricelam commented Aug 24, 2018

bricelam commented Aug 27, 2018

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

airbreather 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

airbreather Aug 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

airbreather Aug 28, 2018 • edited Loading

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

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

ajcvickers left a comment

Choose a reason for hiding this comment

bricelam commented Aug 29, 2018

airbreather Aug 28, 2018 •

edited

Loading

airbreather Aug 28, 2018 •

edited

Loading