From 937086feb057a3d5e1f9834baf8decac6ca67b72 Mon Sep 17 00:00:00 2001 From: Jerry Shao Date: Wed, 22 May 2024 12:01:42 +0800 Subject: [PATCH] [#1761] fix(mysql)clarify `timestamp` and `datetime` type convert (#3493) ### What changes were proposed in this pull request? - map `datetime` of MySQL to `timestamp` of Gravitino - map `timestamp` of MySQL to `timestamp_tz` type of Gravitino. ### Why are the changes needed? unify the type semantics Fix: #1761 ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? tests added Co-authored-by: mchades --- .../mysql/converter/MysqlTypeConverter.java | 13 ++- .../converter/TestMysqlTypeConverter.java | 7 +- .../integration/test/CatalogMysqlIT.java | 81 +++++++++++++++++++ .../test/MysqlTableOperationsIT.java | 1 - 4 files changed, 97 insertions(+), 5 deletions(-) diff --git a/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/converter/MysqlTypeConverter.java b/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/converter/MysqlTypeConverter.java index c05f89f94ca..2d88d7b530c 100644 --- a/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/converter/MysqlTypeConverter.java +++ b/catalogs/catalog-jdbc-mysql/src/main/java/com/datastrato/gravitino/catalog/mysql/converter/MysqlTypeConverter.java @@ -41,7 +41,13 @@ public Type toGravitinoType(JdbcTypeBean typeBean) { return Types.DateType.get(); case TIME: return Types.TimeType.get(); + // MySQL converts TIMESTAMP values from the current time zone to UTC for storage, and back + // from UTC to the current time zone for retrieval. (This does not occur for other types + // such as DATETIME.) see more details: + // https://dev.mysql.com/doc/refman/8.0/en/datetime.html case TIMESTAMP: + return Types.TimestampType.withTimeZone(); + case DATETIME: return Types.TimestampType.withoutTimeZone(); case DECIMAL: return Types.DecimalType.of( @@ -79,8 +85,11 @@ public String fromGravitinoType(Type type) { return type.simpleString(); } else if (type instanceof Types.TimeType) { return type.simpleString(); - } else if (type instanceof Types.TimestampType && !((Types.TimestampType) type).hasTimeZone()) { - return type.simpleString(); + } else if (type instanceof Types.TimestampType) { + // MySQL converts TIMESTAMP values from the current time zone to UTC for storage, and back + // from UTC to the current time zone for retrieval. (This does not occur for other types + // such as DATETIME.) see more details: https://dev.mysql.com/doc/refman/8.0/en/datetime.html + return ((Types.TimestampType) type).hasTimeZone() ? TIMESTAMP : DATETIME; } else if (type instanceof Types.DecimalType) { return type.simpleString(); } else if (type instanceof Types.VarCharType) { diff --git a/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/converter/TestMysqlTypeConverter.java b/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/converter/TestMysqlTypeConverter.java index 9342f433392..282746ac0e7 100644 --- a/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/converter/TestMysqlTypeConverter.java +++ b/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/converter/TestMysqlTypeConverter.java @@ -12,6 +12,7 @@ import static com.datastrato.gravitino.catalog.mysql.converter.MysqlTypeConverter.BIGINT; import static com.datastrato.gravitino.catalog.mysql.converter.MysqlTypeConverter.BINARY; import static com.datastrato.gravitino.catalog.mysql.converter.MysqlTypeConverter.CHAR; +import static com.datastrato.gravitino.catalog.mysql.converter.MysqlTypeConverter.DATETIME; import static com.datastrato.gravitino.catalog.mysql.converter.MysqlTypeConverter.DECIMAL; import static com.datastrato.gravitino.catalog.mysql.converter.MysqlTypeConverter.DOUBLE; import static com.datastrato.gravitino.catalog.mysql.converter.MysqlTypeConverter.FLOAT; @@ -39,7 +40,8 @@ public void testToGravitinoType() { checkJdbcTypeToGravitinoType(Types.DoubleType.get(), DOUBLE, null, null); checkJdbcTypeToGravitinoType(Types.DateType.get(), DATE, null, null); checkJdbcTypeToGravitinoType(Types.TimeType.get(), TIME, null, null); - checkJdbcTypeToGravitinoType(Types.TimestampType.withoutTimeZone(), TIMESTAMP, null, null); + checkJdbcTypeToGravitinoType(Types.TimestampType.withoutTimeZone(), DATETIME, null, null); + checkJdbcTypeToGravitinoType(Types.TimestampType.withTimeZone(), TIMESTAMP, null, null); checkJdbcTypeToGravitinoType(Types.DecimalType.of(10, 2), DECIMAL, "10", "2"); checkJdbcTypeToGravitinoType(Types.VarCharType.of(20), VARCHAR, "20", null); checkJdbcTypeToGravitinoType(Types.FixedCharType.of(20), CHAR, "20", null); @@ -58,7 +60,8 @@ public void testFromGravitinoType() { checkGravitinoTypeToJdbcType(DOUBLE, Types.DoubleType.get()); checkGravitinoTypeToJdbcType(DATE, Types.DateType.get()); checkGravitinoTypeToJdbcType(TIME, Types.TimeType.get()); - checkGravitinoTypeToJdbcType(TIMESTAMP, Types.TimestampType.withoutTimeZone()); + checkGravitinoTypeToJdbcType(TIMESTAMP, Types.TimestampType.withTimeZone()); + checkGravitinoTypeToJdbcType(DATETIME, Types.TimestampType.withoutTimeZone()); checkGravitinoTypeToJdbcType(DECIMAL + "(10,2)", Types.DecimalType.of(10, 2)); checkGravitinoTypeToJdbcType(VARCHAR + "(20)", Types.VarCharType.of(20)); checkGravitinoTypeToJdbcType(CHAR + "(20)", Types.FixedCharType.of(20)); diff --git a/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/CatalogMysqlIT.java b/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/CatalogMysqlIT.java index 3e6ef5b8038..a7b4b057947 100644 --- a/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/CatalogMysqlIT.java +++ b/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/CatalogMysqlIT.java @@ -588,6 +588,87 @@ void testColumnDefaultValueConverter() { } } + @Test + void testColumnTypeConverter() { + // test convert from MySQL to Gravitino + String tableName = GravitinoITUtils.genRandomName("test_type_converter"); + String fullTableName = schemaName + "." + tableName; + String sql = + "CREATE TABLE " + + fullTableName + + " (\n" + + " tinyint_col tinyint,\n" + + " smallint_col smallint,\n" + + " int_col int,\n" + + " bigint_col bigint,\n" + + " float_col float,\n" + + " double_col double,\n" + + " date_col date,\n" + + " time_col time,\n" + + " timestamp_col timestamp,\n" + + " datetime_col datetime,\n" + + " decimal_6_2_col decimal(6, 2),\n" + + " varchar20_col varchar(20),\n" + + " text_col text,\n" + + " binary_col binary\n" + + ");\n"; + + mysqlService.executeQuery(sql); + Table loadedTable = + catalog + .asTableCatalog() + .loadTable(NameIdentifier.of(metalakeName, catalogName, schemaName, tableName)); + + for (Column column : loadedTable.columns()) { + switch (column.name()) { + case "tinyint_col": + Assertions.assertEquals(Types.ByteType.get(), column.dataType()); + break; + case "smallint_col": + Assertions.assertEquals(Types.ShortType.get(), column.dataType()); + break; + case "int_col": + Assertions.assertEquals(Types.IntegerType.get(), column.dataType()); + break; + case "bigint_col": + Assertions.assertEquals(Types.LongType.get(), column.dataType()); + break; + case "float_col": + Assertions.assertEquals(Types.FloatType.get(), column.dataType()); + break; + case "double_col": + Assertions.assertEquals(Types.DoubleType.get(), column.dataType()); + break; + case "date_col": + Assertions.assertEquals(Types.DateType.get(), column.dataType()); + break; + case "time_col": + Assertions.assertEquals(Types.TimeType.get(), column.dataType()); + break; + case "timestamp_col": + Assertions.assertEquals(Types.TimestampType.withTimeZone(), column.dataType()); + break; + case "datetime_col": + Assertions.assertEquals(Types.TimestampType.withoutTimeZone(), column.dataType()); + break; + case "decimal_6_2_col": + Assertions.assertEquals(Types.DecimalType.of(6, 2), column.dataType()); + break; + case "varchar20_col": + Assertions.assertEquals(Types.VarCharType.of(20), column.dataType()); + break; + case "text_col": + Assertions.assertEquals(Types.StringType.get(), column.dataType()); + break; + case "binary_col": + Assertions.assertEquals(Types.BinaryType.get(), column.dataType()); + break; + default: + Assertions.fail("Unexpected column name: " + column.name()); + } + } + } + @Test void testAlterAndDropMysqlTable() { Column[] columns = createColumns(); diff --git a/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/MysqlTableOperationsIT.java b/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/MysqlTableOperationsIT.java index fd4cf52fa31..bd0e4b04fdb 100644 --- a/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/MysqlTableOperationsIT.java +++ b/catalogs/catalog-jdbc-mysql/src/test/java/com/datastrato/gravitino/catalog/mysql/integration/test/MysqlTableOperationsIT.java @@ -683,7 +683,6 @@ public void testCreateNotSupportTypeTable() { Types.FixedType.of(10), Types.IntervalDayType.get(), Types.IntervalYearType.get(), - Types.TimestampType.withTimeZone(), Types.UUIDType.get(), Types.ListType.of(Types.DateType.get(), true), Types.MapType.of(Types.StringType.get(), Types.IntegerType.get(), true),