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

Teradata connector #12078

Closed
wants to merge 2 commits into from
Closed

Conversation

sajjoseph
Copy link

Presto Teradata connector

Implements Teradata connector using Teradata JDBC driver. Tested against Teradata 16.20 even though it should work with older versions (16.10, 15.10, 15.0, 14.10) as well.

@lai-aa
Copy link

lai-aa commented Dec 18, 2018

Just curious, What's the difference between this connector and the official "Teradata-To-Presto QueryGrid connector", especially the performance side?

@sajjoseph
Copy link
Author

sajjoseph commented Dec 25, 2018 via email

Copy link
Contributor

@highker highker left a comment

Choose a reason for hiding this comment

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

Unit tests are needed.

@@ -0,0 +1,42 @@
# Presto Teradata Plugin

This is a plugin for Presto that allow you to use Teradata Jdbc connection to connect to Teradata database.
Copy link
Contributor

Choose a reason for hiding this comment

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

... allows you ...

<parent>
<groupId>com.facebook.presto</groupId>
<artifactId>presto-root</artifactId>
<version>0.215-SNAPSHOT</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

Update the snapshot

@@ -0,0 +1,83 @@
<?xml version="1.0"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

The indentation of this file doesn't look right; please 4 instead of 8.

extends BaseJdbcClient
{
static final String TERADATA_DRIVER_NAME = "com.teradata.jdbc.TeraDriver";
private boolean usePreparedStatement;
Copy link
Contributor

Choose a reason for hiding this comment

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

space after this statement.

public TeradataClient(JdbcConnectorId connectorId, BaseJdbcConfig config, TeradataConfig teradataConfig)
throws SQLException
{
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Use // instead of /* with indentation.

protected SchemaTableName getSchemaTableName(ResultSet resultSet)
throws SQLException
{
String tableSchema = resultSet.getString("TABLE_SCHEM");
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these TABLE_SCHEM, TABLE_NAME, etc?

static final String TERADATA_DRIVER_NAME = "com.teradata.jdbc.TeraDriver";
private boolean usePreparedStatement;
@Inject
public TeradataClient(JdbcConnectorId connectorId, BaseJdbcConfig config, TeradataConfig teradataConfig)
Copy link
Contributor

Choose a reason for hiding this comment

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

If TeradataConfig is a BaseJdbcConfig, why we need pass both of them in?

this.defaultRowPreFetch = defaultRowPreFetch;
}

private String defaultRowPreFetch = DEFAULT_ROW_PRE_FETCH;
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this together with other vars.

return defaultRowPreFetch;
}

@Config("teradata.defaultRowPrefetch")
Copy link
Contributor

Choose a reason for hiding this comment

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

we use dashes: default-row-prefetch

return usePreparedStatement;
}

@Config("teradata.use-preparedstatement")
Copy link
Contributor

Choose a reason for hiding this comment

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

prepared-statement

@stale
Copy link

stale bot commented Aug 18, 2019

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the task, make sure you've addressed reviewer comments, and rebase on the latest master. Thank you for your contributions!

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.

4 participants