-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add Teradata connector #26731
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
base: master
Are you sure you want to change the base?
Add Teradata connector #26731
Conversation
|
|
||
| import java.util.List; | ||
|
|
||
| public record EnvironmentResponse( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add a compact constructor and add rnn check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as others
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
| boolean destroyEnv, | ||
| String region) | ||
| { | ||
| this.token = token; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rnn check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requirednotnull check added
| public static final String BEARER = "Bearer "; | ||
|
|
||
| private Headers() | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
private Headers()
{}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
|
|
||
| @AfterAll | ||
| public void cleanupTestDatabase() | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could use closeAfterClass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated with closeAfterClass
| @Test | ||
| public void testAddColumn() | ||
| { | ||
| Assumptions.abort("Skipping as connector does not support column level write operations"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same other places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed all applicable places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @chenjian2664 for your review. addressed all your review comments.
|
|
||
| <dependency> | ||
| <groupId>com.teradata.jdbc</groupId> | ||
| <artifactId>terajdbc</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maven central lists this with "Teradata Generic Download License" license.
Is that license compatible for inclusion in Apache project?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @findepi, Thanks for pointing this out. The Teradata JDBC driver is distributed under the "Teradata Generic Download License" and is free to download and use.
We are currently using the same driver in the Airbyte project for the Teradata connector without issues.
That said, we are checking internally with our legal team to confirm whether this license is compatible for inclusion in Apache projects like Trino. We’ll get back to you as soon as we have a confirmation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@findepi
Our Legal team has confirmed that While Teradata's JDBC driver is not under an open source license like Apache license, we believe this license via scenario ii (section 2(a)(ii)) would make it a free license that is similar to the Oracle Free License.
If there is a specific concern about our JDBC driver, please share the specific concern so we can analyse and review and respond accordingly.
How will these tests be run on Trino CI? |
Thank you @findepi for your review. We need Trino team help to configure required environment variables to run Teradata connector integration tests and need to update Trino CI to include Teradata integration tests |
| Properties connectionProperties = new Properties(); | ||
| Driver driver = DriverManager.getDriver(config.getConnectionUrl()); | ||
| String longMech = LogonMechanism.fromString(teradataConfig.getLogMech()).getMechanism(); | ||
| connectionProperties.put("LOGMECH", longMech); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
longMech -> logMech ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified to logonMechanism
| // Initialize ClearScape Instance and Get the host name from ClearScape API in case config is using clearscape | ||
| if (config.isUseClearScape()) { | ||
| boolean destroyEnv = false; | ||
| if (System.getenv("CLEARSCAPE_DESTORY_ENV") != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use io.trino.testing.SystemEnvironmentUtils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with io.trino.testing.SystemEnvironmentUtils. Thank you for suggestion
ebyhr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left initial comments.
| import static java.util.Objects.requireNonNull; | ||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| final class TeradataConnectorTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to TestTeradataConnectorTest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| ## Configuration | ||
|
|
||
| To configure the Teradata connector, create a catalog properties file in | ||
| `etc/catalog` named, for example, `teradata.properties`, to mount the Teradata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use example.properties to differentiate it from the connector name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
modified to example.properties
| source by appending parameters to the JDBC connection string set in the | ||
| connection-url catalog configuration property. | ||
|
|
||
| For example, to specify SSLMODE: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| For example, to specify SSLMODE: | |
| For example, to specify `SSLMODE`: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
| The Teradata connector provides a schema for every Teradata database. You can | ||
| see the available Teradata databases by running SHOW SCHEMAS: | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for other SQL examples:
| ``` | |
| ```sql |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
|
|
||
| ## SQL support | ||
|
|
||
| The connector provides read access access to data and metadata in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| The connector provides read access access to data and metadata in | |
| The connector provides read access to data and metadata in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| public class TestTeradataConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refer to other config test classes, e.g. TestMongoClientConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TeradataConfig was removed based on the latest review comments, so the corresponding TestTeradataConfig has also been deleted.
plugin/trino-teradata/README.md
Outdated
|
|
||
| ⚠️ **Note:** Run the following command from the Trino parent directory. | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove a leading space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
| @@ -0,0 +1,282 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you update core/trino-server/src/main/provisio/trino.xml?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it. Whats the role of this entry?
| { | ||
| @Language("SQL") String sql = String.format("CREATE TABLE %s AS SELECT * FROM %s", table.objectName(), table); | ||
| queryRunner.execute(session, sql); | ||
| ((ObjectAssert) assertThat(queryRunner.execute(session, "SELECT count(*) FROM " + table.objectName()).getOnlyValue()).as("Table is not loaded properly: %s", new Object[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The outer ObjectAssert can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
| return new Builder(server); | ||
| } | ||
|
|
||
| public static void main(String[] args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually put query runner's main method to the bottom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Followed same and updated accordingly
@electrum Could you please squash the changes so I can address the new review comments? Also, could you configure the ClearScape secrets on this branch so we can run the Teradata integration tests? |
d9373c8 to
dce6d35
Compare
|
/test-with-secrets sha=946d9b03311a93b377f778d24df6e6d6abbad29d |
|
The CI workflow run with tests that require additional secrets finished as failure: https://github.com/trinodb/trino/actions/runs/19518520557 |
@ebyhr addressed all your review comments. Could you please re-review the code changes |
a25b824 to
e79b323
Compare
| @@ -0,0 +1,214 @@ | |||
| # Teradata connector | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mosabua Do you want to review this doc?
| public class TeradataClient | ||
| extends BaseJdbcClient | ||
| { | ||
| private static final PredicatePushdownController TERADATA_STRING_PUSHDOWN = FULL_PUSHDOWN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove a redundant constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed edundant constant.
| RemoteQueryModifier remoteQueryModifier) | ||
| { | ||
| super("\"", connectionFactory, queryBuilder, config.getJdbcTypesMappedToVarchar(), identifierMapping, remoteQueryModifier, true); | ||
| this.teradataJDBCCaseSensitivity = teradataConfig.getTeradataCaseSensitivity(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why we need to control case sensitivity with a config property. Why can't we rely on column metadata from Teradata instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. You’re right — Teradata metadata is consistent, so we don’t need a separate config property to control case sensitivity. I’ve removed the config‑based control and now rely directly on the metadata provided by the Teradata instance.
| protected void createSchema(ConnectorSession session, Connection connection, String remoteSchemaName) | ||
| { | ||
| execute(session, format( | ||
| "CREATE DATABASE %s AS PERMANENT = 60000000, SPOOL = 120000000", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the rationale of these numbers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Teradata requires the permanent storage value to be explicitly specified in bytes, as no default is provided. We are using 60,000,000 bytes, consistent with the storage value defined in other Teradata connectors used by the tools
| // try to use result set metadata from select * from table to populate the mapping | ||
| try { | ||
| HashMap<String, CaseSensitivity> caseMap = new HashMap<>(); | ||
| String sql = format("select * from %s.%s where 0=1", schemaTableName.getSchemaName(), schemaTableName.getTableName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to add quoted to the schema and table names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Uppercase select, from, where.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing out this. Updated with quoted and Uppercase for select, from, where.
String sql = format("SELECT * FROM %s.%s WHERE 0=1", quoted(schemaTableName.getSchemaName()), quoted(schemaTableName.getTableName()));| PreparedStatement pstmt = connection.prepareStatement(sql); | ||
| ResultSetMetaData rsmd = pstmt.getMetaData(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated abbreviations with meaningful variable names
| @Override | ||
| public Optional<ColumnMapping> toColumnMapping(ConnectorSession session, Connection connection, JdbcTypeHandle typeHandle) | ||
| { | ||
| // this method should ultimately encompass all the expected teradata data types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove an helpful comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed
| private static ColumnMapping charColumnMapping(int charLength, boolean isCaseSensitive) | ||
| { | ||
| if (charLength > CharType.MAX_LENGTH) { | ||
| return varcharColumnMapping(charLength, isCaseSensitive); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fallback to varchar type is weird. Could you leave a code comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve removed the if loop since it’s not required in this case. Teradata’s maximum CHAR/VARCHAR length (64,000) is always within Trino’s supported CharType.MAX_LENGTH (65,536), so the fallback branch would never be triggered.
plugin/trino-teradata/pom.xml
Outdated
| <dependency> | ||
| <groupId>io.trino</groupId> | ||
| <artifactId>trino-plugin-toolkit</artifactId> | ||
| <type>test-jar</type> | ||
| <scope>test</scope> | ||
| </dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rebase on master. The build will fail because of logical conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rebased with master.
| case Type typeInstance when typeInstance == DOUBLE -> WriteMapping.doubleMapping("double precision", doubleWriteFunction()); | ||
| case Type typeInstance when typeInstance == DATE -> WriteMapping.longMapping("date", dateWriteFunctionUsingLocalDate()); | ||
| case DecimalType decimalTypeInstance -> { | ||
| String dataType = String.format("decimal(%s, %s)", decimalTypeInstance.getPrecision(), decimalTypeInstance.getScale()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Static import format method, or just use formatted instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used Static import format method
| import static io.airlift.configuration.testing.ConfigAssertions.assertRecordedDefaults; | ||
| import static io.airlift.configuration.testing.ConfigAssertions.recordDefaults; | ||
|
|
||
| public class TestTeradataConfig |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://trino.io/docs/current/develop/tests.html#conventions-and-recommendations
Test classes should be defined as package-private and final.
Same for other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied
| public class TestTeradataConfig | ||
| { | ||
| @Test | ||
| public void testDefaults() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://trino.io/docs/current/develop/tests.html#conventions-and-recommendations
Test methods should be defined as package-private.
Same for other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TeradataConfig class is removed but Applied suggested changes to all applicable tests
plugin/trino-teradata/pom.xml
Outdated
| <dependency> | ||
| <groupId>com.teradata.jdbc</groupId> | ||
| <artifactId>terajdbc</artifactId> | ||
| <version>20.00.00.49</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <version>20.00.00.49</version> | |
| <version>20.00.00.51</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with latest recent release of Teradata Jdbc driver version
| */ | ||
| package io.trino.plugin.teradata.integration.clearscape; | ||
|
|
||
| public class Error4xxException |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error4xxException and Error5xxException are redundant. I would merge them into BaseException, and rename the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merged the two exception classes to single ClearScapeServiceException
| Pattern.compile("^(https?://)(www\\.)?api.clearscape.teradata\\.com.*"); | ||
| private Model model; | ||
|
|
||
| private boolean isValidUrl(String url) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method can be static. Note that we put helper methods after the usage. Move this method under getTeradataHttpClient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied the suggestion across applicable places of Teradata connector
| public interface TeradataTestConstants | ||
| { | ||
| String ENV_CLEARSCAPE_URL = "https://api.clearscape.teradata.com"; | ||
| String ENV_CLEARSCAPE_USERNAME = "demo_user"; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public final class TeradataTestConstants
{
public static String CLEARSCAPE_URL = "https://api.clearscape.teradata.com";
public static String CLEARSCAPE_USERNAME = "demo_user";
private TeradataTestConstants() {}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied the suggested changes to TeradataTestConstants: converted to a final class and added the final keyword to both CLEARSCAPE_URL and CLEARSCAPE_USERNAME.
| private static void verifyResultOrFailure(AssertProvider<QueryAssertions.QueryAssert> queryAssertProvider, Consumer<QueryAssertions.QueryAssert> verifyResults, | ||
| Consumer<TrinoExceptionAssert> verifyFailure) | ||
| { | ||
| requireNonNull(verifyResults, "verifyResults is null"); | ||
| requireNonNull(verifyFailure, "verifyFailure is null"); | ||
| QueryAssertions.QueryAssert queryAssert = assertThat(queryAssertProvider); | ||
| verifyResults.accept(queryAssert); | ||
| } | ||
|
|
||
| @Override | ||
| protected SqlExecutor onRemoteDatabase() | ||
| { | ||
| return database; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move these helper methods to the bottom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved the helper methods to the bottom of the class as requested
| @Test | ||
| public void testRenameSchema() | ||
| { | ||
| abort("Skipping as connector does not support RENAME SCHEMA"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsupported operations should be handled by skipTestUnless condition in BaseConnectorTest. Why do we override these method in this class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. At the moment, a couple of tests are failing because Teradata does not yet support INSERT operations. To prevent false failures, I added explicit aborts for the following tests:
-
testInsertIntoNotNullColumn -
testInsertWithoutTemporaryTable -
testColumnName -
testUpdateNotNullColumn -
testWriteBatchSizeSessionProperty -
testWriteTaskParallelismSessionProperty
Once Teradata adds support for inserts, these overrides can be removed.
| .execute(getQueryRunner(), teradataJDBCCreateAndInsert("date")); | ||
| } | ||
|
|
||
| private DataSetup teradataJDBCCreateAndInsert(String tableNamePrefix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| private DataSetup teradataJDBCCreateAndInsert(String tableNamePrefix) | |
| private DataSetup teradataCreateAndInsert(String tableNamePrefix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modified to teradataCreateAndInsert
| <module>plugin/trino-snowflake</module> | ||
| <module>plugin/trino-spooling-filesystem</module> | ||
| <module>plugin/trino-sqlserver</module> | ||
| <module>plugin/trino-teradata</module> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update EnvMultinodeAllConnectors. Also, we need a simple product test that ensures there is no class loader issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ebyhr could you please provide a reference of other connector to write product test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we add an entry for Teradata in EnvMultinodeAllConnectors, the trino-product-tests-launcher plugin build fails with an unhandled exception reported by the Error Prone static analysis plugin at EnvMultinodeAllConnectors.java:[78,25]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we add an entry for Teradata in EnvMultinodeAllConnectors, the trino-product-tests-launcher plugin build fails with an unhandled exception reported by the Error Prone static analysis plugin at EnvMultinodeAllConnectors.java:[78,25]
Should be solvable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ebyhr added Teradata to EnvMultinodeAllConnectors and introduced a TestTeradata product test to verify class loader isolation.
trino-product-tests-launcher/src/main/resources/docker/trino-product-tests/conf/environment/multinode-teradata added below entries
connector.name=teradata
connection-url=jdbc:teradata://${ENV:TERADATA_HOSTNAME}/
connection-user=${ENV:TERADATA_USERNAME}
connection-password=${ENV:TERADATA_PASSWORD}Could you please review and let me know if any adjustments are needed?
a773d47 to
df0e587
Compare
ec895f3 to
6b0015c
Compare
…radata Added read and limited write support to Teradata Trino Connector
6b0015c to
db25a17
Compare
mosabua
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
This pull request has gone a while without any activity. Ask for help on #core-dev on Trino slack. |
Description
This PR introduces the Teradata connector, enabling Trino to query and interact with Teradata data sources.
Key additions:
Implements the Teradata connector SPI with support for:
• Connection setup
• Schema retrieval
• Data type mapping
Adds integration tests against a Teradata ClearScape Analytics™ Experience instance to verify connection, query execution, and result mapping.
Updates documentation under connectors to include usage examples and configuration options.
This connector extends Trino’s ecosystem to support Teradata users, allowing them to leverage Trino’s distributed query engine for federated queries.
Additional context and related issues
The Teradata connector module has both unit tests and integration tests.
The integration tests require access to a Teradata ClearScape Analytics™ Experience.
You can follow the steps below to run the integration tests locally.
Prerequisites
Create a new ClearScape Analytics™ Experience account
If you don't already have one, sign up at:
Teradata ClearScape Analytics™ Experience
Login
Sign in with your new account at:
ClearScape Analytics™ Experience Login
Collect the API Token
Use the Copy API Token button in the UI to retrieve your token.
Define the following environment variables
Running Integration Tests
Once the environment variables are set, run the integration tests with:
./mvnw clean install -pl :trino-teradataRelease notes
( ) This is not user-visible or is docs only, and no release notes are required.
(x ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: