search

go-spatial / tegola

Tegola is a Mapbox Vector Tile server written in Go
http://tegola.io/
MIT License
1.28k stars 195 forks source link

HANA database provider #893

Closed mrylov closed 1 year ago

mrylov commented 1 year ago

Dear Tegola community,

We, the SAP HANA Spatial Team, would like to add HANA support to the Tegola tile server. SAP HANA is an in-memory database with an OGC-compliant spatial engine. A free community edition of SAP HANA is available here.

Content

This pull request includes:

Requirements

The HANA provider requires the open-source library go-hdb that is used to connect to a HANA instance. Note, this driver requires at least go v.1.18 and to change some dependencies in the vendor directory.

License

The HANA provider is licensed under the MIT license.

Responsibilities

We promise to keep tracking of new issues/bugs, CI runs, API changes, documentation improvements related to the HANA provider and make sure that they are fixed/implemented promptly. There will be a contact person at SAP responsible for the maintenance.

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build ffc36ce66-PR-893

Changes Missing Coverage Covered Lines Changed/Added Lines %
provider/hana/error.go 0 17 0.0%
provider/hana/util.go 406 584 69.52%
provider/hana/hana.go 363 647 56.11%
<!-- Total: 801 1280 62.58% -->
Totals Coverage Status
Change from base Build 5e4c8d1cc: 1.6%
Covered Lines: 6546
Relevant Lines: 13956
💛 - Coveralls
ARolek commented 1 year ago

@mrylov wow! We sure weren't expecting this PR to show up.

We promise to keep tracking of new issues/bugs, CI runs, API changes, documentation improvements related to the HANA provider and make sure that they are fixed/implemented promptly. There will be a contact person at SAP responsible for the maintenance.

When I saw this PR my first thought was "Great! But who's going to help support this functionality?" I'm glad you addressed this. Once we get this through code review let's figure out how to best document this point of contact.

I have not had a chance to review this PR in depth, but my first glance review was looking at which tile providers you implemented. It looks like you're targeting both the normal tile provider and the mvt_provider, which is excellent. I did notice that it looks like there's some commented out code for the NewMVTTileProvider() func that might need to be addressed.

Also a quick request, can you please clean up the commits in this PR? Generally speaking we like commits broken into logical chunks as this helps with code review. Since you're vendoring code in this PR I usually step through the PRs commits not reviewing the commit with the vendored code.

Thanks for the contribution!

mrylov commented 1 year ago

Hi @ARolek, Thank you for your prompt reply. I'm really glad that you are willing to accept our contribution.

As you mentioned above, we did implement both types of providers. However, we did comment some code, as there was an issue in the implementation of the ST_AsMVT function on the HANA side. The issue must have been fixed now.

Regarding your request, I can reorganize the PR in the following commits:

Is this structure sufficient to start the review?

ARolek commented 1 year ago

As you mentioned above, we did implement both types of providers. However, we did comment some code, as there was an issue in the implementation of the ST_AsMVT function on the HANA side. The issue must have been fixed now.

Excellent. It would be great to support ST_AsMVT as it's usually more performant and less error prone than tegola's native geo processing.

Regarding your request, I can reorganize the PR in the following commits:

Looks good to me, with one additional request, please have a separate commit for code vendoring. If you want, you could even reduce this to 3 commits: Vendoring, NewTileProvider, NewMVTTileProvider.

Thanks!

mrylov commented 1 year ago

Looks good to me, with one additional request, please have a separate commit for code vendoring. If you want, you could even reduce this to 3 commits: Vendoring, NewTileProvider, NewMVTTileProvider.

Thank you @ARolek. What exactly do you mean by Vendoring? Do you mean any changes related to the vendor folder, as well as to the go.mod and go.sum files?

ARolek commented 1 year ago

Thank you @ARolek. What exactly do you mean by Vendoring? Do you mean any changes related to the vendor folder, as well as to the go.mod and go.sum files?

Correct. When you run go mod -vendor the database driver is being copied into the vendor folder. This is a lot of code, and it wont get reviewed with the PR. Breaking the code vendoring into a separate commit makes code review easier as I don't need to dig through the driver files.

mrylov commented 1 year ago

@ARolek, I've restructured the commits as you requested. In TestMVTForLayers, there are several TODO comments that need to be addressed once we update our HANA Cloud instance used in CI. We are planning to perform this update within 2-3 weeks.

ARolek commented 1 year ago

In TestMVTForLayers, there are several TODO comments that need to be addressed once we update our HANA Cloud instance used in CI. We are planning to perform this update within 2-3 weeks.

Sounds good! we can review the second commit for now. Enjoy the weekend.

mrylov commented 1 year ago

@ARolek, I've adjusted some tests in TestMVTForLayers as we updated our HANA instance.

ARolek commented 1 year ago

@mrylov

I've adjusted some tests in TestMVTForLayers as we updated our HANA instance.

Excellent! I still need to give this a review. Will do so soon. Can you please rebase against master? It looks like this branch has several conflicts.

mrylov commented 1 year ago

@ARolek, I'm looking forward. I've rebased the commits.

mrylov commented 1 year ago

Hi @ARolek, the PR is waiting for a review since end of November. Could you please tell me when you have time to review it? Thank you.

mrylov commented 1 year ago

Superb! Thank you very much @ARolek.

I will be responsible for supporting the HANA provider, so you can refer anyone to me if there are any questions or issues concerning this provider.

ARolek commented 1 year ago

I will be responsible for supporting the HANA provider, so you can refer anyone to me if there are any questions or issues concerning this provider.

Excellent. I just approved the CI to run on this PR, and the last thing we need is a review from @gdey. I have been in touch with him and it should happen shortly.

gdey commented 1 year ago

@ARolek Do we already have a PR that updates the UI? The failing test here is because of that. I really don't think that update should be part of this PR.

mrylov commented 1 year ago

@gdey Thank you very much for the prompt review.

I've added the code maintainer as you suggested in 63a67b0.

gdey commented 1 year ago

LGTM. Okay will approve it (looks like iOS Github does not let me do code reviews), once i can get to my computer.

gdey commented 1 year ago

@mrylov Can we have two things? So, I can merge this in.

  1. rebase it to the latest version of master
  2. squash it down to one commit.

Thank you!

mrylov commented 1 year ago

Can we have two things? So, I can merge this in.

@gdey, I've done as you said. Many thanks.

gdey commented 1 year ago

We will merge this in after #916; which fixes the UI issues the CI on this is breaking on.