search

qgis / QGIS-Enhancement-Proposals

QEP's (QGIS Enhancement Proposals) are used in the process of creating and discussing new enhancements for QGIS
118 stars 37 forks source link

Support SAP HANA databases in QGIS #151

Closed stefanuhrig closed 3 years ago

stefanuhrig commented 5 years ago

QGIS Enhancement: Support SAP HANA databases in QGIS

Date 2019/07/24

Author Stefan Uhrig (@stefanuhrig)

Contact stefan.uhrig at sap dot com

maintainer @mrylov

Version QGIS 3.X

Summary

SAP HANA is an in-memory database with an OGC-compliant spatial engine. A free community edition of SAP HANA is available here.

SAP's customer base is in quite some demand for HANA support in QGIS. Most of those customers are already using QGIS, mostly with PostGIS, Oracle and/or MSSQL. They expect that they can work with HANA in QGIS in the same way they work with these databases.

We would like to add HANA support to QGIS. Additionally, we would like to ensure that QGIS users have the same experience when working with HANA as they have working with other databases.

Proposed Solution

We would like to add a new provider for SAP HANA to QGIS. We have prepared pull request qgis/QGIS#30734 for that.

Some people in the community have already raised concerns about adding a new provider to the QGIS repository and suggested alternatives like providing HANA support as plug-in or accessing HANA via GDAL.

However, we are concerned that we cannot achieve the same kind of integration into QGIS with these alternative approaches as we can by adding a new data provider to QGIS. The experience for our customers will then be different from working with other databases in QGIS. This will likely lead to limited acceptance and unsatisfied customers.

Affected Files

See qgis/QGIS#30734 for a list of files.

Backwards Compatibility

We don't expect any backward compatibility issues.

Votes

pcav commented 5 years ago

I would agree on that, provided there is a long term commitment by at least one developer to keep the provider up to date. Our pas experiences were problematic, as mentioned. BTW, it would be preferable to also include authentication.

nirvn commented 5 years ago

An alternative approach here would be to seize this opportunity to involve the relevant developers to insure serving a provider in the form of a plugin achieves the same level of integration as a core provider. @wonder-sk probably has more to say on that front.

I can't confirm it's currently the case but I'm very certain it should be :)

elpaso commented 5 years ago

Worth mentioning that it is also possible to develop a QGIS vector data provider in Python: https://github.com/qgis/QGIS/blob/master/tests/src/python/provider_python.py

nyalldawson commented 5 years ago

I'll not fully against the inclusion of a new provider, so long as:

We learnt our lesson hard with allowing the DB2 provider in, and now we're forever cursed with the burden of maintaining 1000s of lines of abandoned, broken code which none of the project team can even test!

Adding a new provider isn't just a free gift to the project. Rather it's more akin to giving someone an unexpected puppy for Christmas. Sure, it's all fun at first, but then there's always someone who inevitably has to feed it, clean up after it, pay the vet bills,... In this case, I don't want that someone to be the existing project maintainers or PSC. 🐶😆

3nids commented 5 years ago

we are concerned that we cannot achieve the same kind of integration into QGIS

can you precise your concerns here? I am not very familiar with provider API but I would have thought everything would be possible and perfectly integrated in QGIS.

jef-n commented 5 years ago
  • it's not better suited to being included in ogr itself

Every database we support can alternatively be accessed via OGR. Our providers are better integrated. And adding HANA to GDAL would just move the burden on Even a smaller group.

  • it's fully covered by unit tests, including relevant changes to the ci architecture to run these tests on Travis

To me that's not a requirement.

  • there's a formal commitment (possibly even including financial support) to maintain this provider over the long term.

We don't have formal commitments for any of the other providers either.

We learnt our lesson hard with allowing the DB2 provider in, and now we're forever cursed with the burden of maintaining 1000s of lines of abandoned, broken code which none of the project team can even test!

What's the difference to the sqlanywhere provider? That one we removed. We could also do that with DB2 or HANA, if it is orphaned and our burden gets to big.

vpicavet commented 5 years ago

Note that the potential licensing issue has been addressed here https://github.com/SAP/odbc-cpp-wrapper/issues/4

stefanuhrig commented 5 years ago

It is my understanding that the major concern is long term maintenance of the HANA data provider.

To be honest, it is quite hard to promise long term maintenance. Strategies and areas of investment can change quickly in larger companies. However, the contribution is not very intrusive in the code base. We added some lines to src/app/qgisapp.cpp, src/gui/qgsmanageconnectionsdialog.h and src/gui/qgsmanageconnectionsdialog.cpp. The rest of the code resides in its own subfolder. Therefore, it's easy to remove the HANA data provider again in case SAP does not invest in its maintenance anymore.

  • it's fully covered by unit tests, including relevant changes to the ci architecture to run these tests on Travis

The other data provider are not fully covered by unit tests either. In fact, they are barely covered, if they are covered at all. The Postgres tests just test some functions that encode and decode literals, but don't connect to a database. And there seem to be no unit tests for Oracle or MSSQL at all. It would also be quite hard to fully cover these data providers on Travis, as you would have to install and start a database.

  • there's a formal commitment (possibly even including financial support) to maintain this provider over the long term.

As already proposed by @jef-n I suggest QGIS drops support for the HANA data provider if it is not maintained any longer.

jef-n commented 5 years ago
  • it's fully covered by unit tests, including relevant changes to the ci architecture to run these tests on Travis

The other data provider are not fully covered by unit tests either. In fact, they are barely covered, if they are covered at all. The Postgres tests just test some functions that encode and decode literals, but don't connect to a database. And there seem to be no unit tests for Oracle or MSSQL at all. It would also be quite hard to fully cover these data providers on Travis, as you would have to install and start a database.

I don't know where you looked - there are many more postgresql tests and those are also run on travis. MSSQL is less good covered, but also run on travis. Oracle also has tests, but is not run on travis.

stefanuhrig commented 5 years ago

I don't know where you looked

I looked at tests/src/providers/. Are there additional tests I did not spot?

nyalldawson commented 5 years ago

The rest of the code resides in its own subfolder. Therefore, it's easy to remove the HANA data provider again in case SAP does not invest in its maintenance anymore.

Right - easy to remove the code, but what about the harm to users who have come to rely on this functionality? It's an extreme measure to remove existing features, and causes great harm to the reputation of QGIS.

The other data provider are not fully covered by unit tests either.

As @jef-n has pointed out this is incorrect. Look in the tests/SRC/python folder. Every vector provider (even the infamous DB2 provider) implements a full conformance test/stress test suite which ensures that they give consistent results with the same reference data sets. Implementing this is the bare minimum for inclusion as a provider (I'll gaurantee it finds some issues - it really stresses out the providers!).

Adding ci would mean the project can add additional tests to the conformance suite as we find bugs in other providers, and be confident that the new provider doesn't suffer the same issues (even if it's not available locally to test).

For better or worse, QGIS development in recent years is 99% test and CI driven. This work won't be in the 1% exception to this trend 😉

stefanuhrig commented 5 years ago

It's an extreme measure to remove existing features, and causes great harm to the reputation of QGIS.

I guess the harm to the reputation of SAP would be worse, as QGIS could justify the drop of support with the lack of a maintainer pretty well. Anyway, I see your point, and I think it would be an undesirable situation for both QGIS and SAP if the data provider was not maintained anymore. But we would have the same issue if the data source was consumed via OGR, wouldn't we?

Look in the tests/SRC/python folder.

Oh, I was not aware of these tests. These look more like integration tests than unit tests, but that's nitpicking... If this proposal was accepted, we would implement those conformance tests as well before contributing the code, of course.

I'll gaurantee it finds some issues - it really stresses out the providers!

I don't doubt that. We also contributed a JDBC plugin for HANA to geotools, and their JDBC plugin test suite was extremely helpful for identifying issues with our plugin.

pcav commented 5 years ago

I stress the point made by @nyalldawson here: QGIS reputation should be preserved above all.

m-kuhn commented 5 years ago

Worth mentioning that it is also possible to develop a QGIS vector data provider in Python: https://github.com/qgis/QGIS/blob/master/tests/src/python/provider_python.py

Is there any feedback on this? It looks like a perfect solution. Full integration into QGIS as desired - while clearly keeping the maintenance burden and update responsibility outside the QGIS project.

mhugo commented 5 years ago

@m-kuhn I totally agree. But apparently, tight integration with the main application is still missing. See for example the proposed additions to qgisapp.cpp https://github.com/qgis/QGIS/pull/30734/files#diff-989db6afac5dc91e42cf6d3e404431b3

I would need first to work on this integration so that a third party provider could be developed and maintained on its own and still allow for a tight integration with the main QGIS application, as @nirvn pointed out in an earlier comment. In my point of view that would be the best compromise.

m-kuhn commented 5 years ago

I agree.

This would probably require additional pull requests to allow better integration of plugin providers into the main code.

This can be done by @stefanuhrig , someone else from SAP (they seem to be comfortable with working on the QGIS source code as the existing pull request proves) or any contracted core committer.

stefanuhrig commented 5 years ago

Well, adding a core provider is rather simple as existing core providers can be used as template. Creating new extension points in QGIS requires in-depth knowledge of the application and its plug-in concept and is quite complex in comparison.

It seems unfair to me that QGIS has core providers for commercial databases, but does not allow to contribute support for our database in the same way it has been done for other commercial databases.

Even worse, it feels like we are being punished for a competitor neglecting the maintenance of its core provider, while the competitor was actually rewarded by the community taking over the maintenance of that provider.

nyalldawson commented 5 years ago

(please don't read this as an emotional response -- it's intended to be a friendly discussion and hopeful provide some wider context to the previous responses. I'm just not sure if I write well enough to convey the intended tone, sorry!)

@stefanuhrig

Well, adding a core provider is rather simple as existing core providers can be used as template. Creating new extension points in QGIS requires in-depth knowledge of the application and its plug-in concept and is quite complex in comparison.

Fair enough point. (On the other hand, I'm aware of at least 3 core QGIS developers who are actively working on providing a clean break between providers and the rest of the code who I'm sure would willing mentor and advise on the required changes.)

It seems unfair to me that QGIS has core providers for commercial databases, but does not allow to contribute support for our database in the same way it has been done for other commercial databases.

Unfair? maybe. Maybe in the same way that a first child might get different standards for parenting and have different rules from a second child, simply because the parents have learnt more about parenting due to their previous mistakes experiences! :laughing:

If db2 provider was submitted now it would 100% be deemed unsuitable for inclusion. Depending on your viewpoint, it's either just bad luck that the project learnt from the bad experience with db2 and has different standards now OR it's a very good thing which is leading towards increasingly stable software and a better experience for end users.

In hindsight, we made a huge mistake letting the db2 provider in (in its existing state). Back when it was introduced the project had already started making moves to a serious test-driven development style, and the wider development community had long accepted this as the new norm. We should have applied those same standards to the provider. But... we felt pressured due to the huge amount of work which had been put into the db2 provider, and in the end it was just too much effort to get the submitters to redo the work into the ideal state, and we took the least resistance path of accepting the PR. I'm sure other new contributors today, like @roya0045, would attest that we are extremely hard line on ALL new submissions now and are making every contributor, regardless of their employer, jump through the same hoops.

Now, it's the gross exception when any piece of work is accepted into QGIS without accompanying tests running on the CI architecture. That's a practice set in place by the majority of the QGIS core developers, and is unlikely to change in the future.

Again, you could easily argue this is a good or bad thing, depending on whether your are a developer submitting the PR or an end user just wanting stability in their GIS.

Even worse, it feels like we are being punished for a competitor neglecting the maintenance of its core provider, while the competitor was actually rewarded by the community taking over the maintenance of that provider.

(Honestly, if it helps ease the pain, I'd be +0 for totally removing disabling the db2 provider in its current state).

pcav commented 5 years ago

Hi @stefanuhrig I can understand your frustration. We at QGIS.ORG are proud of having always been open and fair, so let me stress the points already made by @nyalldawson : our project has undergone a steady and fast evolution, and our quality standards had improved a lot over time. It should not be surprising that what was acceptable a few years ago it's no longer acceptable nowadays. Our standards now require that every major code addition should be supported on the long term to be evaluated for inclusion in core, and I believe this is a very reasonable and fair requirement. Having some functions as optional (e.g. as plugins) makes everything easier. As for the existing code, of course removing it has a cost, especially for the credibility off the project, so it should be taken with caution. In this specific case, I agree that it may make sense to remove the provider. We should run a survey, however, to evaluate the impact of this decision. Rest assured your work is most welcome, and we will be happy of including it in a way that suits the best interests of the project and the user community.

pcav commented 5 years ago

After discussion in the PSC, we realized that the question is more complex than originally thought. We are working in devising a fair, pragmatic, and robust solution. Please be patient, we'll do our best.

mkemeter commented 5 years ago

Hi @pcav,

I appreciate that you take the time to discuss a viable solution for both sides. We do have a steady pull from customers running on a hybrid commercial/open source landscapes who want to integrate HANA Spatial with QGIS. We did already invest a fair amount of time in the development of the core provider for HANA and also we have learnt our lesson that we did not carefully study the QGIS enhancement processes before starting our development.

However given the current (unfortunate) situation would you be able to give an estimate by when we can discuss the way forward with regards to HANA integration in QGIS? Please also includes us in your ongoing discussion if you feel this would be helpful.

Many thanks in advance, Mathias (SAP HANA Spatial Team)

luipir commented 5 years ago

Worth mentioning that it is also possible to develop a QGIS vector data provider in Python: https://github.com/qgis/QGIS/blob/master/tests/src/python/provider_python.py

Is there any feedback on this? It looks like a perfect solution. Full integration into QGIS as desired - while clearly keeping the maintenance burden and update responsibility outside the QGIS project.

@stefanuhrig @mkemeter did you evaluate this proposal e.g. a plugin installing a python provider?

pcav commented 5 years ago

Hi @mkemeter thanks for understanding. We will discuss this case, in a more general framework, during the upcoming Contributors Meeting In Bucarest, in about one week. The discussion will be kept open. Regards.

stefanuhrig commented 5 years ago

@luipir We haven't really looked into this so far.

Can a python provider plug into the menus (e.g. Layer --> Add Layer --> Add HANA Spatial Layer...), into the Browser with a custom icon and provide its custom connection dialog?

pcav commented 5 years ago

We are just discussing about this, we'll come out with the outcomme soon. Thanks.

mkemeter commented 5 years ago

Hi @pcav, did you come to a conclusion in Bucharest? We are still keen on making this fly.

pcav commented 5 years ago

Yes, we are finalising a reply. Sorry for the delay.

mkemeter commented 5 years ago

Hi @pcav,

will any of you be joining State of the Map in Heidelberg or Intergeo in Stuttgart. @stefanuhrig and @mrylov will be joining State of the Map and I will be visiting Intergeo.

We would be more than happy to meet in person for a technical discussion.

Regards, Mathias

pcav commented 5 years ago

I will most likely not. You are welcome to ask in qgis-dev to check if someone will be there. Sorry about the delay.

m-kuhn commented 5 years ago

We might have someone from us at Intergeo, but it's not yet clear

pcav commented 5 years ago

Dear @mkemeter

We wanted to reach out to you regarding your proposed patch for QGIS to add support for Hana. As you have probably realised, there have been some concerns raised about the integration of this code. We don’t believe these are insurmountable but we would like to ask you if you could respond on a few points as outlined below.

Lastly, we would like to say big “thank you” for choosing QGIS as a platform for your Hana provider implementation - we do appreciate that having it in QGIS and the new opportunities that it will open for our users in enterprise environments. We hope you enjoy working with the QGIS Community and we look forward to having a long and fruitful association together! We would also like to invite you to consider financially supporting the QGIS project through our sustaining membership programme (https://www.qgis.org/en/site/getinvolved/governance/sustaining_members sustaining_members.html#qgis-sustaining-memberships) - as you probably realised from the discussions surrounding your pull request, there is a lot of community / volunteer effort that goes into maintaining and managing the QGIS project and financial support allows us to offload the maintenance burden introduced by adding new provider such as the Hana driver to ad hoc contractors or paid community members.

Best regards

The QGIS Community

mkemeter commented 5 years ago

Dear QGIS Community, dear @pcav,

thanks for your detailed answer. The majority of your points is what we have anticipated anyway. I guess it goes without saying that we want/need to take responsibility for the functionality, that we contribute.

Let me comment on the individual points to see if we can come to common ground:

Overall we do appreciate your positive answer and would like to setup a small task force to work on the open questions.

Let me know what you think, Mathias

pcav commented 5 years ago

Thanks, let's proceed in this direction. All the best.

mkemeter commented 5 years ago

We are following up with @mbernasocchi and @m-kuhn on this.

Many thanks, Mathias

gibuja commented 4 years ago

Dears all, Will this suggestion be implemented? I'm working now on a SAP HANA implementation, and we are interested in the QGIS solution for HANA database. Tks. Giovane.

mkemeter commented 4 years ago

Hi Giovane,

from SAP side we are working towards a resolution to contribute our plugin code. There is actually already an implementation existing and we are about to develop further tests.

However, if you want to use QGIS with SAP HANA today, you may consider using our GeoServer plugin to expose the views and tables as a WFS service which then can be consumed by QGIS.

You can find more information in this blog: https://blogs.sap.com/2019/03/28/how-to-connect-sap-hana-with-geoserver/

If you have further questions, do not hesitate to reach out via email (see https://github.com/mkemeter).

Regards, Mathias

mkemeter commented 4 years ago

Hi everyone,

as you can see we have opened a new pull request. I do hope, that we addressed all the points that have previously been discussed. Let me know what you think.

We actually planned to discuss the details this weekend in Den Bosch. That unfortunately did not work out :(

Regards, Mathias