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

Support Spatial Types for PostGIS #1927

Merged
merged 65 commits into from
Nov 30, 2024
Merged

Conversation

Pipboyguy
Copy link
Collaborator

@Pipboyguy Pipboyguy commented Oct 4, 2024

Description

This PR adds support for PostGIS GEOMETRY type in PostgreSQL. It introduces a logical type for GEOMETRY on binary or text fields.

Users can declare geometry columns via the postgres_adapter, enabling direct insertion from WKT, WKB and WKB hex representations, in addition to GeoPandas integration with automatic geometry column detection and conversion.

Related Issues

Limitations

  • LinearRing only works with wkb type (python binary type)
  • EWKB (Extended WKB) is not yet supported
  • geopandas can be supported as well, but will need more work

Typical usage pattern:

postgres_adapter(geodata_resource, geometry=["geom"], srid=3857)

@Pipboyguy Pipboyguy added enhancement New feature or request destination Issue related to new destinations community This issue came from slack community workspace labels Oct 4, 2024
@Pipboyguy Pipboyguy self-assigned this Oct 4, 2024
@Pipboyguy Pipboyguy linked an issue Oct 4, 2024 that may be closed by this pull request
@Pipboyguy Pipboyguy marked this pull request as draft October 4, 2024 13:28
Copy link

netlify bot commented Oct 4, 2024

Deploy Preview for dlt-hub-docs ready!

Name Link
🔨 Latest commit 34bdfbf
🔍 Latest deploy log https://app.netlify.com/sites/dlt-hub-docs/deploys/67474da1d5ce43000814701a
😎 Deploy Preview https://deploy-preview-1927--dlt-hub-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Collaborator

@burnash burnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @Pipboyguy
I have a couple of questions, please see my comments.

@Pipboyguy Pipboyguy requested a review from burnash November 6, 2024 15:26
@Pipboyguy Pipboyguy requested a review from rudolfix November 14, 2024 21:38
@rudolfix rudolfix removed the request for review from burnash November 25, 2024 10:26
@Pipboyguy Pipboyguy requested a review from burnash November 25, 2024 10:42
Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is really good!

  1. my suggestion is that we drop wkb and just keep wkb_text. also move shapely to dev dependencies
  2. IMO csv with WKT and WKB_HEX should work! did you try it? it'd be hard to understand why not....

tests/load/postgres/utils.py Show resolved Hide resolved
) and not file_path.endswith("insert_values"):
# Only insert_values load jobs supported for geom types.
# TODO: This isn't actually true, can make it work with geoarrow!
raise TerminalValueError(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it really true? if column is text and contains WKT data it should get inserted. did you try it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added csv loader cases to the geo test, and it all passes. Your intuition was entirely correct!

@@ -56,7 +67,12 @@ def escape_postgres_literal(v: Any) -> Any:
if isinstance(v, (list, dict)):
return _escape_extended(json.dumps(v))
if isinstance(v, bytes):
return f"'\\x{v.hex()}'"
if is_valid_wkb(v):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is very impractical :/ we have a dependence on shapely and there may be false positives (there are no magic numbers in wkb). passing expected type to escape...literal will slow this down

we could use a special binary type but user would need to wrap binary anyway. so my take is to drop wkb and just keep wkb_hex.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, removed wkb support, will update docs too

@Pipboyguy Pipboyguy requested a review from rudolfix November 27, 2024 16:52
Copy link
Collaborator

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rudolfix rudolfix merged commit 09914a3 into devel Nov 30, 2024
57 of 59 checks passed
@rudolfix rudolfix deleted the 696-support-spatial-geo-types branch November 30, 2024 19:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community This issue came from slack community workspace destination Issue related to new destinations enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Support spatial / geo types
3 participants