Skip to content

Commit

Permalink
Map SQL Server tinyint to Trino SMALLINT
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
tangjiangling authored and hashhar committed Mar 3, 2022
1 parent b67d9bb commit 4b20ba1
Show file tree
Hide file tree
Showing 6 changed files with 41 additions and 15 deletions.
1 change: 1 addition & 0 deletions docs/src/main/sphinx/connector/sqlserver.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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``
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -285,7 +284,10 @@ public Optional<ColumnMapping> 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());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -133,7 +132,7 @@ public void testAllDatatypes()
BIGINT,
SMALLINT,
INTEGER,
TINYINT,
SMALLINT,
DOUBLE,
REAL,
CHAR,
Expand All @@ -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 ",
Expand All @@ -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",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 | | |

0 comments on commit 4b20ba1

Please sign in to comment.