Translate spatial aggregates by roji · Pull Request #28115 · dotnet/efcore

roji · 2022-05-27T12:55:20Z

src/EFCore.SqlServer.NTS/Query/Internal/SqlServerNetTopologySuiteAggregateMethodTranslator.cs

smitpatel · 2022-05-27T15:46:27Z

src/EFCore.Sqlite.NTS/Query/Internal/SqliteNetTopologySuiteAggregateMethodTranslator.cs

+        IDiagnosticsLogger<DbLoggerCategory.Query> logger)
+    {
+        if (source.Selector is not SqlExpression sqlExpression
+            || (method != GeometryCombineMethod && method != UnionMethod && method != ConvexHullMethod))


Why use different patterns in 2 providers in same PR. 😉

Only because in SQLite we need to compose the additional Collect before doing ST_ConvexHull...

smitpatel · 2022-05-27T15:47:51Z

Amazing. 🎉

roji · 2022-05-27T15:54:17Z

Amazing.

Yeah, it's really nice to see this working - thanks for doing all the infra for it!

bricelam

So cool to see this. EF6 never had them!

src/EFCore.SqlServer.NTS/Query/Internal/SqlServerNetTopologySuiteAggregateMethodTranslator.cs

...SqlServer.NTS/Query/Internal/SqlServerNetTopologySuiteAggregateMethodCallTranslatorPlugin.cs

src/EFCore.Sqlite.NTS/Query/Internal/SqliteNetTopologySuiteAggregateMethodTranslator.cs

test/EFCore.Specification.Tests/Query/SpatialQueryTestBase.cs

test/EFCore.SqlServer.FunctionalTests/Query/SpatialQuerySqlServerGeographyTest.cs

roji · 2022-05-27T20:34:16Z

FYI corresponding support for PostGIS: npgsql/efcore.pg@f77330d

roji · 2022-05-27T21:37:19Z

@smitpatel will hold off merging for a bit in case you have further comments.

roji · 2022-05-28T19:05:51Z

BTW @bricelam PostGIS has a few more spatial aggregate functions: https://postgis.net/docs/PostGIS_Special_Functions_Index.html#PostGIS_Aggregate_Functions

roji · 2022-06-04T17:20:45Z

FYI holding off merging this until we decide on top-level aggregate strategy, as per #28104 (comment).

roji · 2022-06-18T15:43:40Z

@smitpatel now that we've excluded top-level aggregates (#28264), I think we can come back to this (@bricelam approved for the spatial side but maybe you have comments on the query aggregate side).

smitpatel · 2022-06-20T23:02:05Z

src/EFCore.SqlServer.NTS/Query/Internal/SqlServerNetTopologySuiteAggregateMethodTranslator.cs

+            nullable: true,
+            argumentsPropagateNullability: new[] { false },
+            typeof(Geometry),
+            _typeMappingSource.FindMapping(typeof(Geometry), storeType));


Validate that sqlExpression has TypeMapping assigned (like other method/member translators in NTS. If you want to default fallback to geometry then assign the type mapping to sqlExpression first and then just propagate here.

Thanks, done. Note that I still look up the return type mapping since the sqlExpression's mapping may be for a specific geometry type (e.g. Point), but the aggregate methods return Geometry and the CLR type won't match.

smitpatel · 2022-06-21T16:23:07Z

src/Shared/Check.cs

+    [Conditional("DEBUG")]
+    [DoesNotReturn]
+    public static void DebugFail(string message)
+        => throw new Exception($"Check.DebugAssert failed: {message}");


Message points to DebugAssert :/

Closes dotnet#13278

ghost · 2022-06-21T17:51:35Z

Hello @roji!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2022-06-21T18:04:34Z

Apologies, while this PR appears ready to be merged, I've been configured to only merge when all checks have explicitly passed. The following integrations have not reported any progress on their checks and are blocking auto-merge:

Azure Pipelines

These integrations are possibly never going to report a check, and unblocking auto-merge likely requires a human being to update my configuration to exempt these integrations from requiring a passing check.

Give feedback on this

From the bot dev team

We've tried to tune the bot such that it posts a comment like this only when auto-merge is blocked for exceptional, non-intuitive reasons. When the bot's auto-merge capability is properly configured, auto-merge should operate as you would intuitively expect and you should not see any spurious comments.

Please reach out to us at fabricbotservices@microsoft.com to provide feedback if you believe you're seeing this comment appear spuriously. Please note that we usually are unable to update your bot configuration on your team's behalf, but we're happy to help you identify your bot admin.

roji requested review from bricelam and smitpatel May 27, 2022 12:55

roji mentioned this pull request May 27, 2022

Spatial: Translate aggregate functions #13278

Closed

roji commented May 27, 2022

View reviewed changes

src/EFCore.SqlServer.NTS/Query/Internal/SqlServerNetTopologySuiteAggregateMethodTranslator.cs Outdated Show resolved Hide resolved

smitpatel reviewed May 27, 2022

View reviewed changes

src/EFCore.SqlServer.NTS/Query/Internal/SqlServerNetTopologySuiteAggregateMethodTranslator.cs Show resolved Hide resolved

smitpatel reviewed May 27, 2022

View reviewed changes

bricelam suggested changes May 27, 2022

View reviewed changes

bricelam approved these changes May 27, 2022

View reviewed changes

roji mentioned this pull request Jun 5, 2022

Document new function translations in 7.0 dotnet/EntityFramework.Docs#3868

Closed

4 tasks

roji force-pushed the SpatialAggregates branch from 5ea42c9 to 9389247 Compare June 18, 2022 15:58

smitpatel reviewed Jun 20, 2022

View reviewed changes

roji requested a review from smitpatel June 21, 2022 14:52

smitpatel reviewed Jun 21, 2022

View reviewed changes

smitpatel approved these changes Jun 21, 2022

View reviewed changes

Translate spatial aggregates

d2d3679

Closes dotnet#13278

roji force-pushed the SpatialAggregates branch from 85d3262 to d2d3679 Compare June 21, 2022 17:51

roji added the auto-merge label Jun 21, 2022

ghost merged commit e257d1d into dotnet:main Jun 21, 2022

roji deleted the SpatialAggregates branch June 21, 2022 18:59

ajcvickers mentioned this pull request Jun 23, 2022

.NET Data Biweekly Updates (2022) #27185

Closed

This pull request was closed.

Conversation

roji commented May 27, 2022

Uh oh!

Uh oh!

Uh oh!

smitpatel May 27, 2022

Choose a reason for hiding this comment

Uh oh!

roji May 27, 2022

Choose a reason for hiding this comment

Uh oh!

smitpatel commented May 27, 2022

Uh oh!

roji commented May 27, 2022

Uh oh!

bricelam left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roji commented May 27, 2022

Uh oh!

roji commented May 27, 2022

Uh oh!

roji commented May 28, 2022

Uh oh!

roji commented Jun 4, 2022

Uh oh!

roji commented Jun 18, 2022

Uh oh!

smitpatel Jun 20, 2022

Choose a reason for hiding this comment

Uh oh!

roji Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

smitpatel Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

ghost commented Jun 21, 2022

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

ghost commented Jun 21, 2022

From the bot dev team

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.