Copilot commented on code in PR #13036:
URL: https://github.com/apache/gravitino/pull/13036#discussion_r3966048429
##########
catalogs/catalog-jdbc-mysql/src/main/java/org/apache/gravitino/catalog/mysql/operation/MysqlTableOperations.java:
##########
@@ -203,6 +214,95 @@ protected void correctJdbcTableFields(
tableBuilder.withComment(
tableBuilder.properties().getOrDefault(COMMENT,
tableBuilder.comment()));
}
+ correctLossyTypeColumns(connection, databaseName, tableName, tableBuilder);
+ }
+
+ /**
+ * VARBINARY/ENUM/SET fall back to a bare Types.ExternalType, and BIT/BINARY
of any width both
+ * collapse to plain Types.BinaryType, because JdbcTypeConverter only sees
TYPE_NAME/COLUMN_SIZE.
+ * If any column needs it, this queries information_schema.columns once for
the whole table to
+ * recover the full declaration (e.g. "enum('a','b','c')", "binary(16)") and
fixes those columns.
+ */
+ private static void correctLossyTypeColumns(
+ Connection connection, String databaseName, String tableName,
JdbcTable.Builder tableBuilder)
+ throws SQLException {
+ Column[] columns = tableBuilder.columns();
+ if (ArrayUtils.isEmpty(columns)
+ ||
Arrays.stream(columns).noneMatch(MysqlTableOperations::isLossyTypeCandidate)) {
+ return;
+ }
+
+ Map<String, String> fullTypesByColumn =
+ fetchColumnFullTypes(connection, databaseName, tableName);
+ JdbcColumn[] correctedColumns = new JdbcColumn[columns.length];
+ for (int i = 0; i < columns.length; i++) {
+ JdbcColumn column = (JdbcColumn) columns[i];
+ correctedColumns[i] =
+ isLossyTypeCandidate(column)
+ ? correctColumnType(column, fullTypesByColumn.get(column.name()))
+ : column;
+ }
+ tableBuilder.withColumns(correctedColumns);
+ }
+
+ private static boolean isLossyTypeCandidate(Column column) {
+ Type type = column.dataType();
+ if (type instanceof Types.BinaryType) {
+ return true;
+ }
+ if (type instanceof Types.ExternalType) {
+ String catalogString = ((Types.ExternalType) type).catalogString();
+ return VARBINARY.equalsIgnoreCase(catalogString)
+ || ENUM.equalsIgnoreCase(catalogString)
+ || SET.equalsIgnoreCase(catalogString);
+ }
+ return false;
+ }
+
+ private static JdbcColumn correctColumnType(JdbcColumn column, String
fullType) {
+ if (StringUtils.isEmpty(fullType)) {
+ return column;
+ }
+ // A plain BINARY(1) (or unspecified length) already round-trips to
Types.BinaryType as-is.
+ if (column.dataType() instanceof Types.BinaryType && parseLength(fullType)
<= 1) {
+ return column;
+ }
+ return JdbcColumn.builder()
+ .withName(column.name())
+ .withType(Types.ExternalType.of(fullType))
+ .withComment(column.comment())
+ .withNullable(column.nullable())
+ .withAutoIncrement(column.autoIncrement())
+ .withDefaultValue(column.defaultValue())
+ .build();
+ }
+
+ private static int parseLength(String fullType) {
+ Matcher matcher = LENGTH_PATTERN.matcher(fullType);
+ return matcher.find() ? Integer.parseInt(matcher.group(1)) : 1;
+ }
+
+ private static Map<String, String> fetchColumnFullTypes(
+ Connection connection, String databaseName, String tableName) throws
SQLException {
+ Map<String, String> columnTypes = new HashMap<>();
+ // TABLE_SCHEMA/TABLE_NAME comparisons in information_schema use a
case-insensitive collation,
+ // so with lower_case_table_names=0 a schema holding both "a_b" and "A_B"
would return rows for
+ // both; re-check TABLE_NAME exactly, matching the pattern used in
getColumnBuilder.
+ String query =
+ "SELECT TABLE_NAME, COLUMN_NAME, COLUMN_TYPE FROM
information_schema.columns "
+ + "WHERE TABLE_SCHEMA = ? AND TABLE_NAME = ?";
+ try (PreparedStatement statement = connection.prepareStatement(query)) {
+ statement.setString(1, databaseName);
+ statement.setString(2, tableName);
+ try (ResultSet resultSet = statement.executeQuery()) {
+ while (resultSet.next()) {
+ if (Objects.equals(resultSet.getString("TABLE_NAME"), tableName)) {
+ columnTypes.put(resultSet.getString("COLUMN_NAME"),
resultSet.getString("COLUMN_TYPE"));
+ }
+ }
+ }
+ }
Review Comment:
The exact `TABLE_NAME` re-check can cause `columnTypes` to remain empty on
MySQL servers where `lower_case_table_names=1` (table names are stored/returned
in lowercase, but callers may pass mixed case). In that scenario the query
matches rows, but the `Objects.equals(...)` filter drops them, preventing any
lossy-type correction. A robust approach is to prefer exact-case matches when
present (for `lower_case_table_names=0` ambiguity), but fall back to
case-insensitive matching when no exact matches exist (or explicitly query
`@@lower_case_table_names` and adjust the comparison accordingly).
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcTable.java:
##########
@@ -113,6 +114,13 @@ public String comment() {
public Map<String, String> properties() {
return properties;
}
+
+ /**
+ * @return The columns currently set on this builder.
+ */
+ public Column[] columns() {
+ return columns;
Review Comment:
Returning the builder’s internal `Column[]` exposes mutable state to callers
(they can modify the array contents and inadvertently corrupt the builder).
Consider returning a defensive copy (e.g., `Arrays.copyOf(columns,
columns.length)`) or a read-only view (e.g., `List<Column>`) to keep the
builder encapsulated.
##########
catalogs/catalog-jdbc-mysql/src/main/java/org/apache/gravitino/catalog/mysql/operation/MysqlTableOperations.java:
##########
@@ -203,6 +214,95 @@ protected void correctJdbcTableFields(
tableBuilder.withComment(
tableBuilder.properties().getOrDefault(COMMENT,
tableBuilder.comment()));
}
+ correctLossyTypeColumns(connection, databaseName, tableName, tableBuilder);
+ }
+
+ /**
+ * VARBINARY/ENUM/SET fall back to a bare Types.ExternalType, and BIT/BINARY
of any width both
+ * collapse to plain Types.BinaryType, because JdbcTypeConverter only sees
TYPE_NAME/COLUMN_SIZE.
+ * If any column needs it, this queries information_schema.columns once for
the whole table to
+ * recover the full declaration (e.g. "enum('a','b','c')", "binary(16)") and
fixes those columns.
+ */
+ private static void correctLossyTypeColumns(
+ Connection connection, String databaseName, String tableName,
JdbcTable.Builder tableBuilder)
+ throws SQLException {
+ Column[] columns = tableBuilder.columns();
+ if (ArrayUtils.isEmpty(columns)
+ ||
Arrays.stream(columns).noneMatch(MysqlTableOperations::isLossyTypeCandidate)) {
+ return;
+ }
+
+ Map<String, String> fullTypesByColumn =
+ fetchColumnFullTypes(connection, databaseName, tableName);
+ JdbcColumn[] correctedColumns = new JdbcColumn[columns.length];
+ for (int i = 0; i < columns.length; i++) {
+ JdbcColumn column = (JdbcColumn) columns[i];
+ correctedColumns[i] =
+ isLossyTypeCandidate(column)
+ ? correctColumnType(column, fullTypesByColumn.get(column.name()))
+ : column;
+ }
Review Comment:
This logic assumes every `Column` in `tableBuilder.columns()` is a
`JdbcColumn` and performs an unchecked cast. If a different `Column`
implementation is ever used in the builder (now that `columns()` is exposed),
this will fail at runtime. Consider guarding with `instanceof JdbcColumn` (and
skipping correction or rebuilding in a more generic way when it’s not), so the
correction remains safe if builder usage evolves.
##########
catalogs/catalog-jdbc-mysql/src/test/java/org/apache/gravitino/catalog/mysql/operation/TestMysqlTableOperations.java:
##########
@@ -1139,6 +1143,55 @@ public void testCalculateDatetimePrecision() {
"Non-datetime type should return 0 precision");
}
+ @Test
+ public void testLoadColumnsWithLossyTypeDeclarations() throws Exception {
+ // These types either drop their parameters (VARBINARY/ENUM/SET) or
collapse distinct widths
+ // into the same Gravitino type (BIT/BINARY) when only JDBC's
TYPE_NAME/COLUMN_SIZE metadata
+ // is used.
+ String tableName = RandomStringUtils.randomAlphabetic(16) +
"_lossy_type_table";
+ DataSource dataSource =
DataSourceUtils.createDataSource(getMySQLCatalogProperties());
+ try {
+ try (Connection connection = dataSource.getConnection()) {
+ connection.setCatalog(TEST_DB_NAME.toString());
+ try (Statement statement = connection.createStatement()) {
+ statement.execute(
+ String.format(
+ "CREATE TABLE `%s` ("
+ + "c_int INT, "
+ + "c_varbinary VARBINARY(100), "
+ + "c_enum ENUM('a','b','c'), "
+ + "c_set SET('x','y','z'), "
+ + "c_bit BIT(8), "
+ + "c_bit1 BIT(1), "
+ + "c_binary1 BINARY(1), "
+ + "c_binary BINARY(16))",
+ tableName));
+ }
+ }
+ } finally {
+ DataSourceUtils.closeDataSource(dataSource);
+ }
+
+ JdbcTable table = TABLE_OPERATIONS.load(TEST_DB_NAME.toString(),
tableName);
+ Map<String, Type> typeByColumn =
+ Arrays.stream(table.columns()).collect(Collectors.toMap(Column::name,
Column::dataType));
+
+ // A plain, non-candidate column must be left untouched by the correction.
+ Assertions.assertEquals(Types.IntegerType.get(),
typeByColumn.get("c_int"));
+ Assertions.assertEquals(
+ Types.ExternalType.of("varbinary(100)"),
typeByColumn.get("c_varbinary"));
+ Assertions.assertEquals(Types.ExternalType.of("enum('a','b','c')"),
typeByColumn.get("c_enum"));
+ Assertions.assertEquals(Types.ExternalType.of("set('x','y','z')"),
typeByColumn.get("c_set"));
+ Assertions.assertEquals(Types.ExternalType.of("bit(8)"),
typeByColumn.get("c_bit"));
+ Assertions.assertEquals(Types.ExternalType.of("binary(16)"),
typeByColumn.get("c_binary"));
+ // BIT(1) still maps to boolean, and a default-width BINARY(1) already
round-trips correctly
+ // as Types.BinaryType, so neither must be affected by this fix.
+ Assertions.assertEquals(Types.BooleanType.get(),
typeByColumn.get("c_bit1"));
+ Assertions.assertEquals(Types.BinaryType.get(),
typeByColumn.get("c_binary1"));
+
+ TABLE_OPERATIONS.drop(TEST_DB_NAME.toString(), tableName);
Review Comment:
The test drops the table only at the end; if any assertion fails (or `load`
throws), the table won’t be dropped, which can pollute the test DB and cause
follow-up failures in CI. Wrap the `load` + assertions in a `try`/`finally`
that always calls `TABLE_OPERATIONS.drop(...)` (optionally ignoring 'table not
found' if creation failed earlier).
##########
catalogs/catalog-jdbc-mysql/src/main/java/org/apache/gravitino/catalog/mysql/operation/MysqlTableOperations.java:
##########
@@ -203,6 +214,95 @@ protected void correctJdbcTableFields(
tableBuilder.withComment(
tableBuilder.properties().getOrDefault(COMMENT,
tableBuilder.comment()));
}
+ correctLossyTypeColumns(connection, databaseName, tableName, tableBuilder);
+ }
+
+ /**
+ * VARBINARY/ENUM/SET fall back to a bare Types.ExternalType, and BIT/BINARY
of any width both
+ * collapse to plain Types.BinaryType, because JdbcTypeConverter only sees
TYPE_NAME/COLUMN_SIZE.
+ * If any column needs it, this queries information_schema.columns once for
the whole table to
+ * recover the full declaration (e.g. "enum('a','b','c')", "binary(16)") and
fixes those columns.
+ */
+ private static void correctLossyTypeColumns(
+ Connection connection, String databaseName, String tableName,
JdbcTable.Builder tableBuilder)
+ throws SQLException {
+ Column[] columns = tableBuilder.columns();
+ if (ArrayUtils.isEmpty(columns)
+ ||
Arrays.stream(columns).noneMatch(MysqlTableOperations::isLossyTypeCandidate)) {
+ return;
+ }
+
+ Map<String, String> fullTypesByColumn =
+ fetchColumnFullTypes(connection, databaseName, tableName);
+ JdbcColumn[] correctedColumns = new JdbcColumn[columns.length];
+ for (int i = 0; i < columns.length; i++) {
+ JdbcColumn column = (JdbcColumn) columns[i];
Review Comment:
This logic assumes every `Column` in `tableBuilder.columns()` is a
`JdbcColumn` and performs an unchecked cast. If a different `Column`
implementation is ever used in the builder (now that `columns()` is exposed),
this will fail at runtime. Consider guarding with `instanceof JdbcColumn` (and
skipping correction or rebuilding in a more generic way when it’s not), so the
correction remains safe if builder usage evolves.
--
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]
