geyanggang commented on code in PR #13548:
URL: https://github.com/apache/gravitino/pull/13548#discussion_r4217713735
##########
catalogs/catalog-jdbc-mysql/src/main/java/org/apache/gravitino/catalog/mysql/converter/MysqlTypeConverter.java:
##########
@@ -26,6 +26,7 @@
/** Type converter for MySQL. */
public class MysqlTypeConverter extends JdbcTypeConverter {
+ static final int MAX_DECIMAL_PRECISION = 38;
Review Comment:
nit: worth a one-line comment that this mirrors the limit enforced by
Types.DecimalType.checkPrecisionScale, so the two stay in sync if the core
limit ever changes.
##########
catalogs/catalog-jdbc-mysql/src/test/java/org/apache/gravitino/catalog/mysql/converter/TestMysqlTypeConverter.java:
##########
@@ -74,6 +74,11 @@ public void testToGravitinoType() {
checkJdbcTypeToGravitinoType(Types.TimestampType.withTimeZone(9),
TIMESTAMP, 29, null, 9);
checkJdbcTypeToGravitinoType(Types.DecimalType.of(10, 2), DECIMAL, 10, 2,
0);
checkJdbcTypeToGravitinoType(Types.DecimalType.of(10, 2),
DECIMAL_UNSIGNED, 10, 2, 0);
+ checkJdbcTypeToGravitinoType(Types.DecimalType.of(38, 30), DECIMAL, 38,
30, 0);
+ checkJdbcTypeToGravitinoType(Types.ExternalType.of("decimal(39,30)"),
DECIMAL, 39, 30, 0);
+ checkJdbcTypeToGravitinoType(Types.ExternalType.of("decimal(65,30)"),
DECIMAL, 65, 30, 0);
+ checkJdbcTypeToGravitinoType(
+ Types.ExternalType.of("decimal(65,30) unsigned"), DECIMAL_UNSIGNED,
65, 30, 0);
Review Comment:
The unit coverage looks good. Could we also add an integration test in
CatalogMysqlIT that actually loads a real DECIMAL(65,30) table and asserts the
column comes back as ExternalType.of("decimal(65,30)")? That's the exact path
the issue reports (load + grant), and it's the main thing not covered by the
converter-only test.
--
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]
