mchades commented on code in PR #6657:
URL: https://github.com/apache/gravitino/pull/6657#discussion_r2118895571
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/operation/DorisTableOperations.java:
##########
@@ -795,4 +795,14 @@ protected Distribution getDistributionInfo(
return DorisUtils.extractDistributionInfoFromSql(createTableSyntax);
}
}
+
+ @Override
+ public Integer calculateDatetimePrecision(String typeName, int columnSize,
int scale) {
+ String upperTypeName = typeName.toUpperCase();
+ if (upperTypeName.equals("DATETIME")) {
+ // DATETIME format: 'YYYY-MM-DD HH:MM:SS' (19 chars) + decimal point +
precision
+ return columnSize >= 20 ? columnSize - 20 : 0;
Review Comment:
suggest use `"YYYY-MM-DD HH:MM:SS.".length()` instead of constant `20`
directly
##########
trino-connector/trino-connector/src/main/java/org/apache/gravitino/trino/connector/catalog/jdbc/mysql/MySQLDataTypeTransformer.java:
##########
@@ -38,23 +38,75 @@ public class MySQLDataTypeTransformer extends
GeneralDataTypeTransformer {
// column should be less than 16383. For more details, please refer to
// https://dev.mysql.com/doc/refman/8.0/en/char.html
private static final int MYSQL_VARCHAR_LENGTH_LIMIT = 16383;
+ private static final int TIMESTAMP_PRECISION_SECONDS = 0;
+ private static final int TIMESTAMP_PRECISION_MILLIS = 3;
+ private static final int TIMESTAMP_PRECISION_MICROS = 6;
@Override
public io.trino.spi.type.Type getTrinoType(Type type) {
if (type.name() == Name.STRING) {
return io.trino.spi.type.VarcharType.createUnboundedVarcharType();
} else if (Name.TIMESTAMP == type.name()) {
Types.TimestampType timestampType = (Types.TimestampType) type;
- if (timestampType.hasTimeZone()) {
+ return timestampType.hasTimeZone()
+ ? getTimestampWithTimeZoneType(timestampType)
+ : getTimestampType(timestampType);
+ } else if (Name.TIME == type.name()) {
+ return getTimeType(((Types.TimeType) type));
+ }
+ return super.getTrinoType(type);
+ }
+
+ private static TimestampWithTimeZoneType getTimestampWithTimeZoneType(
+ Types.TimestampType timestampType) {
+
+ int precision =
+ timestampType.hasPrecisionSet() ? timestampType.precision() :
TIMESTAMP_PRECISION_SECONDS;
+ switch (precision) {
+ case TIMESTAMP_PRECISION_SECONDS:
return TimestampWithTimeZoneType.TIMESTAMP_TZ_SECONDS;
- } else {
+ case TIMESTAMP_PRECISION_MILLIS:
+ return TimestampWithTimeZoneType.TIMESTAMP_TZ_MILLIS;
+ case TIMESTAMP_PRECISION_MICROS:
+ return TimestampWithTimeZoneType.TIMESTAMP_TZ_MICROS;
+ default:
+ throw new TrinoException(
+ GravitinoErrorCode.GRAVITINO_ILLEGAL_ARGUMENT,
+ "Invalid MySQL timestamp precision: " + precision + ". Valid
values are 0, 3, 6");
Review Comment:
You can add a comment to describe the source of this behavior.
##########
catalogs/catalog-jdbc-doris/src/main/java/org/apache/gravitino/catalog/doris/converter/DorisTypeConverter.java:
##########
@@ -58,7 +59,9 @@ public Type toGravitino(JdbcTypeBean typeBean) {
case DATE:
return Types.DateType.get();
case DATETIME:
- return Types.TimestampType.withoutTimeZone();
+ return Optional.ofNullable(typeBean.getDatetimePrecision())
+ .map(Types.TimestampType::withoutTimeZone)
+ .orElseGet(Types.TimestampType::withoutTimeZone);
Review Comment:
always return `Types.TimestampType::withoutTimeZone` ?
##########
catalogs/catalog-jdbc-doris/src/test/java/org/apache/gravitino/catalog/doris/integration/test/CatalogDorisIT.java:
##########
Review Comment:
could you plz add tests for datetime/timestamp column with specific
precision?
##########
catalogs/catalog-jdbc-mysql/src/main/java/org/apache/gravitino/catalog/mysql/converter/MysqlTypeConverter.java:
##########
@@ -72,15 +73,21 @@ public Type toGravitino(JdbcTypeBean typeBean) {
case DATE:
return Types.DateType.get();
case TIME:
- return Types.TimeType.get();
+ return Optional.ofNullable(typeBean.getDatetimePrecision())
+ .map(Types.TimeType::of)
+ .orElseGet(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();
+ return Optional.ofNullable(typeBean.getDatetimePrecision())
Review Comment:
when will `typeBean.getDatetimePrecision()` return null?
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/operation/JdbcTableOperations.java:
##########
@@ -626,4 +631,16 @@ protected JdbcColumn.Builder
getBasicJdbcColumnInfo(ResultSet column) throws SQL
.withNullable(nullable)
.withDefaultValue(defaultValue);
}
+
+ /**
+ * Calculate the precision for time/datetime/timestamp types.
+ *
+ * @param typeName the type name from database
+ * @param columnSize the column size from database
+ * @param scale
+ * @return the precision of the time/datetime/timestamp type
+ */
+ public Integer calculateDatetimePrecision(String typeName, int columnSize,
int scale) {
+ return null;
Review Comment:
why always return null?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
