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

Make primary key requirements less restrictive in HANA #41136

Merged
merged 2 commits into from Jan 25, 2021

Conversation

mrylov
Copy link
Contributor

@mrylov mrylov commented Jan 22, 2021

This change allows creating a valid HANA provider with undefined primary key.

@github-actions github-actions bot added this to the 3.18.0 milestone Jan 22, 2021
Co-authored-by: Matthias Kuhn <matthias@opengis.ch>
@@ -71,6 +71,7 @@ class QgsHanaConnection : public QObject
QString getColumnDataType( const QString &schemaName, const QString &tableName, const QString &columnName );
int getColumnSrid( const QString &schemaName, const QString &tableName, const QString &columnName );
QgsHanaResultSetRef getColumns( const QString &schemaName, const QString &tableName, const QString &fieldName );
bool isTable( const QString &schemaName, const QString &tableName );
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bool isTable( const QString &schemaName, const QString &tableName );
bool isTable( const QString &schemaName, const QString &tableName ) const;

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently, we cannot change this to const because this method calls (indirectly via another non-const method) odbc::Connection::createStatment(), which is not const.

Actually, the question how const-correctness should look like in a database connector is quite interesting. Intuitively, one might expect that methods that don't change database objects are const. However, from a C++-language point of view, const-correctness relates to the state of C++objects.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should look at it from a pure c++ perspective. And use mutable to hide away implementation details.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had a closer look at the QgsHanaConnection class. Currently, no method is const. For consistency, I'd keep the new method as not const as well.

It's not quite clear to me, which methods should be const in this class. Most of them require the creation of a statement, which is a modification of the mConnection member.

Is it okay to keep it non-const for the time being and to come up with a criteria for making methods const in this class @m-kuhn? We could then do the constness changes in a separate follow-up pull request.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@m-kuhn m-kuhn merged commit 7ba68b3 into qgis:master Jan 25, 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

3 participants