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

feat(geospatial): add support for duckdb operations on literals #8570

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

ncclementi
Copy link
Contributor

Description of changes

This PR add support for duckdb geospatial operations directly on geospatial literals.

I gave this a try, I'm not sure this is the best way to go, but happy to iterate over it if there is a better way around.

Issues closed

Closes: #8143

cc: @gforsyth (thanks for the pointer to _run_pre_execute_hooks)

@ncclementi ncclementi force-pushed the geo-literals-ddb branch 2 times, most recently from a2df0fa to 6584523 Compare March 6, 2024 22:36
@cpcloud cpcloud added this to the 9.0 milestone Mar 7, 2024
Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

I think the framing of the unary op test case as binary op with no arguments is a bit awkward, but non-blocking.

Implementation looks 👌🏻

@cpcloud cpcloud added feature Features or general enhancements geospatial Geospatial related functionality duckdb The DuckDB backend labels Mar 7, 2024
@ncclementi
Copy link
Contributor Author

Just in case, Don't merge yet.

I'll take care of Jim's comment about the con he is right, and I'll split the tests in two separate ones. I was trying to get cute with it, but it'll be more confusing for future devs.

@ncclementi ncclementi force-pushed the geo-literals-ddb branch 3 times, most recently from d8cdb59 to dde5b2e Compare March 7, 2024 17:43
@ncclementi
Copy link
Contributor Author

This one is ready to go/ ready for the last review.

Copy link
Member

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@cpcloud cpcloud merged commit b4c4369 into ibis-project:main Mar 8, 2024
78 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duckdb The DuckDB backend feature Features or general enhancements geospatial Geospatial related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: geospatial literals don't work out of the box with the default backend
3 participants