Skip to content

Comments

Add support for database engine SAP Hana#8411

Merged
villebro merged 8 commits intoapache:masterfrom
axuew:master
Nov 12, 2019
Merged

Add support for database engine SAP Hana#8411
villebro merged 8 commits intoapache:masterfrom
axuew:master

Conversation

@axuew
Copy link
Contributor

@axuew axuew commented Oct 18, 2019

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

support for database engine SAP Hana
This feature requires the addition of a third library: sqlalchemy-hana and hdbcli
image

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

REVIEWERS

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

This looks much better than the previous PR. A few small change requests, but apart from that ok. Also, please add Hana to the list of officially supported dbs in docs/index.rst.

@mistercrunch
Copy link
Member

Oops, looks like something went wrong with this PR

@axuew
Copy link
Contributor Author

axuew commented Oct 22, 2019

Oops, looks like something went wrong with this PR

It has been resubmitted.

@axuew axuew requested a review from villebro October 22, 2019 06:28
@mistercrunch
Copy link
Member

black and isort are failing on the build. For isort, you'll have to rebase once this is in master #8423 .

For black, you can run the command black to lint your code, or if you want you can install the commit hooks so that it's done automatically every time you commit.
https://github.com/apache/incubator-superset/blob/master/CONTRIBUTING.md#git-hooks

@axuew
Copy link
Contributor Author

axuew commented Oct 23, 2019

black and isort are failing on the build. For isort, you'll have to rebase once this is in master #8423 .

For black, you can run the command black to lint your code, or if you want you can install the commit hooks so that it's done automatically every time you commit.
https://github.com/apache/incubator-superset/blob/master/CONTRIBUTING.md#git-hooks

Ok, we'll have time to solve this problem in a few days

@axuew
Copy link
Contributor Author

axuew commented Oct 26, 2019

black and isort are failing on the build. For isort, you'll have to rebase once this is in master #8423 .
For black, you can run the command black to lint your code, or if you want you can install the commit hooks so that it's done automatically every time you commit.
https://github.com/apache/incubator-superset/blob/master/CONTRIBUTING.md#git-hooks

Ok, we'll have time to solve this problem in a few days

It has been resubmitted. Please try again. Thank you

@rumbin
Copy link
Contributor

rumbin commented Oct 28, 2019

@axuew: Are you sure that the hana.py actually works?
I tested your current version and get the following error:

image

Judging by the raw query it constructs, it seems that the defined time grain functions are not being used; instead it looks like the PostgreSQL functions are employed:

image

This can also be seen from the options provided in the drop-down, which are more than you actually defined:

image

I'm not sure if it's my fault here at any point, though. But I would kindly ask you to double-check if the definition is working for you. And if it does, if you have any hints on why it doesn't for me...

@rumbin
Copy link
Contributor

rumbin commented Oct 29, 2019

FYI: There seems to be a bug when filtering on BOOLEAN values: There seems to be no way to enter the boolean value other than in a custom WHERE clause.

When using the Filters field in Explore View with Filter Select enabled, it displays the 0 and 1 as available values, but won't let you actually pick and confirm one of them, since the Save button is grayed-out:

image

When trying the same using a Filter Chart in a Dashboard, the value 0 or 1 is selectable, but the filtered charts contain no data, since the SQL query contains a WHERE predicate like AND my_boolean_column IN (NULL) instead of e.g. AND my_boolean_column IN (true).

image

I.e., the conversion of HANA's true/false to its representation as 1/0 in Python and back to SQL code is not working as it should, yet.

I don't know if such issues are supposed to be fixed in the initial PR, or if things like this may get addressed later on...

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

@axuew this is very close to being mergeable, hang in there! Please check the comments by @rumbin and myself, validate any changes locally and run black to format the code properly.

@rumbin the problem you picked up regarding booleans might affect other engines, too, so I propose opening a ticket about that once this gets finalized.

@axuew axuew requested a review from villebro November 11, 2019 12:30
@axuew axuew requested a review from villebro November 11, 2019 13:55
Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM; will leave this open for a day or so for the possibility for others to chime in, but I think this is good to go now.

@villebro villebro merged commit 763f59f into apache:master Nov 12, 2019
graceguo-supercat pushed a commit that referenced this pull request Nov 13, 2019
* Add support for database engine SAP Hana

* Support hana services

Increase time, minute, and second

* Fix hana return string

* Fix formatting errors
@mistercrunch mistercrunch added the 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels label Feb 28, 2024
@mistercrunch mistercrunch added the 🚢 0.36.0 First shipped in 0.36.0 label Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 0.36.0 First shipped in 0.36.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants