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

Do a much more thorough job with item times #728

Merged
merged 10 commits into from
May 12, 2021

Conversation

jisantuc
Copy link
Contributor

Overview

This PR makes a few changes now that stac4s models datetimes better:

  • it updates the datetime, start_datetime, and end_datetime fields as appropriate on item insert, bulk insert, and update
  • it updates the filter generators to test item search behavior
  • it fixes a bug I didn't know about where search datetime filters were only checking the datetime column
  • it significantly improves test coverage for search filter + item time interactions

Checklist

  • New tests have been added or existing tests have been modified

Notes

CI is gonna fail until the stac4s release that just happened is resolvable in Maven. Might be soon.

Testing Instructions

  • import a catalog on a fully up-to-date Franklin that's already been migrated
  • do some time filtering
  • everything should work as you expect
  • everything else is exercised by tests

Closes #724

@jisantuc
Copy link
Contributor Author

ha I assumed these failures were all because 0.4.0 of stac4s wasn't available yet oops. ANYWAY 🤞🏻 for this one

@jisantuc jisantuc requested a review from pomadchin May 12, 2021 23:18
Copy link
Contributor

@pomadchin pomadchin left a comment

Choose a reason for hiding this comment

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

💯

@@ -153,7 +153,7 @@ class CatalogStacImport(val catalogRoot: String) {
)
),
().asJsonObject,
forItem.properties,
forItem.properties.asJson.asObject.getOrElse(JsonObject.empty),
Copy link
Contributor

Choose a reason for hiding this comment

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

Ouf, I also don't know how to handle it better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I spent about four seconds adding an Encoder.AsObject in stac4s but decided not to -- it's not hard but it didn't seem important enough

Comment on lines +21 to +27
Some(
fr"(datetime >= $start AND datetime <= $end) OR (start_datetime >= $start AND end_datetime <= $end)"
)
case Some(start) :: _ =>
Some(fr"datetime >= $start")
Some(fr"(datetime >= $start OR start_datetime >= $start)")
case _ :: Some(end) :: _ =>
Some(fr"datetime <= $end")
Some(fr"(datetime <= $end OR end_datetime <= $end)")
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

@jisantuc jisantuc merged commit 5261cce into master May 12, 2021
@jisantuc jisantuc deleted the feature/js/insert-time-fields-on-insert branch May 12, 2021 23:41
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.

datetimes not inserted with item creation
2 participants