Associate spatial functions to each type

[adrien-berchet]

A nice improvement would be to associate only the relevant functions to each spatial type, instead of associating all functions to all types. With this feature we wouldn't have to specify the target type when it is different from Geometry (as described here: https://geoalchemy-2.readthedocs.io/en/latest/core_tutorial.html#use-raster-functions).

[papacodebear]

@adrien-berchet I've started to dig into this, but I'm noticing a build inconsistency which is preventing progress. The GitHub actions yml is set to test python versions 3.7 - 3.11 https://github.com/geoalchemy/geoalchemy2/blob/master/.github/workflows/test_and_publish.yml#L32, but the pre-commit hooks are configured to use only python 3.8 https://github.com/geoalchemy/geoalchemy2/blob/master/.pre-commit-config.yaml#L2. Can the 3.8 restriction be loosened? My local setup is using 3.11.

[adrien-berchet]

Hi @papacodebear !
Sure, it's no big deal. Just note that in the CI the lint job, which runs the pre-commit hooks, uses Py38. So if you use features from Python > 3.8 the pre-commit hook might be valid on your local setup but not in the CI.

[papacodebear]

@adrien-berchet Gotcha, sounds good and noted. I'm trying to create a branch, but I don't have permissions to. Would you mind adding me as a contributor? Also, I assume that I should be branching off of and creating PR's into master, correct?

[adrien-berchet]

Could you create a fork on your side to make the branch there? Actually I don't have permissions to add you as a contributor (I will try to ask for it, it would make sense that I get them now).
Yes you should target the master branch.
Thanks!

[adrien-berchet]

Hi @papacodebear, did you have some time to work on this? (just to know, no pressure ☺️ )

[papacodebear]

Hi @adrien-berchet , no worries! Yeah I've started to work on this, but the scope has definitely expanded.

The way I've started to tackle this is to define which functions actually belong to each type, which has been the majority of the effort. I started by taking the "master" list of functions from here, and tried to associate them with the right types, but then realized that PostGIS has also made updates over time, and so pulled the new list of functions directly from PostGIS.

Rasters
Geography
I'm looking at adding box functions as well, but haven't really started this yet. I've been trying to reconcile the existing set of functions with those that are just for the geometry type. I'm trying to prune those functions that were already in the master list here to include only those relevant to geometries, while also including any that may have been added to postgis in the meantime, but this is still in progress. I'm referencing this table .

I sent you an invite to the forked repository if you'd like to take a quick look at the progress. Any word on getting the collaborator access?

Edit: I realize the forked repository is public, my branch is here: https://github.com/papacodebear/geoalchemy2/tree/papacodebear/issue396
Edit: Separated functions are defined here: https://github.com/papacodebear/geoalchemy2/tree/papacodebear/issue396/geoalchemy2/functions

[adrien-berchet]

Very nice!
I didn't know this table (http://postgis.net/docs/manual-3.3/PostGIS_Special_Functions_Index.html#PostGIS_TypeFunctionMatrix), it's very interesting. And indeed, I guess most of the work is to translate this table into actual GeoAlchemy2 functions (and with proper specific compilations for other dialect than Postgis).
I did not read your code in details but I'm wondering one thing: why did you moved spatial types into one separated files?

[papacodebear]

Apologies for the delay, I moved the spatial types into individual files mostly for readability reasons, using the one class per module pattern. It can be helpful to know what types exist by looking in the directory. It's also helpful to reduce merge conflicts when multiple people are working on different types. I sort of see it as the same benefit as splitting the dialects here: https://github.com/geoalchemy/geoalchemy2/tree/master/geoalchemy2/types/dialects .

It's also going to become more important as we split the spatial functions per type, since the _FUNCTIONS variable is so large, we would want them in separate files for separate types.

I don't want to step on any toes, so let me know if you want me to change it back, but I think there are benefits to splitting it.

[adrien-berchet]

Ok, I see, that's interesting. In our case the type classes are very simple so it may be an overkill but we can still try this new organization.

[papacodebear]

Hi @adrien-berchet ,

I've split the functions per type, but now I'm running into an issue where I could use some guidance. I'm trying to accommodate the func use case, and having an issue with multiple possible types.

As an example, take ST_Intersection, which applies to both rasters and geometries.

Geometry: http://postgis.net/docs/manual-3.3/ST_Intersection.html
Raster: http://postgis.net/docs/manual-3.3/RT_ST_Intersection.html

If we're in a position where a user could write an expression like

with Session(engine) as session:
    geometry_test = table("geometrytest", column("id"), column("geom"), column("rast"))
    query = select(func.ST_Intersection(geometry_test.c.rast, geometry_test.c.geom))
    geom_row = session.scalars(query).one()

It's not obvious what the return type is unless either the user specifies it, or we keep a list of potential argument types and known return types. For instance the example here should return a set of geometry values, even though the first argument is a raster.

By my estimation, it looks like we're having to guess what the return type is based on arguments to the function.

This problem would occur in varying degrees of confusion for the following functions:

• ST_Intersection
• ST_Segmentize
• ST_SetSRID
• ST_SnapToGrid
• ST_Transform
• ST_Union

Should I look into building the list of possible arguments and corresponding return types?

[adrien-berchet]

Hi @papacodebear !
Sorry for the long delay, I had no time to answer when you posted this and then I completely forgot about this...
Anyway, I'm afraid you're right and we'll have to list all possible couples of argument types and return types. Then we'll have to register them with specific internal names. For example, ST_Intersection(geom, rast, band_num=1) could be registered as geom_rast_st_intersection, so the user could call geom.st_intersection(rast, band_num=1) and internally it would call geom_rast_st_intersection. But if the user works with core queries as you showed, I think he will have to specifically call something like select(func.geom_rast_ST_Intersection(geometry_test.c.geom, geometry_test.c.rast, band_num=1)) or select(func.ST_Intersection(geometry_test.c.geom, geometry_test.c.rast, band_num=1, type_=GeometrySet)) (though the GeometrySet type should be defined first).

[mbway]

Responding here to a question asked in #478 as to whether
typing.overload can be used to solve this problem.

I don't think it can be used to solve this problem as @overload is a type-level construct and in order to have the SQL
type be different depending on the input data types would require self.type to be assigned a different value at
runtime.

It looks like this should be possible at least in some situations:

https://github.com/sqlalchemy/sqlalchemy/blob/26f4b3e72f8a250e406f5d9871212b83cbc7cfc5/lib/sqlalchemy/sql/functions.py#L1385

SQLAlchemy provides ReturnTypeFromArgs(GenericFunction) which indicates that runtime introspection is possible. The case here is more complex than simply copying the input type to the output type but I think the same techniques should apply.