feat(db_engine_specs): added support for Denodo Virtual DataPort

[denodo-research-labs]

SUMMARY

Following up on #20656, this PR adds basic support for connecting to Denodo Virtual DataPort (aka VDP Server) from Superset.

This includes:

• A Denodo DB Engine Spec (in superset/db_engine_specs/denodo.py).
• Version configuration for the companion SQLAlchemy dialect (in pyproject.toml).
• Changes to the documentation in order to mention the new support (in docs/docs/configuration/databases.mdx).

ADDITIONAL INFORMATION

• [ ] Has associated issue:
• [ ] Required feature flags:
• [ ] Changes UI
• [ ] Includes DB Migration (follow approval process in SIP-59)
  • [ ] Migration is atomic, supports rollback & is backwards-compatible
  • [ ] Confirm DB migration upgrade and downgrade tested
  • [ ] Runtime estimates and downtime expectations provided
• [x] Introduces new feature or API
• [ ] Removes existing feature or API

[codecov[bot]]

Codecov Report

:x: Patch coverage is 86.66667% with 6 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 83.91%. Comparing base (76d897e) to head (267dbfd). :warning: Report is 2203 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/db_engine_specs/denodo.py	86.66%	6 Missing :warning:

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #29927       +/-   ##
===========================================
+ Coverage   60.48%   83.91%   +23.42%     
===========================================
  Files        1931      534     -1397     
  Lines       76236    38728    -37508     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    32499    -13615     
+ Misses      28017     6229    -21788     
+ Partials     2105        0     -2105     

Flag	Coverage Δ
hive	48.94% <68.88%> (-0.22%)	:arrow_down:
javascript	?
mysql	76.76% <68.88%> (?)
postgres	76.86% <68.88%> (?)
presto	53.40% <68.88%> (-0.40%)	:arrow_down:
python	83.91% <86.66%> (+20.41%)	:arrow_up:
sqlite	76.31% <68.88%> (?)
unit	60.90% <86.66%> (+3.27%)	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[rusackas]

Feel free to add the logo, too! They can appear on the website and readme.

You can also test compatibility and add it to this automated report: https://github.com/apache/superset/blob/master/superset/db_engine_specs/README.md

@betodealmeida might be able to provide a tiny bit of guidance on the latter.

[denodo-research-labs]

As @michael-s-molina commented on #29927 that new features can only be accepted for 4.1, we are focusing on this PR. We've just added a couple of changes to the db_engine_spec in order to fine-tune timestamp handling and resolve most of the items the pylint was complaining about.

Feel free to add the logo, too! They can appear on the website and readme.

Will do. Is there a specific order to be followed for adding logos? It's not alphabetic...

You can also test compatibility and add it to this automated report: https://github.com/apache/superset/blob/master/superset/db_engine_specs/README.md

@betodealmeida might be able to provide a tiny bit of guidance on the latter.

Yes please, any hints on how to do this would be useful.

[michael-s-molina]

As @michael-s-molina commented on https://github.com/apache/superset/pull/29927 that new features can only be accepted for 4.1, we are focusing on this PR.

@denodo-research-labs Ideally, you would only merge this to master and let the release manager determine in which minor version this should be included. For 4.1, the release manager is @sadpandajoe.

[michael-s-molina]

@denodo-research-labs If you wish to know more details about our release process you can check https://github.com/apache/superset/wiki/Release-Process

[denodo-research-labs]

@betodealmeida Any guidance on how to perform the compatibility tests and updating the automated report in https://github.com/apache/superset/blob/master/superset/db_engine_specs/README.md? Are we meant to execute superset db-test <URI> and then manually update the README file with its output, or is there any kind of automated way of doing this?

[denodo-research-labs]

Just checking: anything else needed from our side? Are those modifications on superset/db_engine_specs/README.md a requirement? If so, any guidance on how to perform them?

[denodo-research-labs]

We are trying to make plans regarding Superset support in some of our components: is there any possibility that this gets included in 4.1?

[villebro]

@denodo-research-labs this PR has a conflict against master branch. So at minimum, it will need to be rebased. Ideally, you should also add unit tests under tests/unit_tests/db_engine_specs (you can look at the existing tests for examples)

[denodo-research-labs]

@villebro PR rebased on master, added unit tests.

[betodealmeida]

@betodealmeida Any guidance on how to perform the compatibility tests and updating the automated report in https://github.com/apache/superset/blob/master/superset/db_engine_specs/README.md? Are we meant to execute superset db-test <URI> and then manually update the README file with its output, or is there any kind of automated way of doing this?

Running python superset/db_engine_specs/lib.py will update the table (you need to manually add the output to the README file), but we can do that later as well.

[denodo-research-labs]

Just added a new commit to make the automated PR code checks happier: replaced == None with is None in the unit test.

[denodo-research-labs]

Any chance this PR can get the review it still needs before 4.1 is out? This affects our plans for the integration of Apache Superset with some parts of our software, so if by now it is clear that this integration won't happen for 4.1, it would be great to know as soon as possible in order to remake our plans accordingly.

[villebro]

@denodo-research-labs please check CI results: a required lint check is still failing:

Other than that it looks good to go.

[denodo-research-labs]

Thanks @villebro

Isn't this the error that is being reported by the linter?

 ************* Module superset.db_engine_specs.denodo
superset/db_engine_specs/denodo.py:28:0: R0903: Too few public methods (0/2) (too-few-public-methods)

But there's not much that can be done about that in an implementation of a DB Engine Spec... it is basically a class only overwriting those attributes / methods that need to be overwritten. Perhaps we are missing anything else here?

[michael-s-molina]

@denodo-research-labs you can add # pylint: disable=too-few-public-methods to the _ErrorPatterns class.

[villebro]

Any chance this PR can get the review it still needs before 4.1 is out? This affects our plans for the integration of Apache Superset with some parts of our software, so if by now it is clear that this integration won't happen for 4.1, it would be great to know as soon as possible in order to remake our plans accordingly.

@denodo-research-labs as this is a new feature, I suspect it won't make it into 4.1 anymore, as the cut for this release has already been made (generally only bugfixes get cherried into the release after the cut). But let's try to get this merged asap so it doesn't miss the 5.0 cut.

[denodo-research-labs]

OK, now unless we've broken something :) both pylint and ruff should be happy. We added the pylint disable flag, and let ruff check and format the code.

@denodo-research-labs as this is a new feature, I suspect it won't make it into 4.1 anymore, as the cut for this release has already been made (generally only bugfixes get cherried into the release after the cut). But let's try to get this merged asap so it doesn't miss the 5.0 cut.

Thanks for the info @villebro. Is there a planned date for 5.0 already?

[villebro]

@denodo-research-labs you can check the release process here: https://github.com/apache/superset/wiki/Release-Process . Work for 5.0 should start soon after 4.1 is out.

[villebro]

It seems there's still some linting issues. I recommend setting up pre-commit hooks, as they'll automatically take care of simple linting issues like those in this PR: https://superset.apache.org/docs/contributing/development/#git-hooks

[denodo-research-labs]

We did that, but everything looks fine on our end:

$ git status
On branch denodo/master/db_engine_spec
Your branch is up to date with 'origin/denodo/master/db_engine_spec'.

$ pre-commit run --files superset/db_engine_specs/denodo.py
auto-walrus..............................................................Passed
mypy.....................................................................Passed
pip-compile-multi verify.............................(no files to check)Skipped
check docstring is first.................................................Passed
check for added large files..............................................Passed
check yaml...........................................(no files to check)Skipped
debug statements (python)................................................Passed
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
prettier.............................................(no files to check)Skipped
Blacklist................................................................Passed
Helm Docs............................................(no files to check)Skipped
ruff.....................................................................Passed
ruff-format..............................................................Passed
pylint...................................................................Passed

Unfortunately the GitHub task does not give much info, only that ruff is still not happy:

ruff.....................................................................Failed
- hook id: ruff
- files were modified by this hook

Found 1 error (1 fixed, 0 remaining).

ruff-format..............................................................Failed
- hook id: ruff-format
- files were modified by this hook

2 files reformatted, 1427 files left unchanged

pylint...................................................................Passed

But ruff says everything is alright:

$ ruff check superset/db_engine_specs/denodo.py
All checks passed!

Any pointers on how to find out what is happening? ruff version is 0.7.1, pylint 3.3.1

[denodo-research-labs]

@villebro found it, our fault. We were only focusing on ruff liking the denodo.py file, but what it was actually complaining about was that it needed to reformat 1 line of code in the unit test test_denodo.py.

Now pre-install runs for us with --all-files:

$ pre-commit run --all-files
auto-walrus..............................................................Passed
mypy.....................................................................Passed
pip-compile-multi verify.................................................Passed
check docstring is first.................................................Passed
check for added large files..............................................Passed
check yaml...............................................................Passed
debug statements (python)................................................Passed
fix end of files.........................................................Passed
trim trailing whitespace.................................................Passed
prettier.................................................................Passed
Blacklist................................................................Passed
Helm Docs................................................................Passed
ruff.....................................................................Passed
ruff-format..............................................................Passed
pylint...................................................................Passed

Sorry for all the fuss.

[denodo-research-labs]

Great to see this merged. Thanks for all the help @villebro @michael-s-molina @betodealmeida @rusackas