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 geo.{intersects, distance, length} #50

Merged
merged 13 commits into from
Jan 23, 2024

Conversation

srepmub
Copy link
Contributor

@srepmub srepmub commented Nov 29, 2023

updated query parsing for geo.* functions, and added django backend implementation.

-add GEOGRAPHY token and Geography AST node
-account for identifier namespace in various places
-add django implementation using django.contrib.gis
-use '__' for namespace separator in backend function names
-fix geo.distance number of args

updated query parsing for geo.* functions, and added django backend
implementation.

-add GEOGRAPHY token and Geography AST node
-account for identifier namespace in various places
-add django implementation using django.contrib.gis
-use '__' for namespace separator in backend function names
-fix geo.distance number of args
this allows for syntax like:

OData.CSC.Intersects(area=geography'SRID=4326;Point(1 2)')

-add NamedParam ast Node (with name, param attrs)
-add '=' literal
-add list_named_parameters grammar (similar to list_expr)
-only check args for functions with namespace () or ('geo',)
-in django_q visitor, passed NamedParams as keyword args

externally, we can now use the following to support above query:

class AstToDjangoQVisitorCSC(AstToDjangoQVisitor):
    def djangofunc_odata__csc__intersects(self, area):
        return super().djangofunc_geo__intersects(ast.Identifier('footprint'), area)
@OliverHofkens OliverHofkens self-requested a review December 1, 2023 07:38
@OliverHofkens OliverHofkens added the enhancement New feature or request label Dec 1, 2023
@OliverHofkens
Copy link
Member

Hey, thanks for this, it's looking great! 👍

Two things I'd like to add (or see added) are:

  • Being able to still run the project without having the GIS libraries installed. Currently this crashes on import of the django.contrib.gis package.
  • Some tests on these features. Although that might be hard if they depend on databases with GIS support. Something to think about.

I'd happily work on those. It would be a good opportunity for me to learn some Geo/GIS stuff!

@OliverHofkens
Copy link
Member

Just for traceability mentioning that this implements #48

@srepmub
Copy link
Contributor Author

srepmub commented Dec 4, 2023

thanks for adding checks and tests! :) I somehow missed the tests, sorry.

something else I forgot was to improve the argument names and typing for djangofunc_geo__*. I will do this shortly.

@OliverHofkens
Copy link
Member

Hey, great work so far!

I finally found some time to work a bit on this again.
I managed to get some end-to-end integration tests working on this branch: https://github.com/gorilla-co/odata-query/tree/tests/geo-integration-tests

Still ironing out some minor issues. E.g. certain Python versions fail to find the GDAL libraries, while others work fine.

@svniemeijer
Copy link

Any news on this? I had a look at the failed test and it looked like a linting error (which should probably be easy to resolve).
It would be great if this feature could get integrated.

@OliverHofkens
Copy link
Member

Hey, sorry about the wait! I've been experimenting with some end-to-end integration tests using a local Spatialite database on this branch: https://github.com/gorilla-co/odata-query/tree/tests/geo-integration-tests

That's turning out to be a bit of a pain, some versions of Python seem to have issues finding/using the geo libraries. Afterwards I'd still have to get that set up in the Github actions workflows.
If you need this urgently, I'd be willing to merge this as-is and release it under a pre-release tag.

Sorry again, and thanks for your patience 🙏

@OliverHofkens OliverHofkens self-assigned this Jan 21, 2024
@OliverHofkens
Copy link
Member

Bingo, we now have E2E integration tests using a Spatialite database running in Github Actions.
Locally they should be automatically skipped if the required system libraries cannot be loaded 👍

Adding some colleagues as reviewers for a quick final checkoff and then I'll merge. Thanks again for this!

@OliverHofkens OliverHofkens merged commit 18a422a into gorilla-co:master Jan 23, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants