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

Extending temporal search #182

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

rhysrevans3
Copy link
Contributor

@rhysrevans3 rhysrevans3 commented Jan 5, 2024

Related Issue(s):

Description:
Extending temporal search include start_datetime and end_datetime properties

PR Checklist:

  • Code is formatted and linted (run pre-commit run --all-files)
  • Tests pass (run make test)
  • Documentation has been updated to reflect changes, if applicable
  • Changes are added to the changelog

@rhysrevans3 rhysrevans3 marked this pull request as ready for review January 16, 2024 11:41
@jonhealy1
Copy link
Collaborator

@rhysrevans3 This pr looks really good. We have just been in the middle of adding opensearch support. Do you have any experience with opensearch?

@rhysrevans3
Copy link
Contributor Author

@jonhealy1 I've added some more tests but realised the current version of stac-pydantic the api is using doesn't allow a null datetime field. To update to the latest version of stac-pydantic will require changes to both stac-fastapi and stac-fastapi-elasticsearch. I've made the changes to stac-fastapi in this pull request and am working on the changes to elasticsearch. But I think this pull request will have to wait until they're merged.

@rhysrevans3
Copy link
Contributor Author

@rhysrevans3 This pr looks really good. We have just been in the middle of adding opensearch support. Do you have any experience with opensearch?

I don't have any experience with Opensearch but am happy merge main's changes into this branch and try to add this functionality to the opensearch parts.

@jonhealy1
Copy link
Collaborator

@rhysrevans3 Any new thoughts? Hopefully we can merge this soon.

@rhysrevans3
Copy link
Contributor Author

@jonhealy1 update on this pull request:

  • I've added the extended datetime functionality to the opensearch code.
  • I've added some extra tests to meet all the cases I can think of.
  • I've added some comments to explain the database logic.
    Some of the tests I've added are failing as the current version of stac-pydantic doesn't allow the datetime field to be null. So I'm still waiting on the fast-api pydantic v2 pull request to be merged

@jonhealy1
Copy link
Collaborator

Hi @rhysrevans3, the pydantic v2 is merged now into main via stac-pydantic if you want to have another look at this. Cheers.

@rhysrevans3
Copy link
Contributor Author

@jonhealy1 it looks like that stac-pydantic still won't allow a null datetime I thought the validator would allow this but I think the typing stops it. https://github.com/stac-utils/stac-pydantic/blob/main/stac_pydantic/item.py#L31-L58

Good news is there appears to be 3 pull requests that will fix this:
stac-utils/stac-pydantic#116
stac-utils/stac-pydantic#131
stac-utils/stac-pydantic#135

Sorry for the delay in this.

@jonhealy1
Copy link
Collaborator

Hi @rhysrevans3 - no problems whatsoever - thanks for staying on top of this!

@rhysrevans3
Copy link
Contributor Author

@jonhealy1 looks like this might be finally ready to merge. :)

@rhysrevans3 rhysrevans3 requested a review from jonhealy1 June 3, 2024 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants