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

Added initial Teradata JDBC driver implementation #5537

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ofir-insait
Copy link

This patch introduces initial Teradata JDBC driver implementation.

Introducing the following env vars:
CUBEJS_DB_TERADATA_NAME - optional, name of the database to connect to CUBEJS_DB_TERADATA_URL - mandatory, e.g.
jdbc:teradata://<teradata_db_ip>/USER=<teradata_user>,PASSWORD=<teradata_user_pwd>
CUBEJS_DB_TERADATA_ACCEPT_POLICY - optional, true/false to accept terms and conditions

As part of the Docker build we fetch an external dependency, namely the Teradata JDBC jar file into:
/cubejs/packages/cubejs-teradata-jdbc-driver/download/terajdbc4.jar

More information on setting up a server and using this driver is documented here:
packages/cubejs-teradata-jdbc-driver/README.md
We tested it with an on-prem Teradata server as well as Teradata Express.
Known limitations include:

  1. Max 15 transactions per cursor
    https://docs.teradata.com/r/GVKfXcemJFkTJh_89R34UQ/YB_t1rB9boiHUNbSQlvLRA (Error 3130).

  2. Cursor limitations
    After 15 transactions we get the 3130 error from the cursor.
    Temporary workaround: doing a Cube pre-processing step where we manually
    generate the schemas outside of Cube using Teradata SQL CLI client (bteq).

  3. Only handling table views
    Handling tables (in addition to table views)
    Currently we only support table views (tableKind=‘V’) and not tables.
    Need to implement and test it.

Check List

  • Tests has been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Issue Reference this PR resolves

[For example #12]

Description of Changes Made (if issue reference is not provided)

[Description goes here]

Introducing the following env vars:
CUBEJS_DB_TERADATA_NAME - optional, name of the database to connect to
CUBEJS_DB_TERADATA_URL - mandatory, e.g.
                         jdbc:teradata://<teradata_db_ip>/USER=<teradata_user>,PASSWORD=<teradata_user_pwd>
CUBEJS_DB_TERADATA_ACCEPT_POLICY - optional, true/false to accept terms and conditions

As part of the Docker build we fetch an external dependency,
namely the Teradata JDBC jar file into:
/cubejs/packages/cubejs-teradata-jdbc-driver/download/terajdbc4.jar

More information on setting up a server and using this driver is documented here:
packages/cubejs-teradata-jdbc-driver/README.md
We tested it with an on-prem Teradata server as well as Teradata Express.
Known limitations:
1) Max 15 transactions per cursor
   https://docs.teradata.com/r/GVKfXcemJFkTJh_89R34UQ/YB_t1rB9boiHUNbSQlvLRA (Error 3130).

2) Cursor limitations
   After 15 transactions we get the 3130 error from the cursor.
   Temporary workaround: doing a Cube pre-processing step where we manually
   generate the schemas outside of Cube using Teradata SQL CLI client (bteq).

3) Only handling table views
   Handling tables (in addition to table views)
   Currently we only support table views (tableKind=‘V’) and not tables.
   Need to implement and test it.
@ofir-insait ofir-insait requested review from a team as code owners October 28, 2022 16:46
@github-actions github-actions bot added the pr:community Contribution from Cube.js community members. label Oct 28, 2022
@ofir-insait
Copy link
Author

In order to test this patch with Cube you can setup a Teradata Express (Vantage) server on Azure VM (for example) using this guide:
https://quickstarts.teradata.com/run-vantage-express-on-microsoft-azure.html

You can also setup a local Teradata instance using VirtualBox on x86:
https://quickstarts.teradata.com/getting.started.vbox.html

@paco-valdez
Copy link
Contributor

@ofir-insait I'm not familiar with DB Drivers, so I can't do a full review, but my 2 cents are that drivers usually don't have their own env variable to define DB name:
Exhibit A
Exhibit B
Exhibit C


public async getTablesQuery(databaseName: string) {
try {
// Only SELECT VIEWS (tabkeKind=V)
Copy link
Contributor

Choose a reason for hiding this comment

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

@ofir-insait Also, can you provide some insight as to why only Views are supported?

Copy link
Author

Choose a reason for hiding this comment

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

The main reason is time, this is what our clients used and what we needed to support.
We have actually started with schema discovery for tables as well and the delta is not huge to add this support.
Teradata obviously has metadata for tables as well.

Note that this is also stated in the commit message:

Only handling table views
Handling tables (in addition to table views)
Currently we only support table views (tableKind=‘V’) and not tables.
Need to implement and test it.

@ofir-insait
Copy link
Author

@ofir-insait I'm not familiar with DB Drivers, so I can't do a full review, but my 2 cents are that drivers usually don't have their own env variable to define DB name: Exhibit A Exhibit B Exhibit C

Hey @pacofvf this is partially true, if you look at DataBricks for example, you would find several db specific env vars.
As was advised on Slack, we used DataBricks as a baseline for contributing this patch.

What we could do, though, is:

  1. Use CUBEJS_DB_NAME instead of CUBEJS_DB_TERADATA_NAME
  2. Remove CUBEJS_DB_TERADATA_ACCEPT_POLICY and set it to hard coded true

But we would still need CUBEJS_DB_TERADATA_URL IMHO.

@paveltiunov
Copy link
Member

@ofir-insait Thanks a lot for contributing! An E2E test for Teradata would be very much appreciated. You can use this one as an example: https://github.com/cube-js/cube.js/blob/master/packages/cubejs-testing/test/smoke-materialize.test.ts.

@paveltiunov paveltiunov self-assigned this Nov 29, 2022
@paveltiunov
Copy link
Member

@ofir-insait While you're working on a test, you might want to consider publishing your driver version as is and adding a backlink in our docs. Please see https://github.com/cube-js/cube.js/blob/master/CONTRIBUTING.md#contributing-database-drivers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:community Contribution from Cube.js community members.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants