Add WKB Support for PostGIS Geometry Columns #9951
Conversation
The implementation of `Geometry` reads needs to map `geometry` ColumnType to `GEOMETRY` Type which is imported from trino-geospatial dependency. And in order to make the new Connector pass through test environment, `TYPE_MANAGER` of `TestPostgreSqlClient ` and `ConnectorContext` of `TestPostgreSqlPlugin` need to have `GEOMETRY` Type. Also, `PostgreSqlQueryRunner` needs to install `GeoPlugin`.
Specifically, it overrides `buildSql` method from `BaseJdbcClient` to make Query for `geometry` Type column surrounded by `ST_AsBinary` function. Then we will get WKB content from JDBC-driver. After that, we use `stGeomFromBinary` function in `trino-geospatial` plugin to get trino `Geometry` Type from WKB.
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@trino.io. For more information, see https://github.com/trinodb/cla. |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
I'm sorry, it seems that I'm making a mistake on my branch name (I'm supporting things more than read stuff). |
|
@slhmy Sorry for the delay. Is this ready to be reviewed? I can take a look later today or tomorrow. |
|
Regarding the idea of adding a config to control this - I'm not sure I understand. If a Postgres doesn't have PostGIS there would be no columns of geometry type (do I understand incorrectly?) so the type mapping you add won't get invoked - so it shouldn't matter if PostGIS is present or not? Or if you mean that Postgres can have GEOMETRY columns even without PostGIS then I think a better way to handle this is to check whether the column comes from PostGIS or not and use the correct function accordingly. (I'm not sure if it's possible to detect PostGIS, just an idea.) The downside of configs is that it's one more toggle for users to enable, to add tests for and doesn't make things "just work". |
@hashhar If tests are not necessary, I think, yes 😗.
It's true, I agree. The main reason driving me to the config work is the test context stuff (you can see But if it's okay to put things together, I think I'm worrying too much.
Oh I'm not sure, either 😂. For me it's okay not to add config. |
| if (jdbcTypeHandle.getJdbcTypeName().isPresent() && jdbcTypeHandle.getJdbcTypeName().get().equals("geometry")) { | ||
| String columnName = column.getColumnName(); | ||
| log.debug("Find geometry type, changing '%s' to '%s'", columnName, "ST_AsBinary(\"" + columnName + "\")"); | ||
| supposedColumnExpressions.put(columnName, "ST_AsBinary(\"" + columnName + "\")"); |
There was a problem hiding this comment.
cc: @ebyhr since you recently were looking into ways to apply functions to columns on the read path. Do you know of a better alternative?
There was a problem hiding this comment.
Could you add a new type mapping test using https://hub.docker.com/r/postgis/postgis?
- Add
TestingPostGisServerlikesTestingPostgreSqlServer - Add
TestPostGisTypeMappinglikesTestPostgreSqlTypeMapping
Thanks for suggestion. I'll try this and notify you as soon as I finish. |
|
@ebyhr |
|
👋 @slhmy - this PR has become inactive. If you're still interested in working on it, please let us know, and we can try to get reviewers to help with that. We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks. |
|
Closing this one out due to inactivity, but please reopen if you would like to pick this back up. |
Close #5580.
I'm not looking forward to a quick merge, for there is still something I'm not sure about.
Just trying to push works to go on.
About geometry read
This solution needs extra dependency
trino-geospatial, and changes are mainly related with theGeometryTypein this dependency.Since
GeometryTypeis not a type in SPI, the using of it is not available in test environment. So if I don't make changes to test, I will fail while installing this plugin. However this change only effects postgresql-connector's test environment. And that means the next time if other connectors need Geometry read ability, we still have to do similar changes.So, if could, works to contain
GeometryTypeinto test environment should be considered.Also, I'm not sure overriding
buildSqlmethod is suggested, but it's necessary for the implementation. It automatically change query likeSELECT geom FROM GeometriesintoSELECT ST_AsBinary(geom) FROM Geometries.ST_AsBinarywill make the binary of PostGIS Geometry into a pure WKB, then we can use ColumnMapping like other types do.About geometry insert
I‘ll do this recently. Basically, it will override
getBindExpressionforgeometryWriteFunction, and this will enables me to have chance to wrap aST_GeomFromWKBfunction to wrap around geometry value.I get the idea from Geotools's PostGISPSDialect, which I think is the way Geotools uses to insert geometries.
About config
I'm deciding adding an
enablePostGISFeaturesconfig for connector, currently it's only related with geometryColumnMapping stuff. But I think it's better to manage all the PostGIS features in a same config.Plan
To see things I finished & haven't finished:
PostGISextension)Regrets
It's my first try in trino, I'm not so familiar with the whole project. Any change will be welcomed. My speed of reply may be a little bit slow, thank for you understanding.❤️️