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

fix: Teradata connection fixes #15930

Closed
wants to merge 42 commits into from
Closed

Conversation

mccushjack
Copy link
Contributor

SUMMARY

fixes to Teradata driver and limit methods in teradata.py

110385813-a7590200-801c-11eb-95c4-ea227cd032c4
110385894-c6579400-801c-11eb-8a54-fe4e0f938157
110385924-cfe0fc00-801c-11eb-8b10-d2b169d3545e

TEST PLAN

This solution works and is tested with Teradata Vantage, but we are happy to make other modifications if the team supports.

ADDITIONAL INFORMATION

The request all realtes to the follwoing bug

New or Changed Public Interfaces
The present proposal requires no new or changed public interfaces.

New dependencies

Only for Teradata connections, the present proposal requires the installation of

the teradatasqlalchemy connector, and
the teradatasql driver
packages to connect to a Teradata Vantage system Advanced SQL Engine Database.
These packages are actively maintained and their licenses are Teradata proprietary. The teradatasql license can be found at:
https://github.com/Teradata/python-driver/blob/master/LICENSE
whereas the teradatasqlalchemy code is not publicly posted at this point.

Migration Plan and Compatibility
No migration is necessary. The proposed code changes are an extension and non-destructive to the existing code, thus not affecting existing compatibility.

@villebro
Copy link
Member

@mccushjack I'm going through old unreviewed PRs, and sadly this valuable PR had gone unreviewed. Can you rebase this PR to resolve the conflict? I'll finish reviewing this once that gets done.

@villebro
Copy link
Member

@mccushjack oh no, this is conflicted again 🙁 Would you mind rebasing one more time and pinging me when you're done so I can get this merged?

@junlincc
Copy link
Member

@mccushjack 🙏

@cwiebe18
Copy link

@junlincc we are sending cheesecake to @mccushjack to ask him, pretty pls :-)

rebase for Teradata fixes for merge for 15930
@cwiebe18
Copy link

cwiebe18 commented Nov 1, 2021

@mccushjack thank you so much!

@junlincc @villebro anything you could do to nudge this along would be so appreciated!

rebase for issue 15930 and update to new Teradata driver
@mccushjack
Copy link
Contributor Author

@junlincc @villebro and @cwiebe18 -- The rebase is complete. The only major change outside of teradata.py fixes was an update to the main setup.py for loading Teradata's native python driver which removes the dependency on odbc driver. This should make interoperability easier and more seamless. LMK if you have any questions or comments.

@michael-s-molina
Copy link
Member

@mccushjack I think something went wrong with the rebase. It's showing 1,099 file changes.

@villebro
Copy link
Member

@mccushjack did you notice the failed unit test? I believe this is good to go once it's fixed:
image

@nytai
Copy link
Member

nytai commented Jan 24, 2022

@mccushjack seems like linting is failing for this PR. Sorry it has taken so long to get this change merged/reviewed. If you wouldn't mind fixing the lint errors and getting CI passing again, I'll make sure this gets merged. If you're part of the superset slack, feel free to ping me to get things expedited.

@mccushjack
Copy link
Contributor Author

@nytai I got some help to fix our unit test. I think everything is working now let @mccushjack and @dmcnulla know if there are any updates needed.

@dmcnulla
Copy link
Contributor

I do not know what this Python Misc/pre-commit check does, and I cannot tell from the error what is wrong. Can I buy a clue from somebody?

@nytai
Copy link
Member

nytai commented Jan 31, 2022

It runs a few linting type checks. @dmcnulla to get this check passing, the following steps should work.

  • make an edits to superset/tests/unit_tests/db_engine_specs/test_teradata.py and superset/superset/db_engine_specs/teradata.py (can be simple blank space change)
  • stage those files git add .
  • run pre-commit (should be installed via requirements/integration.txt)

@nytai
Copy link
Member

nytai commented Feb 1, 2022

Closing, as this work was absorbed by #18240

@nytai nytai closed this Feb 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants