Addressed pr review comments

Author: sc250072

core/trino-server/src/main/provisio/trino.xml

@@ -284,6 +284,12 @@
         </artifact>
     </artifactSet>
 
+    <artifactSet to="plugin/teradata">
+        <artifact id="${project.groupId}:trino-teradata:zip:${project.version}">
+            <unpack />
+        </artifact>
+    </artifactSet>
+
     <artifactSet to="plugin/teradata-functions">
         <artifact id="${project.groupId}:trino-teradata-functions:zip:${project.version}">
             <unpack />

docs/src/main/sphinx/connector/teradata.md

@@ -19,7 +19,7 @@ To connect to Teradata, you need:
 ## Configuration
 
 To configure the Teradata connector, create a catalog properties file in
-`etc/catalog` named, for example, `teradata.properties`, to mount the Teradata
+`etc/catalog` named, for example, `example.properties`, to mount the Teradata
 connector as the `teradata` catalog. Create the file with the following 
 contents, replacing the connection properties as appropriate for your setup:
 
@@ -51,7 +51,7 @@ your data source, you can enable TLS between your cluster and the data
 source by appending parameters to the JDBC connection string set in the 
 connection-url catalog configuration property.
 
-For example, to specify SSLMODE:
+For example, to specify `SSLMODE`:
 
 ```properties
 connection-url=jdbc:teradata://example.teradata.com/SSLMODE=REQUIRED
@@ -136,6 +136,43 @@ this table:
 
 No other types are supported.
 
+### Trino type to Teradata type mapping
+
+The connector maps Trino types to the corresponding Teradata types following
+this table:
+
+:::{list-table} Trino type to Teradata type mapping
+:widths: 40, 40, 20
+:header-rows: 1
+
+* - Trino type
+  - Teradata type
+  - Notes
+* - `TINYINT`
+  - `SMALLINT`
+  -
+* - `SMALLINT`
+  - `SMALLINT`
+  -
+* - `INTEGER`
+  - `INTEGER`
+  -
+* - `BIGINT`
+  - `BIGINT`
+  -
+* - `REAL`
+  - `FLOAT`
+  -
+* - `DOUBLE`
+  - `DOUBLE`
+  - 
+* - `DATE`
+  - `DATE`
+  -
+::::
+
+No other types are supported.
+
 ```{include} jdbc-type-mapping.fragment
 ```
 
@@ -144,28 +181,28 @@ No other types are supported.
 The Teradata connector provides a schema for every Teradata database. You can
 see the available Teradata databases by running SHOW SCHEMAS:
 
-```
+```sql
 SHOW SCHEMAS FROM teradata;
 ```
 
 If you have a Teradata database named sales, you can view the tables in this
 database by running SHOW TABLES:
 
-```
+```sql
 SHOW TABLES FROM teradata.sales;
 ```
 
 You can see a list of the columns in the orders table in the sales database
 using either of the following:
 
-```
+```sql
 DESCRIBE teradata.sales.orders;
 SHOW COLUMNS FROM teradata.sales.orders;
 ```
 
 Finally, you can access the orders table in the sales database:
 
-```
+```sql
 SELECT * FROM teradata.sales.orders;
 ```
 

plugin/trino-teradata/README.md

@@ -37,6 +37,6 @@ Once the environment variables are set, run the integration tests with:
 
 ⚠️ **Note:** Run the following command from the Trino parent directory.
 
-  ```
+```
  ./mvnw clean install -pl :trino-teradata
 ```

plugin/trino-teradata/src/main/java/io/trino/plugin/teradata/TeradataClient.java

@@ -113,40 +113,6 @@ public TeradataClient(
         this.teradataJDBCCaseSensitivity = teradataConfig.getTeradataCaseSensitivity();
     }
 
-    private static ColumnMapping charColumnMapping(int charLength, boolean isCaseSensitive)
-    {
-        if (charLength > CharType.MAX_LENGTH) {
-            return varcharColumnMapping(charLength, isCaseSensitive);
-        }
-        CharType charType = createCharType(charLength);
-        return ColumnMapping.sliceMapping(
-                charType,
-                charReadFunction(charType),
-                charWriteFunction(),
-                isCaseSensitive ? TERADATA_STRING_PUSHDOWN : CASE_INSENSITIVE_CHARACTER_PUSHDOWN);
-    }
-
-    private static ColumnMapping varcharColumnMapping(int varcharLength, boolean isCaseSensitive)
-    {
-        VarcharType varcharType = varcharLength <= VarcharType.MAX_LENGTH
-                ? createVarcharType(varcharLength)
-                : createUnboundedVarcharType();
-        return ColumnMapping.sliceMapping(
-                varcharType,
-                varcharReadFunction(varcharType),
-                varcharWriteFunction(),
-                isCaseSensitive ? TERADATA_STRING_PUSHDOWN : CASE_INSENSITIVE_CHARACTER_PUSHDOWN);
-    }
-
-    private boolean deriveCaseSensitivity(CaseSensitivity caseSensitivity)
-    {
-        return switch (teradataJDBCCaseSensitivity) {
-            case CASE_INSENSITIVE -> false;
-            case CASE_SENSITIVE -> true;
-            default -> caseSensitivity != null;
-        };
-    }
-
     @Override
     protected void createSchema(ConnectorSession session, Connection connection, String remoteSchemaName)
     {
@@ -340,7 +306,7 @@ public Optional<ColumnMapping> toColumnMapping(ConnectorSession session, Connect
         return Optional.empty();
     }
 
-    private Optional<ColumnMapping> numberMapping(JdbcTypeHandle typeHandle)
+    private static Optional<ColumnMapping> numberMapping(JdbcTypeHandle typeHandle)
     {
         int precision = typeHandle.requiredColumnSize();
         int scale = typeHandle.requiredDecimalDigits();
@@ -351,6 +317,40 @@ private Optional<ColumnMapping> numberMapping(JdbcTypeHandle typeHandle)
         return Optional.of(decimalColumnMapping(createDecimalType(precision, scale)));
     }
 
+    private static ColumnMapping charColumnMapping(int charLength, boolean isCaseSensitive)
+    {
+        if (charLength > CharType.MAX_LENGTH) {
+            return varcharColumnMapping(charLength, isCaseSensitive);
+        }
+        CharType charType = createCharType(charLength);
+        return ColumnMapping.sliceMapping(
+                charType,
+                charReadFunction(charType),
+                charWriteFunction(),
+                isCaseSensitive ? TERADATA_STRING_PUSHDOWN : CASE_INSENSITIVE_CHARACTER_PUSHDOWN);
+    }
+
+    private static ColumnMapping varcharColumnMapping(int varcharLength, boolean isCaseSensitive)
+    {
+        VarcharType varcharType = varcharLength <= VarcharType.MAX_LENGTH
+                ? createVarcharType(varcharLength)
+                : createUnboundedVarcharType();
+        return ColumnMapping.sliceMapping(
+                varcharType,
+                varcharReadFunction(varcharType),
+                varcharWriteFunction(),
+                isCaseSensitive ? TERADATA_STRING_PUSHDOWN : CASE_INSENSITIVE_CHARACTER_PUSHDOWN);
+    }
+
+    private boolean deriveCaseSensitivity(CaseSensitivity caseSensitivity)
+    {
+        return switch (teradataJDBCCaseSensitivity) {
+            case CASE_INSENSITIVE -> false;
+            case CASE_SENSITIVE -> true;
+            default -> caseSensitivity != null;
+        };
+    }
+
     @Override
     public WriteMapping toWriteMapping(ConnectorSession session, Type type)
     {

plugin/trino-teradata/src/test/java/io/trino/plugin/teradata/TestTeradataPlugin.java

@@ -16,7 +16,6 @@
 import io.trino.plugin.jdbc.JdbcConnectorFactory;
 import io.trino.spi.connector.ConnectorFactory;
 import io.trino.testing.TestingConnectorContext;
-import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
 
 import java.util.Map;
@@ -31,7 +30,6 @@ public void testCreateConnector()
     {
         TeradataPlugin plugin = new TeradataPlugin();
         ConnectorFactory factory = getOnlyElement(plugin.getConnectorFactories());
-        Assertions.assertNotNull(factory);
         assertThat(factory).isInstanceOf(JdbcConnectorFactory.class);
         factory.create("test",
                         Map.of(

plugin/trino-teradata/src/test/java/io/trino/plugin/teradata/integration/TeradataQueryRunner.java

@@ -25,7 +25,6 @@
 import io.trino.testing.DistributedQueryRunner;
 import io.trino.testing.QueryRunner;
 import io.trino.tpch.TpchTable;
-import org.assertj.core.api.ObjectAssert;
 import org.intellij.lang.annotations.Language;
 
 import java.util.List;
@@ -45,20 +44,6 @@ public static Builder builder(TestingTeradataServer server)
         return new Builder(server);
     }
 
-    public static void main(String[] args)
-            throws Exception
-    {
-        Logging logger = Logging.initialize();
-        logger.setLevel("io.trino.plugin.teradata", Level.DEBUG);
-        logger.setLevel("io.trino", Level.INFO);
-        TestingTeradataServer server = new TestingTeradataServer("TeradataQueryRunner", false);
-        QueryRunner queryRunner = builder(server).addCoordinatorProperty("http-server.http.port", "8080").setInitialTables(TpchTable.getTables()).build();
-
-        Logger log = Logger.get(TeradataQueryRunner.class);
-        log.info("======== SERVER STARTED ========");
-        log.info("\n====\n%s\n====", queryRunner.getCoordinator().getBaseUrl());
-    }
-
     public static class Builder
             extends DistributedQueryRunner.Builder<Builder>
     {
@@ -75,8 +60,8 @@ public void copyTable(QueryRunner queryRunner, QualifiedObjectName table, Sessio
         {
             @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[] {
-                    table.objectName()})).isEqualTo(queryRunner.execute(session, "SELECT count(*) FROM " + table).getOnlyValue());
+            assertThat(queryRunner.execute(session, "SELECT count(*) FROM " + table.objectName()).getOnlyValue()).as("Table is not loaded properly: %s", new Object[] {
+                    table.objectName()}).isEqualTo(queryRunner.execute(session, "SELECT count(*) FROM " + table).getOnlyValue());
         }
 
         public void copyTpchTables(QueryRunner queryRunner, String sourceCatalog, String sourceSchema, Session session, Iterable<TpchTable<?>> tables)
@@ -121,5 +106,19 @@ public DistributedQueryRunner build()
             });
             return super.build();
         }
+
+        public static void main(String[] args)
+                throws Exception
+        {
+            Logging logger = Logging.initialize();
+            logger.setLevel("io.trino.plugin.teradata", Level.DEBUG);
+            logger.setLevel("io.trino", Level.INFO);
+            TestingTeradataServer server = new TestingTeradataServer("TeradataQueryRunner", false);
+            QueryRunner queryRunner = builder(server).addCoordinatorProperty("http-server.http.port", "8080").setInitialTables(TpchTable.getTables()).build();
+
+            Logger log = Logger.get(TeradataQueryRunner.class);
+            log.info("======== SERVER STARTED ========");
+            log.info("\n====\n%s\n====", queryRunner.getCoordinator().getBaseUrl());
+        }
     }
 }

plugin/trino-teradata/src/test/java/io/trino/plugin/teradata/integration/TestTeradataTypeMapping.java

@@ -33,6 +33,7 @@
 import static io.trino.spi.type.TinyintType.TINYINT;
 import static io.trino.spi.type.VarcharType.createVarcharType;
 import static io.trino.testing.datatype.SqlDataTypeTest.create;
+import static java.lang.String.format;
 import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
 
 final class TestTeradataTypeMapping
@@ -275,7 +276,7 @@ void testDate()
 
     private DataSetup teradataJDBCCreateAndInsert(String tableNamePrefix)
     {
-        String prefix = String.format("%s.%s", database.getDatabaseName(), tableNamePrefix);
+        String prefix = format("%s.%s", database.getDatabaseName(), tableNamePrefix);
         return new CreateAndInsertDataSetup(database, prefix);
     }
 }

plugin/trino-teradata/src/test/java/io/trino/plugin/teradata/integration/clearscape/BaseException.java

@@ -16,16 +16,11 @@
 public class BaseException
         extends RuntimeException
 {
-    private final int statusCode;
+    protected final int statusCode;
 
     public BaseException(int statusCode, String body)
     {
         super(body);
         this.statusCode = statusCode;
     }
-
-    public int getStatusCode()
-    {
-        return statusCode;
-    }
 }

plugin/trino-teradata/src/test/java/io/trino/plugin/teradata/integration/clearscape/CreateEnvironmentRequest.java

@@ -13,8 +13,17 @@
  */
 package io.trino.plugin.teradata.integration.clearscape;
 
+import static java.util.Objects.requireNonNull;
+
 public record CreateEnvironmentRequest(
         String name,
         String region,
-        String password
-) {}
+        String password)
+{
+    public CreateEnvironmentRequest
+    {
+        requireNonNull(name, "name should not be null");
+        requireNonNull(region, "region should not be null");
+        requireNonNull(password, "password should not be null");
+    }
+}

plugin/trino-teradata/src/test/java/io/trino/plugin/teradata/integration/clearscape/DeleteEnvironmentRequest.java

@@ -13,6 +13,13 @@
  */
 package io.trino.plugin.teradata.integration.clearscape;
 
+import static java.util.Objects.requireNonNull;
+
 public record DeleteEnvironmentRequest(
-        String name
-) {}
+        String name)
+{
+    public DeleteEnvironmentRequest
+    {
+        requireNonNull(name, "name should not be null");
+    }
+}