jarredhj0214 commented on code in PR #13548:
URL: https://github.com/apache/gravitino/pull/13548#discussion_r4225821286
##########
catalogs/catalog-jdbc-mysql/src/main/java/org/apache/gravitino/catalog/mysql/converter/MysqlTypeConverter.java:
##########
@@ -166,4 +167,17 @@ public String fromGravitino(Type type) {
throw new IllegalArgumentException(
String.format("Couldn't convert Gravitino type %s to MySQL type",
type.simpleString()));
}
+
+ private static Type toGravitinoDecimal(JdbcTypeBean typeBean) {
+ int precision = typeBean.getColumnSize();
+ int scale = typeBean.getScale();
+ if (precision <= MAX_DECIMAL_PRECISION) {
+ return Types.DecimalType.of(precision, scale);
Review Comment:
Added comments explaining that Gravitino DecimalType is limited to precision
38 and that larger MySQL decimals are preserved as ExternalType to avoid
failing table loads. Addressed in 53e809c4a.
##########
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:
Added a comment on MAX_DECIMAL_PRECISION documenting that it must stay in
sync with Types.DecimalType.checkPrecisionScale. Addressed in 53e809c4a.
##########
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:
Added CatalogMysqlIT.testLoadHighPrecisionDecimalTable, which creates a
table directly in MySQL with DECIMAL(38,30), DECIMAL(39,30), DECIMAL(65,30),
and DECIMAL(65,30) UNSIGNED, then loads it through Gravitino and asserts the
expected DecimalType/ExternalType values. Also added reverse-conversion unit
coverage. spotlessApply and the MySQL module unit tests passed; the IT
compiled, but I could not execute it locally because Docker is unavailable.
Addressed in 53e809c4a.
--
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]
