From 95d2e8e0cf722e06077dc4b89ee99286123c7c03 Mon Sep 17 00:00:00 2001 From: tangjiangling Date: Wed, 2 Mar 2022 16:51:03 +0800 Subject: [PATCH] Map SQL Server tinyint to Trino SMALLINT In the original implementation, we mapped SQL Server tinyint to Trino TINYINT, but there is a difference in the range they supported: - Trino TINYINT supports a range of [-128, 127] - SQL Server tinyint supports a range of [0, 255] So we can't read or write values in the range [128, 255], so we map SQL Server tinyint to Trino SMALLINT in this commit. Although we are now extending the range of SQL Server tinyint in Trino, we don't have to worry about Trino writing values that are not in the range [0, 255] to SQL Server, because SQL Server already guarantees this for us and this behavior is verified by the `BaseSqlServerTypeMapping#testUnsupportedTinyint` test. --- docs/src/main/sphinx/connector/sqlserver.rst | 1 + .../plugin/sqlserver/SqlServerClient.java | 6 ++-- .../sqlserver/BaseSqlServerTypeMapping.java | 36 +++++++++++++++---- .../SqlServerDataTypesTableDefinition.java | 4 +-- .../tests/product/sqlserver/TestSelect.java | 7 ++-- .../connectors/sqlserver/describe_table.sql | 2 +- 6 files changed, 41 insertions(+), 15 deletions(-) diff --git a/docs/src/main/sphinx/connector/sqlserver.rst b/docs/src/main/sphinx/connector/sqlserver.rst index ddf04281ad80..68df03c084ae 100644 --- a/docs/src/main/sphinx/connector/sqlserver.rst +++ b/docs/src/main/sphinx/connector/sqlserver.rst @@ -125,6 +125,7 @@ Trino supports the following SQL Server data types: SQL Server Type Trino Type ================================== =============================== ``bigint`` ``bigint`` +``tinyint`` ``smallint`` ``smallint`` ``smallint`` ``int`` ``integer`` ``float`` ``double`` diff --git a/plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java b/plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java index 857062c4483e..026f93d5e51d 100644 --- a/plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java +++ b/plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java @@ -124,7 +124,6 @@ import static io.trino.plugin.jdbc.StandardColumnMappings.timeReadFunction; import static io.trino.plugin.jdbc.StandardColumnMappings.timestampColumnMapping; import static io.trino.plugin.jdbc.StandardColumnMappings.timestampWriteFunction; -import static io.trino.plugin.jdbc.StandardColumnMappings.tinyintColumnMapping; import static io.trino.plugin.jdbc.StandardColumnMappings.tinyintWriteFunction; import static io.trino.plugin.jdbc.StandardColumnMappings.varcharReadFunction; import static io.trino.plugin.jdbc.StandardColumnMappings.varcharWriteFunction; @@ -285,7 +284,10 @@ public Optional toColumnMapping(ConnectorSession session, Connect return Optional.of(booleanColumnMapping()); case Types.TINYINT: - return Optional.of(tinyintColumnMapping()); + // Map SQL Server TINYINT to Trino SMALLINT because SQL Server TINYINT is actually "unsigned tinyint" + // We don't check the range of values, because SQL Server will do it for us, and this behavior has already + // been tested in `BaseSqlServerTypeMapping#testUnsupportedTinyint` + return Optional.of(smallintColumnMapping()); case Types.SMALLINT: return Optional.of(smallintColumnMapping()); diff --git a/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerTypeMapping.java b/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerTypeMapping.java index fb2ac5d08396..dc1c7ab51813 100644 --- a/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerTypeMapping.java +++ b/plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerTypeMapping.java @@ -19,6 +19,7 @@ import io.trino.testing.AbstractTestQueryFramework; import io.trino.testing.TestingSession; import io.trino.testing.datatype.CreateAndInsertDataSetup; +import io.trino.testing.datatype.CreateAndTrinoInsertDataSetup; import io.trino.testing.datatype.CreateAsSelectDataSetup; import io.trino.testing.datatype.DataSetup; import io.trino.testing.datatype.SqlDataTypeTest; @@ -49,7 +50,6 @@ import static io.trino.spi.type.TimeZoneKey.UTC_KEY; import static io.trino.spi.type.TimestampType.createTimestampType; import static io.trino.spi.type.TimestampWithTimeZoneType.createTimestampWithTimeZoneType; -import static io.trino.spi.type.TinyintType.TINYINT; import static io.trino.spi.type.VarbinaryType.VARBINARY; import static io.trino.spi.type.VarcharType.createUnboundedVarcharType; import static io.trino.spi.type.VarcharType.createVarcharType; @@ -114,13 +114,27 @@ public void testSqlServerBit() @Test public void testTinyint() { - // TODO: Add min/max/min-1/max+1 tests (https://github.com/trinodb/trino/issues/11209) + // Map SQL Server TINYINT to Trino SMALLINT because SQL Server TINYINT is actually "unsigned tinyint" SqlDataTypeTest.create() - .addRoundTrip("tinyint", "NULL", TINYINT, "CAST(NULL AS TINYINT)") - .addRoundTrip("tinyint", "5", TINYINT, "TINYINT '5'") + .addRoundTrip("tinyint", "NULL", SMALLINT, "CAST(NULL AS SMALLINT)") + .addRoundTrip("tinyint", "0", SMALLINT, "SMALLINT '0'") // min value in SQL Server + .addRoundTrip("tinyint", "5", SMALLINT, "SMALLINT '5'") + .addRoundTrip("tinyint", "255", SMALLINT, "SMALLINT '255'") // max value in SQL Server .execute(getQueryRunner(), sqlServerCreateAndInsert("test_tinyint")) - .execute(getQueryRunner(), trinoCreateAsSelect("test_tinyint")) - .execute(getQueryRunner(), trinoCreateAndInsert("test_tinyint")); + .execute(getQueryRunner(), sqlServerCreateAndTrinoInsert("test_tinyint")); + } + + @Test + public void testUnsupportedTinyint() + { + try (TestTable table = new TestTable(onRemoteDatabase(), "test_unsupported_tinyint", "(data tinyint)")) { + assertSqlServerQueryFails( + format("INSERT INTO %s VALUES (-1)", table.getName()), // min - 1 + "Arithmetic overflow error for data type tinyint, value = -1"); + assertSqlServerQueryFails( + format("INSERT INTO %s VALUES (256)", table.getName()), // max + 1 + "Arithmetic overflow error for data type tinyint, value = 256."); + } } @Test @@ -846,6 +860,16 @@ protected DataSetup sqlServerCreateAndInsert(String tableNamePrefix) return new CreateAndInsertDataSetup(onRemoteDatabase(), tableNamePrefix); } + protected DataSetup sqlServerCreateAndTrinoInsert(String tableNamePrefix) + { + return sqlServerCreateAndTrinoInsert(getSession(), tableNamePrefix); + } + + protected DataSetup sqlServerCreateAndTrinoInsert(Session session, String tableNamePrefix) + { + return new CreateAndTrinoInsertDataSetup(onRemoteDatabase(), new TrinoSqlExecutor(getQueryRunner(), session), tableNamePrefix); + } + private static void checkIsDoubled(ZoneId zone, LocalDateTime dateTime) { verify(zone.getRules().getValidOffsets(dateTime).size() == 2, "Expected %s to be doubled in %s", dateTime, zone); diff --git a/testing/trino-product-tests/src/main/java/io/trino/tests/product/sqlserver/SqlServerDataTypesTableDefinition.java b/testing/trino-product-tests/src/main/java/io/trino/tests/product/sqlserver/SqlServerDataTypesTableDefinition.java index 3a22fc4e58aa..0aec9655883d 100644 --- a/testing/trino-product-tests/src/main/java/io/trino/tests/product/sqlserver/SqlServerDataTypesTableDefinition.java +++ b/testing/trino-product-tests/src/main/java/io/trino/tests/product/sqlserver/SqlServerDataTypesTableDefinition.java @@ -68,7 +68,7 @@ private SqlServerDataTypesTableDefinition() {} Long.MIN_VALUE, Short.MIN_VALUE, Integer.MIN_VALUE, - Byte.MIN_VALUE, + 0, Double.MIN_VALUE, -3.40E+38f, "\0", @@ -88,7 +88,7 @@ private SqlServerDataTypesTableDefinition() {} Long.MAX_VALUE, Short.MAX_VALUE, Integer.MAX_VALUE, - Byte.MAX_VALUE, + 255, Double.MAX_VALUE, Float.MAX_VALUE, "abcd", diff --git a/testing/trino-product-tests/src/main/java/io/trino/tests/product/sqlserver/TestSelect.java b/testing/trino-product-tests/src/main/java/io/trino/tests/product/sqlserver/TestSelect.java index bfcf2cff93d7..beccde02c2f1 100644 --- a/testing/trino-product-tests/src/main/java/io/trino/tests/product/sqlserver/TestSelect.java +++ b/testing/trino-product-tests/src/main/java/io/trino/tests/product/sqlserver/TestSelect.java @@ -48,7 +48,6 @@ import static java.sql.JDBCType.REAL; import static java.sql.JDBCType.SMALLINT; import static java.sql.JDBCType.TIMESTAMP; -import static java.sql.JDBCType.TINYINT; import static java.sql.JDBCType.VARCHAR; import static java.util.Collections.nCopies; @@ -133,7 +132,7 @@ public void testAllDatatypes() BIGINT, SMALLINT, INTEGER, - TINYINT, + SMALLINT, DOUBLE, REAL, CHAR, @@ -154,7 +153,7 @@ public void testAllDatatypes() Long.MIN_VALUE, Short.MIN_VALUE, Integer.MIN_VALUE, - Byte.MIN_VALUE, + 0, Double.MIN_VALUE, -3.40E+38f, "\0 ", @@ -174,7 +173,7 @@ public void testAllDatatypes() Long.MAX_VALUE, Short.MAX_VALUE, Integer.MAX_VALUE, - Byte.MAX_VALUE, + 255, Double.MAX_VALUE, Float.MAX_VALUE, "abcd", diff --git a/testing/trino-product-tests/src/main/resources/sql-tests/testcases/connectors/sqlserver/describe_table.sql b/testing/trino-product-tests/src/main/resources/sql-tests/testcases/connectors/sqlserver/describe_table.sql index 6390bb48484b..f1fa89ce2c6b 100644 --- a/testing/trino-product-tests/src/main/resources/sql-tests/testcases/connectors/sqlserver/describe_table.sql +++ b/testing/trino-product-tests/src/main/resources/sql-tests/testcases/connectors/sqlserver/describe_table.sql @@ -7,7 +7,7 @@ id_employee | integer | | | first_name | varchar(32) | | | last_name | varchar(32) | | | date_of_employment | date | | | -department | tinyint | | | +department | smallint | | | id_department | integer | | | name | varchar(32) | | | salary | integer | | |