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

Timescale parity #374

Merged
merged 17 commits into from
Oct 19, 2020
Merged

Timescale parity #374

merged 17 commits into from
Oct 19, 2020

Conversation

c0c0n3
Copy link
Member

@c0c0n3 c0c0n3 commented Oct 5, 2020

This PR makes all QL API endpoints work with Timescale as a backend. From now on, we should be able to use either backend (Crate or Timescale) to store, query and delete NGSI entity data. We can even use both backends at the same time, routing requests to either DB depending on tenant. But notice there still are some differences between the two:

  • The Crate backend only supports arrays of strings whereas Timescale accepts any JSON array;
  • You can use any NGSI SLF type (point, line, polygon, box) with Timescale whereas Crate is restricted to points;
  • Crate doesn't support equality testing of geo shapes but Timescale does.

Notice this PR implements new modules and functions to fully support geo queries targeting Timescale---see changes in geocoding and sql packages as well as the timescale_geo_query module in the translators package. This PR also implements a number of fixes, tweaks and additions to make the existing translator modules work smoothly with both backends on inserting data and formatting query result sets into JSON---see changes in the translators package.

@c0c0n3
Copy link
Member Author

c0c0n3 commented Oct 5, 2020

@chicco785 so I've put together basically everything we need for Timescale as a backend in this PR, instead of splitting query support and all the rest (insert/formatting/other tweaks) in two separate PRs since it turned out to be less work this way. But there are still a few things I haven't got around to doing yet:

  • Geo equality. Equality isn't supported in Crate and the reporter blindly returns a 422 regardless of backend. That will have to change since the Timescale backend can actually do equality tests. Then the test_geo* modules should be updated accordingly so that in the case of Timescale we check equality works!
  • Health endpoint. Hardcoded to work with Crate only, we'll have to decide what to do here but I suppose it's best left out of this PR. Whatever we eventually do, test_health should be updated accordingly.
  • Reporter tests. They still need some love. At the moment there's a separate test suite for Timescale, but like you said we could just have one test suite by adding Timescale to the existing Docker compose file and then route query tests to either backend using the existing service argument. This works for the query tests, for these other tests we'll have to decide what to do:
    • test_integration: works with Crate, possibly not needed for Timescale.
    • test_multitenancy: ditto.
    • test_notify: test_no_value_no_type_for_attributes is broken but it looks like the test was wrong to start with. In fact, the test checks (among other things) that a numeric attribute (temperature) with a string value ('25') is inserted in Crate with a text column type. The Timescale backend inserts it as float, which is correct?
    • test_sql_injection: needs some massaging to make it work with Timescale too.
    • test_subscribe: ditto.
    • test_time_format: in principle these tests should be able to work with both backends but during test setup we connect to Crate.

@c0c0n3 c0c0n3 marked this pull request as draft October 5, 2020 08:05
@chicco785
Copy link
Contributor

@c0c0n3 any reason to keep it draft?

@c0c0n3
Copy link
Member Author

c0c0n3 commented Oct 5, 2020

@chicco785

any reason to keep it draft?

still working on it, would like to sort out the reporter tests...i.e. last point in the task list above

@c0c0n3
Copy link
Member Author

c0c0n3 commented Oct 6, 2020

I opened issues for all items in the above task list, see: #375, #376, #377, #378.
We can review and merge this PR...

@c0c0n3 c0c0n3 requested a review from chicco785 October 6, 2020 08:49
@c0c0n3 c0c0n3 marked this pull request as ready for review October 6, 2020 08:51
Copy link
Contributor

@chicco785 chicco785 left a comment

Choose a reason for hiding this comment

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

LGTM

@chicco785 chicco785 merged commit 05141cc into master Oct 19, 2020
@c0c0n3 c0c0n3 deleted the ts-queries branch October 28, 2020 12:43
@c0c0n3 c0c0n3 mentioned this pull request Feb 1, 2021
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.

None yet

2 participants