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

Spatial: Translate aggregate functions #13278

Closed
bricelam opened this issue Sep 10, 2018 · 15 comments · Fixed by #28115
Closed

Spatial: Translate aggregate functions #13278

bricelam opened this issue Sep 10, 2018 · 15 comments · Fixed by #28115
Assignees
Labels
area-query area-spatial area-sqlite area-sqlserver closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Milestone

Comments

@bricelam
Copy link
Contributor

bricelam commented Sep 10, 2018

There are a handful of aggregate functions for spatial data. (Collect, Extent, Union, etc.)

It would be nice if we had a way to represent them in LINQ.

.NET SQL Server SpatiaLite PostGIS
EnvelopeCombiner.Combine(geoms) EnvelopeAggregate(@​geoms) Extent(@​geoms) ST_Extent(@​geoms)
GeometryCombiner.Combine(geoms) CollectionAggregate(@​geoms) Collect(@​geoms) ST_Collect(@​geoms)
UnaryUnionOp.Union(geoms) UnionAggregate(@​geoms) GUnion(@​geoms) ST_Union(@​geoms)
ConvexHull.Create(geoms) ConvexHullAggregate(@​geoms) ST_ConvexHull(Collect(@​geoms)) ST_ConvexHull(ST_Collect(@​geoms))
@ajcvickers ajcvickers added this to the Backlog milestone Sep 12, 2018
@bricelam
Copy link
Contributor Author

See also NetTopologySuite/NetTopologySuite#246

@roji
Copy link
Member

roji commented Oct 11, 2020

As this is being proposed for 6.0, would we build more generic support for arbitrary aggregate functions? There's #11850 but that seems to be about DbFunctions - do we need a separate issue to track provider aggregate functions?

/cc @smitpatel

@smitpatel
Copy link
Contributor

This is being proposed for 6.0? 😕

@roji
Copy link
Member

roji commented Oct 11, 2020

Yeah, #22951...

@roji
Copy link
Member

roji commented Oct 11, 2020

(a separate issue for tracking arbitrary non-DbFunction aggregate functions support may be a good idea regardless of 6.0, unless we just want to use #11850 to track that too)

@smitpatel
Copy link
Contributor

arbitrary non-DbFunction aggregate functions support

They are already supported. 🤷

@roji
Copy link
Member

roji commented Oct 11, 2020

Oh, nice!

@roji
Copy link
Member

roji commented Oct 11, 2020

Specifically for plugins (NTS), note that we don't have an extension point for QueryableMethodTranslatingExpressionVisitor, which is needed if we want to support top-level invocations of aggregate functions (similar to Max/Sum/Average).

@bricelam
Copy link
Contributor Author

@roji I added a table to the description in case you're feeling ambitious 😉

@roji
Copy link
Member

roji commented May 25, 2022

Ahahahaha I'm definitely in a very aggregate mood

roji added a commit to roji/efcore that referenced this issue May 27, 2022
roji added a commit to roji/efcore that referenced this issue May 27, 2022
roji added a commit to roji/efcore that referenced this issue May 27, 2022
@roji
Copy link
Member

roji commented May 27, 2022

Submitted PR #28115

Some notes:

  • CollectionAggregate:
    • For two points (0,0) and (1,1), NTS GeometryCombiner.Combine returns MULTIPOINT ((0 0), (1 1)) whereas SQL Server CollectionAggregate returns GEOMETRYCOLLECTION (POINT (0 0), POINT (1 1)). This is probably OK since NTS MultiPoint extends GeometryCollection, and GeometryCombiner.Combine's return type is Geometry.
    • A slightly more faithful translation for CollectionAggregate may be the GeometryCollection constructor, but unfortunately that accepts an array, not an IEnumerable (so we'd need to recognize and ignore ToArray over that). Translating constructor calls is also less easy.
  • EnvelopeAggregate:
    • Weirdly, NTS EnvelopeCombiner.Combine returns an Envelope, which is not a Geometry. Note that Geometry.Envelope does return Geometry, which is why we can translate the non-aggregate version (test).
    • It's possible to convert an Envelope into a Geometry via GeometryFactory.ToGeometry, but that makes the C#-side construct really complicated. We should probably wait for requests before looking into this.
  • Aggregate convex hull:
    • SQL Server also has ConvexHullAggregate (I have no idea what convex hulls are but they sound very cool!).
    • NTS has ConvexHull.Create which seems to correspond.
    • PostGIS and Spatialite don't have this, but the PostGIS docs suggest doing simply ST_ConvexHull(ST_Collect(geom)). We can do the same for Spatialite.

@bricelam
Copy link
Contributor Author

/cc @airbreather (In case you have any additional thoughts)

@airbreather
Copy link

airbreather commented May 27, 2022

  • CollectionAggregate:
    • For two points (0,0) and (1,1), NTS GeometryCombiner.Combine returns MULTIPOINT ((0 0), (1 1)) whereas SQL Server CollectionAggregate returns GEOMETRYCOLLECTION (POINT (0 0), POINT (1 1)). This is probably OK since NTS MultiPoint extends GeometryCollection, and GeometryCombiner.Combine's return type is Geometry.
    • A slightly more faithful translation for CollectionAggregate may be the GeometryCollection constructor, but unfortunately that accepts an array, not an IEnumerable (so we'd need to recognize and ignore ToArray over that). Translating constructor calls is also less easy.

Since these aggregate functions don't look like they're standardized in 06-104r4 like most other things tend to be*, not to mention how many decades these different tools have each been doing their own thing, I think some amount of unfaithfulness is to be expected when trying to translate between these same NTS calls and all of these different providers.

*If there's another standard I don't know about that everybody else but NTS is following, I'm sure we can add something to our core library to help facilitate things like this.

As long as the Geometry that NTS would produce is topologically equal to the Geometry that the provider gives us, then I think that's acceptable (though admittedly some users will have headaches with it).

  • EnvelopeAggregate:
    • Weirdly, NTS EnvelopeCombiner.Combine returns an Envelope, which is not a Geometry. Note that Geometry.Envelope does return Geometry, which is why we can translate the non-aggregate version (test).
    • It's possible to convert an Envelope into a Geometry via GeometryFactory.ToGeometry, but that makes the C#-side construct really complicated. We should probably wait for requests before looking into this.

Agreed: given that not all providers (no providers?) natively have the concept of "Envelope" as a distinct entity, this seems fairly wacky to implement and not-very-discoverable from the perspective of a user.

If there's a real user appetite for this, I wouldn't mind adding something like an EnvelopeCombiner.CombineAsGeometry that does basically the same thing that you're talking about here, just wrapped behind a single static method that you could detect.

  • Aggregate convex hull:
    • [...] (I have no idea what convex hulls are but they sound very cool!).

Short version is, it's the (unique) smallest convex polygon that covers all of the input geometries. There are other ways to get smaller polygons (this one's my favorite), but those won't be convex (short version of "convex" is that you can pick any two points in the polygon, and the line segment between them is fully covered by the polygon).

Another way to describe it is that it's the polygon that you would see if you stretched a rubber band around all the inputs and then let it "snap" around them all.

Correct. Or, at least, that was the intent of NetTopologySuite/NetTopologySuite@d826972, anyway :-)

@roji
Copy link
Member

roji commented May 27, 2022

If there's a real user appetite for this, I wouldn't mind adding something like an EnvelopeCombiner.CombineAsGeometry that does basically the same thing that you're talking about here, just wrapped behind a single static method that you could detect.

That would definitely help us out - we'd be able to translate that directly to its SQL equivalent (and I can imagnie it may even be useful in general to NTS users!).

Aggregate convex hull:

Thanks for the explanation, am not at all a spatial guy :) And yeah, ConvexHull.Create fits our needs perfectly.

@roji roji removed this from the Backlog milestone May 27, 2022
@roji roji self-assigned this May 27, 2022
@roji roji added the closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. label May 27, 2022
@ajcvickers ajcvickers added this to the 7.0.0 milestone May 31, 2022
@roji
Copy link
Member

roji commented May 31, 2022

Issue for addingEnvelopeCombiner.CombineAsGeometry to NTS: NetTopologySuite/NetTopologySuite#610

roji added a commit to roji/efcore that referenced this issue Jun 18, 2022
roji added a commit to roji/efcore that referenced this issue Jun 21, 2022
@ghost ghost closed this as completed in #28115 Jun 21, 2022
ghost pushed a commit that referenced this issue Jun 21, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0, 7.0.0-preview6 Jun 23, 2022
@ajcvickers ajcvickers modified the milestones: 7.0.0-preview6, 7.0.0 Nov 5, 2022
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-query area-spatial area-sqlite area-sqlserver closed-fixed The issue has been fixed and is/will be included in the release indicated by the issue milestone. type-enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants