Skip to content

Conversation

YuriZmytrakov
Copy link
Contributor

@YuriZmytrakov YuriZmytrakov commented Sep 15, 2025

Related Issue(s):

#403

Description:

This PR introduces a new env var USE_DATETIME to control the datetime filtering behavior in the search.

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

@YuriZmytrakov YuriZmytrakov marked this pull request as draft September 15, 2025 15:44
@YuriZmytrakov YuriZmytrakov changed the title first commit USE_DATETIME env var for filter behavior Sep 15, 2025
@YuriZmytrakov YuriZmytrakov force-pushed the CAT-1413-2 branch 5 times, most recently from dbb1831 to ad18cb9 Compare September 16, 2025 14:21
@YuriZmytrakov YuriZmytrakov marked this pull request as ready for review September 16, 2025 14:27
Copy link
Collaborator

@jonhealy1 jonhealy1 left a comment

Choose a reason for hiding this comment

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

@YuriZmytrakov This is really useful. 1. changelog update. 2. we should probably have a test for this.

CHANGELOG.md Outdated
- `STAC_INDEX_ASSETS` environment variable to allow asset serialization to be configurable. [#433](https://github.com/stac-utils/stac-fastapi-elasticsearch-opensearch/pull/433)
- Added the `ENV_MAX_LIMIT` environment variable to SFEOS, allowing overriding of the `MAX_LIMIT`, which controls the `?limit` parameter for returned items and STAC collections. [#434](https://github.com/stac-utils/stac-fastapi-elasticsearch-opensearch/pull/434)
- Updated the `format_datetime_range` function to support milliseconds. [#423](https://github.com/stac-utils/stac-fastapi-elasticsearch-opensearch/pull/423)
- Added `USE_DATETIME` environment variable behavior to set datetime filtering logic. [#443](https://github.com/stac-utils/stac-fastapi-elasticsearch-opensearch/pull/443)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi. Any new changes need to be put under the Unreleased section as v6.3.0 has already been released.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonhealy1 I have moved my changes to Unrealeased section. Thank you!

@YuriZmytrakov YuriZmytrakov force-pushed the CAT-1413-2 branch 3 times, most recently from af1a390 to 969e7a8 Compare September 19, 2025 10:05
@YuriZmytrakov
Copy link
Contributor Author

@YuriZmytrakov This is really useful. 1. changelog update. 2. we should probably have a test for this.

Yes, we can do that.

CHANGELOG.md Outdated

## [Unreleased]

- Added `USE_DATETIME` environment variable behavior to set datetime filtering logic. [#443](https://github.com/stac-utils/stac-fastapi-elasticsearch-opensearch/pull/443)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a, ### Added, header above this entry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thank you!

README.md Outdated
| `STAC_ITEM_LIMIT` | Sets the environment variable for result limiting to SFEOS for the number of returned items and STAC collections. | `10` | Optional |
| `STAC_INDEX_ASSETS` | Controls if Assets are indexed when added to Elasticsearch/Opensearch. This allows asset fields to be included in search queries. | `false` | Optional |
| `ENV_MAX_LIMIT` | Configures the environment variable in SFEOS to override the default `MAX_LIMIT`, which controls the limit parameter for returned items and STAC collections. | `10,000` | Optional |
| `USE_DATETIME` | Configures the environment variable to control datetime filtering behavior. When `true`, searches by datetime field, and if datetime is `null` then by start/end datetime fields. When `false`, always searches only by start/end datetime fields. | true | Optional |
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we need to have a test for this new functionality

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added tests for the use_datetime variable (true/false) to verify changes in filter behavior.

@YuriZmytrakov YuriZmytrakov force-pushed the CAT-1413-2 branch 9 times, most recently from e2b72de to 2132c43 Compare September 21, 2025 10:58
Copy link
Collaborator

@jonhealy1 jonhealy1 left a comment

Choose a reason for hiding this comment

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

Looks great. Tests are great, just the changelog issue to look at.

CHANGELOG.md Outdated

### Added

- Added `USE_DATETIME` environment variable behavior to set datetime filtering logic. [#443](https://github.com/stac-utils/stac-fastapi-elasticsearch-opensearch/pull/443)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @YuriZmytrakov - this entry is still under the v6.3.0 section. It needs to be moved to the Unreleased section above. You will need to add an '### Added' header to the Unreleased section and move your pr entry there.

[Unreleased]

Changed

  • updated numReturned & numMatched fields in itemCollection return to numberReturned & numberMatched. #446

[v6.3.0] - 2025-09-16

Added

  • Added USE_DATETIME environment variable behavior to set datetime filtering logic. #443

Copy link
Contributor Author

@YuriZmytrakov YuriZmytrakov Sep 22, 2025

Choose a reason for hiding this comment

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

Apologies, I was working on not the latest version, I have moved comment in changelog under Unreleased Added section: 3b36f72

@YuriZmytrakov YuriZmytrakov force-pushed the CAT-1413-2 branch 2 times, most recently from d24d291 to 3b36f72 Compare September 22, 2025 07:35
@jonhealy1
Copy link
Collaborator

@YuriZmytrakov Looks like errors with Opensearch?

@YuriZmytrakov YuriZmytrakov force-pushed the CAT-1413-2 branch 2 times, most recently from 5d99d46 to 2528ea2 Compare September 23, 2025 13:46
Yuri Zmytrakov added 2 commits September 23, 2025 16:07
- Added USE_DATETIME env var to control datetime filtering
- USE_DATETIME=True (default): use existing logic that handles both datetime and start/end datetime fields
- USE_DATETIME=False (default): use only start/end datetime fields for search
- test_use_datetime_true: Verify USE_DATETIME=true finds looks up by datetime, if null then by start/end datetime range
- test_use_datetime_false: Verify USE_DATETIME=false only search items with start/end datetime values
@YuriZmytrakov
Copy link
Contributor Author

@jonhealy1 I had to cherry-pick my commits into new branch, as this one keeps failing. Please take a look at the new PR:
#452

@jonhealy1
Copy link
Collaborator

@YuriZmytrakov Do you know why it was failing? Did you change something in the new version?

@YuriZmytrakov
Copy link
Contributor Author

@YuriZmytrakov Do you know why it was failing? Did you change something in the new version?

Yes, I was not able to rebase this branch on the latest main. There was no any changes in the code. @jonhealy1

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