mchades commented on code in PR #6657:
URL: https://github.com/apache/gravitino/pull/6657#discussion_r2215912820
##########
catalogs/catalog-jdbc-mysql/src/main/java/org/apache/gravitino/catalog/mysql/converter/MysqlColumnDefaultValueConverter.java:
##########
@@ -49,7 +54,8 @@ public Expression toGravitino(
}
if (isExpression) {
- if (columnDefaultValue.equals(CURRENT_TIMESTAMP)) {
+ if (columnDefaultValue.equals(CURRENT_TIMESTAMP)
+ || columnDefaultValue.startsWith(CURRENT_TIMESTAMP + "(")) {
Review Comment:
can be simplified to `columnDefaultValue.startsWith(CURRENT_TIMESTAMP)`?
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/operation/JdbcTableOperations.java:
##########
@@ -626,4 +633,117 @@ protected JdbcColumn.Builder
getBasicJdbcColumnInfo(ResultSet column) throws SQL
.withNullable(nullable)
.withDefaultValue(defaultValue);
}
+
+ /**
+ * Calculate the precision for time/datetime/timestamp types.
+ *
+ * <p>This method provides a default behavior returning null for catalogs
that do not implement
+ * this method. Developers should override this method when their database
supports precision
+ * specification for datetime types.
+ *
+ * <p><strong>When to return null:</strong>
+ *
+ * <ul>
+ * <li>When the database does not support precision for datetime types
+ * <li>When the driver version is incompatible (e.g., MySQL driver <
8.0.16)
+ * <li>When the type is not a datetime type (TIME, TIMESTAMP, DATETIME)
+ * <li>When the precision cannot be accurately calculated from the
provided parameters
+ * </ul>
+ *
+ * <p><strong>When to return non-null:</strong>
+ *
+ * <ul>
+ * <li>When the database supports precision for datetime types and the
driver version is
+ * compatible
+ * <li>When the precision can be accurately calculated from columnSize
(e.g., columnSize -
+ * format_length)
+ * <li>For TIME types: return columnSize - 8 (for 'HH:MM:SS' format)
+ * <li>For TIMESTAMP/DATETIME types: return columnSize - 19 (for
'YYYY-MM-DD HH:MM:SS' format)
+ * </ul>
+ *
+ * <p><strong>Examples:</strong>
+ *
+ * <ul>
+ * <li>TIME(3) with columnSize=11: return 3 (11-8)
+ * <li>TIMESTAMP(6) with columnSize=25: return 6 (25-19)
+ * <li>DATETIME(0) with columnSize=19: return 0 (19-19)
+ * </ul>
+ *
+ * @param typeName the type name from database (e.g., "TIME", "TIMESTAMP",
"DATETIME")
+ * @param columnSize the column size from database (total length including
format and precision)
+ * @param scale the scale from database (usually 0 for datetime types)
+ * @return the precision of the time/datetime/timestamp type, or null if not
supported/calculable
+ */
+ public Integer calculateDatetimePrecision(String typeName, int columnSize,
int scale) {
+ return null;
+ }
+
+ /**
+ * Get MySQL driver version from DatabaseMetaData
+ *
+ * @return the driver version string, or null if not available
+ */
+ protected String getMySQLDriverVersion() {
+ try {
+ if (dataSource != null) {
+ try (Connection connection = dataSource.getConnection()) {
+ return connection.getMetaData().getDriverVersion();
+ }
+ }
+ } catch (SQLException e) {
+ LOG.debug("Failed to get driver version", e);
+ }
+ return null;
+ }
+
+ /**
+ * Check if driver version supports accurate columnSize for precision
calculation. For MySQL
+ * driver: Only versions >= 8.0.16 return accurate columnSize for
datetime precision. For other
+ * drivers (like OceanBase): Assume they support accurate precision
calculation
+ *
+ * @param driverVersion the driver version string to check
+ * @return true if the driver version supports accurate precision
calculation, false otherwise
+ */
+ public boolean isMySQLDriverVersionSupported(String driverVersion) {
+ if (StringUtils.isBlank(driverVersion)) {
+ return false;
+ }
+
+ // For non-MySQL drivers, assume they support accurate precision
calculation
+ if (!driverVersion.startsWith("mysql-connector-java-")) {
+ LOG.debug(
+ "Non-MySQL driver detected: {}. Assuming it supports accurate
precision calculation.",
+ driverVersion);
+ return true;
+ }
+
+ // Extract version from driver string like "mysql-connector-java-8.0.19
(Revision: ...)"
+ String versionPattern = "mysql-connector-java-(\\d+\\.\\d+\\.\\d+)";
+ java.util.regex.Pattern pattern =
java.util.regex.Pattern.compile(versionPattern);
+ java.util.regex.Matcher matcher = pattern.matcher(driverVersion);
Review Comment:
plz remove the package prefix
##########
docs/jdbc-doris-catalog.md:
##########
@@ -86,21 +111,21 @@ Please refer to
#### Table column types
-| Gravitino Type | Doris Type |
-|----------------|------------|
-| `Boolean` | `Boolean` |
-| `Byte` | `TinyInt` |
-| `Short` | `SmallInt` |
-| `Integer` | `Int` |
-| `Long` | `BigInt` |
-| `Float` | `Float` |
-| `Double` | `Double` |
-| `Decimal` | `Decimal` |
-| `Date` | `Date` |
-| `Timestamp` | `Datetime` |
-| `VarChar` | `VarChar` |
-| `FixedChar` | `Char` |
-| `String` | `String` |
+| Gravitino Type | Doris Type |
+|----------------|---------------|
+| `Boolean` | `Boolean` |
+| `Byte` | `TinyInt` |
+| `Short` | `SmallInt` |
+| `Integer` | `Int` |
+| `Long` | `BigInt` |
+| `Float` | `Float` |
+| `Double` | `Double` |
+| `Decimal` | `Decimal` |
+| `Date` | `Date` |
+| `Timestamp(p)` | `Datetime(p)` |
Review Comment:
use `Timestamp[(p)]` since the p is unnecessary
##########
docs/lakehouse-iceberg-catalog.md:
##########
@@ -232,25 +232,25 @@ If you doesn't specify distribution expressions, the
table distribution will be
### Table column types
-| Gravitino Type | Apache Iceberg Type |
-|-----------------------------|-----------------------------|
-| `Struct` | `Struct` |
-| `Map` | `Map` |
-| `List` | `Array` |
-| `Boolean` | `Boolean` |
-| `Integer` | `Integer` |
-| `Long` | `Long` |
-| `Float` | `Float` |
-| `Double` | `Double` |
-| `String` | `String` |
-| `Date` | `Date` |
-| `Time` | `Time` |
-| `TimestampType withZone` | `TimestampType withZone` |
-| `TimestampType withoutZone` | `TimestampType withoutZone` |
-| `Decimal` | `Decimal` |
-| `Fixed` | `Fixed` |
-| `Binary` | `Binary` |
-| `UUID` | `UUID` |
+| Gravitino Type | Apache Iceberg Type |
+|-------------------|-----------------------------|
+| `Struct` | `Struct` |
+| `Map` | `Map` |
+| `List` | `Array` |
+| `Boolean` | `Boolean` |
+| `Integer` | `Integer` |
+| `Long` | `Long` |
+| `Float` | `Float` |
+| `Double` | `Double` |
+| `String` | `String` |
+| `Date` | `Date` |
+| `Time[6]` | `Time` |
+| `Timestamp[6]` | `TimestampType withZone` |
+| `Timestamp_tz[6]` | `TimestampType withoutZone` |
Review Comment:
`Timestamp_tz(6)`
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/operation/JdbcTableOperations.java:
##########
@@ -626,4 +633,117 @@ protected JdbcColumn.Builder
getBasicJdbcColumnInfo(ResultSet column) throws SQL
.withNullable(nullable)
.withDefaultValue(defaultValue);
}
+
+ /**
+ * Calculate the precision for time/datetime/timestamp types.
+ *
+ * <p>This method provides a default behavior returning null for catalogs
that do not implement
+ * this method. Developers should override this method when their database
supports precision
+ * specification for datetime types.
+ *
+ * <p><strong>When to return null:</strong>
+ *
+ * <ul>
+ * <li>When the database does not support precision for datetime types
+ * <li>When the driver version is incompatible (e.g., MySQL driver <
8.0.16)
+ * <li>When the type is not a datetime type (TIME, TIMESTAMP, DATETIME)
+ * <li>When the precision cannot be accurately calculated from the
provided parameters
+ * </ul>
+ *
+ * <p><strong>When to return non-null:</strong>
+ *
+ * <ul>
+ * <li>When the database supports precision for datetime types and the
driver version is
+ * compatible
+ * <li>When the precision can be accurately calculated from columnSize
(e.g., columnSize -
+ * format_length)
+ * <li>For TIME types: return columnSize - 8 (for 'HH:MM:SS' format)
+ * <li>For TIMESTAMP/DATETIME types: return columnSize - 19 (for
'YYYY-MM-DD HH:MM:SS' format)
+ * </ul>
+ *
+ * <p><strong>Examples:</strong>
+ *
+ * <ul>
+ * <li>TIME(3) with columnSize=11: return 3 (11-8)
+ * <li>TIMESTAMP(6) with columnSize=25: return 6 (25-19)
+ * <li>DATETIME(0) with columnSize=19: return 0 (19-19)
+ * </ul>
+ *
+ * @param typeName the type name from database (e.g., "TIME", "TIMESTAMP",
"DATETIME")
+ * @param columnSize the column size from database (total length including
format and precision)
+ * @param scale the scale from database (usually 0 for datetime types)
+ * @return the precision of the time/datetime/timestamp type, or null if not
supported/calculable
+ */
+ public Integer calculateDatetimePrecision(String typeName, int columnSize,
int scale) {
+ return null;
+ }
+
+ /**
+ * Get MySQL driver version from DatabaseMetaData
+ *
+ * @return the driver version string, or null if not available
+ */
+ protected String getMySQLDriverVersion() {
+ try {
+ if (dataSource != null) {
+ try (Connection connection = dataSource.getConnection()) {
+ return connection.getMetaData().getDriverVersion();
+ }
+ }
+ } catch (SQLException e) {
+ LOG.debug("Failed to get driver version", e);
+ }
+ return null;
+ }
+
+ /**
+ * Check if driver version supports accurate columnSize for precision
calculation. For MySQL
+ * driver: Only versions >= 8.0.16 return accurate columnSize for
datetime precision. For other
+ * drivers (like OceanBase): Assume they support accurate precision
calculation
Review Comment:
Why should the driver version be greater than 8.0.16? Could you please
attach a relevant document link here?
##########
catalogs/catalog-jdbc-mysql/src/main/java/org/apache/gravitino/catalog/mysql/converter/MysqlColumnDefaultValueConverter.java:
##########
@@ -88,10 +94,17 @@ public Expression toGravitino(
return Literals.timeLiteral(LocalTime.parse(columnDefaultValue,
DATE_TIME_FORMATTER));
case JdbcTypeConverter.TIMESTAMP:
case MysqlTypeConverter.DATETIME:
- return CURRENT_TIMESTAMP.equals(columnDefaultValue)
- ? DEFAULT_VALUE_OF_CURRENT_TIMESTAMP
- : Literals.timestampLiteral(
- LocalDateTime.parse(columnDefaultValue, DATE_TIME_FORMATTER));
+ if (CURRENT_TIMESTAMP.equals(columnDefaultValue)
+ || columnDefaultValue.startsWith(CURRENT_TIMESTAMP + "(")) {
Review Comment:
can be simplified to `columnDefaultValue.startsWith(CURRENT_TIMESTAMP)`?
##########
catalogs/catalog-lakehouse-iceberg/src/test/java/org/apache/gravitino/catalog/lakehouse/iceberg/integration/test/CatalogIcebergBaseIT.java:
##########
@@ -1303,6 +1304,150 @@ public void testTableSortOrder() {
Assertions.assertDoesNotThrow(() ->
tableCatalog.dropTable(tableIdentifier));
}
+ @Test
+ void testTimeTypePrecision() {
+ String tableName = GravitinoITUtils.genRandomName("test_time_precision");
+ NameIdentifier tableIdentifier = NameIdentifier.of(schemaName, tableName);
+ Column[] columns = createColumns();
+ columns =
+ ArrayUtils.addAll(
+ columns,
+ // time type
+ Column.of("time_col", Types.TimeType.get()),
+ Column.of("time_col_0", Types.TimeType.of(0)),
+ Column.of("time_col_1", Types.TimeType.of(1)),
+ Column.of("time_col_3", Types.TimeType.of(3)),
+ Column.of("time_col_6", Types.TimeType.of(6)),
+ // timestamp type
+ Column.of("timestamp_col", Types.TimestampType.withTimeZone()),
+ Column.of("timestamp_col_0", Types.TimestampType.withTimeZone(0)),
+ Column.of("timestamp_col_1", Types.TimestampType.withTimeZone(1)),
+ Column.of("timestamp_col_3", Types.TimestampType.withTimeZone(3)),
+ Column.of("timestamp_col_6", Types.TimestampType.withTimeZone(6)),
+ // datetime type (without time zone)
+ Column.of("datetime_col", Types.TimestampType.withoutTimeZone()),
+ Column.of("datetime_col_0",
Types.TimestampType.withoutTimeZone(0)),
+ Column.of("datetime_col_1",
Types.TimestampType.withoutTimeZone(1)),
+ Column.of("datetime_col_3",
Types.TimestampType.withoutTimeZone(3)),
+ Column.of("datetime_col_6",
Types.TimestampType.withoutTimeZone(6)));
+
+ Map<String, String> properties = createProperties();
+ TableCatalog tableCatalog = catalog.asTableCatalog();
+ tableCatalog.createTable(
+ tableIdentifier,
+ columns,
+ table_comment,
+ properties,
+ Transforms.EMPTY_TRANSFORM,
+ Distributions.NONE,
+ new SortOrder[0]);
+
+ Table loadTable = tableCatalog.loadTable(tableIdentifier);
+
+ // Verify time type precisions
+ Column[] timeColumns =
+ Arrays.stream(loadTable.columns())
+ .filter(c -> c.name().startsWith("time_col"))
+ .toArray(Column[]::new);
+
+ Assertions.assertEquals(5, timeColumns.length);
+ for (Column column : timeColumns) {
+ switch (column.name()) {
+ case "time_col":
+ case "time_col_0":
+ case "time_col_1":
+ case "time_col_3":
+ case "time_col_6":
+ Assertions.assertEquals(Types.TimeType.of(6), column.dataType());
Review Comment:
why precision 0,1,3 equals 6?
##########
catalogs/catalog-lakehouse-paimon/src/main/java/org/apache/gravitino/catalog/lakehouse/paimon/utils/TypeUtils.java:
##########
@@ -229,11 +232,19 @@ public static DataType visit(Type type) {
case DATE:
return DataTypes.DATE();
case TIME:
- return DataTypes.TIME();
+ Types.TimeType timeType = (Types.TimeType) type;
+ int timeTypePrecision =
+ timeType.hasPrecisionSet() ? timeType.precision() :
PRECISION_SECOND;
Review Comment:
why all precision convert to PRECISION_SECOND?
##########
catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/converter/FromIcebergType.java:
##########
@@ -90,13 +92,15 @@ public Type
primitive(org.apache.iceberg.types.Type.PrimitiveType primitive) {
case DATE:
return org.apache.gravitino.rel.types.Types.DateType.get();
case TIME:
- return org.apache.gravitino.rel.types.Types.TimeType.get();
+ return
org.apache.gravitino.rel.types.Types.TimeType.of(PRECISION_MICROSECOND);
Review Comment:
Iceberg only supports one micro precision? Could you please attach the
relevant document here?
##########
docs/jdbc-mysql-catalog.md:
##########
@@ -54,6 +54,34 @@ Besides the [common catalog
properties](./gravitino-server-config.md#apache-grav
You must download the corresponding JDBC driver to the
`catalogs/jdbc-mysql/libs` directory.
:::
+### Driver Version Compatibility
+
+The MySQL catalog includes driver version compatibility checks for datetime
precision calculation:
+
+- **MySQL Connector/J versions >= 8.0.16**: Full support for datetime
precision calculation
+- **MySQL Connector/J versions < 8.0.16**: Limited support - datetime
precision calculation returns `null` with a warning log
+
+This limitation affects the following datetime types:
+- `TIME(p)` - time precision
+- `TIMESTAMP(p)` - timestamp precision
+- `DATETIME(p)` - datetime precision
+
+When using an unsupported driver version, the system will:
+1. Continue to work normally with default precision (0)
+2. Log a warning message indicating the driver version limitation
+3. Return `null` for precision calculations to avoid incorrect results
+
+**Example warning log:**
+```
+WARN: MySQL driver version mysql-connector-java-8.0.11 is below 8.0.16,
+columnSize may not be accurate for precision calculation.
+Returning null for TIMESTAMP type precision. Driver version:
mysql-connector-java-8.0.11
+```
+
+**Recommended driver versions:**
+- `mysql-connector-java-8.0.16` or higher
+- `mysql-connector-java-8.0.26` (default in Gravitino integration tests)
Review Comment:
this line is unnessary
##########
catalogs/catalog-lakehouse-iceberg/src/main/java/org/apache/gravitino/catalog/lakehouse/iceberg/converter/FromIcebergType.java:
##########
@@ -90,13 +92,15 @@ public Type
primitive(org.apache.iceberg.types.Type.PrimitiveType primitive) {
case DATE:
return org.apache.gravitino.rel.types.Types.DateType.get();
case TIME:
- return org.apache.gravitino.rel.types.Types.TimeType.get();
+ return
org.apache.gravitino.rel.types.Types.TimeType.of(PRECISION_MICROSECOND);
Review Comment:
should we also update the logic in `FromIcebergType`?
--
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]
