This is an automated email from the ASF dual-hosted git repository.
yuqi1129 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 6714b80c41 [#14497] fix(jdbc): make rejected table changes readable
(#14507)
6714b80c41 is described below
commit 6714b80c411b0e9ddef27a9777d8ce570e264779
Author: Qi Yu <[email protected]>
AuthorDate: Mon Nov 2 18:06:43 2026 +0800
[#14497] fix(jdbc): make rejected table changes readable (#14507)
### What changes were proposed in this pull request?
Give eleven built-in column/index TableChange classes readable
descriptions and format conflicting rename changes as a comma-separated
list. Add a default connector hook for an unsupported sort-order
explanation, and use it in the JDBC create guard before database access.
### Why are the changes needed?
JDBC rejections expose Java class names/object hashes and hide
connector-specific recovery advice.
Fixes #14497
### Does this PR introduce _any_ user-facing change?
Only string representations and rejection messages change. Existing
IllegalArgumentException/HTTP validation behavior and rejection before
DDL are retained. The default hook works for existing connectors;
connectors may override it without changing capability support.
### How was this patch tested?
- Full API and JDBC common non-Docker unit suites: 323 and 128 tests, no
failures.
- Added assertions for all eleven descriptions, conflicting rename
messages and sort-order rejection through the catalog entry point.
- Reverting the renderers makes both API and catalog regressions fail;
restoring the patch passes both full suites again.
- Spotless, RAT and independent fresh-context review passed.
- No Docker/live connector or REST end-to-end test was run; changed
rejection paths run before database access.
---
.../java/org/apache/gravitino/rel/TableChange.java | 70 ++++++++++++++++++++++
.../java/org/apache/gravitino/TestTableChange.java | 34 +++++++++++
.../catalog/jdbc/JdbcCatalogOperations.java | 19 +++---
.../jdbc/operation/JdbcTableOperations.java | 14 +++++
.../catalog/jdbc/TestJdbcCatalogOperations.java | 47 +++++++++++++++
5 files changed, 174 insertions(+), 10 deletions(-)
diff --git a/api/src/main/java/org/apache/gravitino/rel/TableChange.java
b/api/src/main/java/org/apache/gravitino/rel/TableChange.java
index 719a6f3f7a..2493b2500e 100644
--- a/api/src/main/java/org/apache/gravitino/rel/TableChange.java
+++ b/api/src/main/java/org/apache/gravitino/rel/TableChange.java
@@ -838,6 +838,12 @@ public interface TableChange {
result = 31 * result + Arrays.deepHashCode(fieldNames);
return result;
}
+
+ /** Returns a readable description of this change. */
+ @Override
+ public String toString() {
+ return "ADDINDEX " + getName();
+ }
}
/**
@@ -884,6 +890,12 @@ public interface TableChange {
public int hashCode() {
return Objects.hash(name, ifExists);
}
+
+ /** Returns a readable description of this change. */
+ @Override
+ public String toString() {
+ return "DELETEINDEX " + getName();
+ }
}
/**
@@ -1152,6 +1164,12 @@ public interface TableChange {
public String[] fieldName() {
return fieldName;
}
+
+ /** Returns a readable description of this change. */
+ @Override
+ public String toString() {
+ return "ADDCOLUMN " + String.join(".", fieldName()) + " " +
dataType.simpleString();
+ }
}
/**
@@ -1221,6 +1239,12 @@ public interface TableChange {
public String[] fieldName() {
return fieldName;
}
+
+ /** Returns a readable description of this change. */
+ @Override
+ public String toString() {
+ return "RENAMECOLUMN " + String.join(".", fieldName()) + " TO " +
newName;
+ }
}
/**
@@ -1286,6 +1310,13 @@ public interface TableChange {
result = 31 * result + Arrays.hashCode(fieldName);
return result;
}
+
+ /** Returns a readable description of this change. */
+ @Override
+ public String toString() {
+ // Default expressions may be connector-defined and have no user-facing
renderer.
+ return "UPDATECOLUMNDEFAULTVALUE " + String.join(".", fieldName());
+ }
}
/**
@@ -1355,6 +1386,15 @@ public interface TableChange {
public String[] fieldName() {
return fieldName;
}
+
+ /** Returns a readable description of this change. */
+ @Override
+ public String toString() {
+ return "UPDATECOLUMNTYPE "
+ + String.join(".", fieldName())
+ + " TO "
+ + newDataType.simpleString();
+ }
}
/**
@@ -1424,6 +1464,12 @@ public interface TableChange {
public String[] fieldName() {
return fieldName;
}
+
+ /** Returns a readable description of this change. */
+ @Override
+ public String toString() {
+ return "UPDATECOLUMNCOMMENT " + String.join(".", fieldName()) + " TO " +
newComment;
+ }
}
/**
@@ -1494,6 +1540,12 @@ public interface TableChange {
public String[] fieldName() {
return fieldName;
}
+
+ /** Returns a readable description of this change. */
+ @Override
+ public String toString() {
+ return "UPDATECOLUMNPOSITION " + String.join(".", fieldName()) + " TO "
+ position;
+ }
}
/**
@@ -1560,6 +1612,12 @@ public interface TableChange {
public String[] fieldName() {
return fieldName;
}
+
+ /** Returns a readable description of this change. */
+ @Override
+ public String toString() {
+ return "DELETECOLUMN " + String.join(".", fieldName());
+ }
}
/**
@@ -1616,6 +1674,12 @@ public interface TableChange {
result = 31 * result + Arrays.hashCode(fieldName);
return result;
}
+
+ /** Returns a readable description of this change. */
+ @Override
+ public String toString() {
+ return "UPDATECOLUMNNULLABILITY " + String.join(".", fieldName()) + " TO
" + nullable;
+ }
}
/**
@@ -1671,5 +1735,11 @@ public interface TableChange {
result = 31 * result + Arrays.hashCode(fieldName);
return result;
}
+
+ /** Returns a readable description of this change. */
+ @Override
+ public String toString() {
+ return "UPDATECOLUMNAUTOINCREMENT " + String.join(".", fieldName()) + "
TO " + autoIncrement;
+ }
}
}
diff --git a/api/src/test/java/org/apache/gravitino/TestTableChange.java
b/api/src/test/java/org/apache/gravitino/TestTableChange.java
index 67ab3e3978..f5302537f8 100644
--- a/api/src/test/java/org/apache/gravitino/TestTableChange.java
+++ b/api/src/test/java/org/apache/gravitino/TestTableChange.java
@@ -53,6 +53,40 @@ import org.junit.jupiter.api.Test;
public class TestTableChange {
+ @Test
+ public void testReadableChangeDescriptions() {
+ String[] field = {"nested", "c"};
+ TableChange[] changes = {
+ TableChange.addColumn(field, Types.StringType.get()),
+ TableChange.renameColumn(field, "new_name"),
+ TableChange.updateColumnDefaultValue(field,
Literals.stringLiteral("value")),
+ TableChange.updateColumnType(field, Types.LongType.get()),
+ TableChange.updateColumnComment(field, "comment"),
+ TableChange.updateColumnPosition(field, ColumnPosition.first()),
+ TableChange.deleteColumn(field, true),
+ TableChange.updateColumnNullability(field, false),
+ TableChange.updateColumnAutoIncrement(field, true),
+ TableChange.addIndex(IndexType.UNIQUE_KEY, "idx", new String[][]
{field}),
+ TableChange.deleteIndex("idx", true)
+ };
+ String[] descriptions = {
+ "ADDCOLUMN nested.c string",
+ "RENAMECOLUMN nested.c TO new_name",
+ "UPDATECOLUMNDEFAULTVALUE nested.c",
+ "UPDATECOLUMNTYPE nested.c TO long",
+ "UPDATECOLUMNCOMMENT nested.c TO comment",
+ "UPDATECOLUMNPOSITION nested.c TO FIRST",
+ "DELETECOLUMN nested.c",
+ "UPDATECOLUMNNULLABILITY nested.c TO false",
+ "UPDATECOLUMNAUTOINCREMENT nested.c TO true",
+ "ADDINDEX idx",
+ "DELETEINDEX idx"
+ };
+ for (int i = 0; i < changes.length; i++) {
+ assertEquals(descriptions[i], changes[i].toString());
+ }
+ }
+
@Test
public void testRenameTable() {
String newName = "New table name";
diff --git
a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java
b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java
index 5b67a4aaf4..631b53db12 100644
---
a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java
+++
b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/JdbcCatalogOperations.java
@@ -605,13 +605,10 @@ public class JdbcCatalogOperations
Arrays.stream(changes)
.filter(tableChange -> !(tableChange instanceof
TableChange.RenameTable))
.map(String::valueOf)
- .collect(Collectors.joining("\n"));
+ .collect(Collectors.joining(", "));
Preconditions.checkArgument(
StringUtils.isEmpty(otherChange),
- String.format(
- "The operation to change the table name cannot be performed
together with other operations."
- + "The list of operations that you cannot perform includes:
\n %s",
- otherChange));
+ String.format("A table rename cannot be combined with other changes:
%s", otherChange));
return renameTable(tableIdent, (TableChange.RenameTable)
renameTableOptional.get());
}
return internalAlterTable(tableIdent, changes);
@@ -654,11 +651,13 @@ public class JdbcCatalogOperations
Index[] indexes)
throws NoSuchSchemaException, TableAlreadyExistsException {
JdbcTableOperations jdbcTableOperations = (JdbcTableOperations)
tableOperation;
- if (!jdbcTableOperations.supportsTableSortOrder()) {
- Preconditions.checkArgument(
- null == sortOrders || sortOrders.length == 0,
- "The current JDBC connector: %s does not support sort orders",
- jdbcTableOperations.getClass().getName());
+ if (!jdbcTableOperations.supportsTableSortOrder()
+ && sortOrders != null
+ && sortOrders.length > 0) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Cannot create table %s: %s",
+ tableIdent,
jdbcTableOperations.unsupportedTableSortOrderMessage()));
}
StringIdentifier identifier = StringIdentifier.fromProperties(properties);
diff --git
a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/operation/JdbcTableOperations.java
b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/operation/JdbcTableOperations.java
index 39b462b0de..e5ea76112f 100644
---
a/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/operation/JdbcTableOperations.java
+++
b/catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/operation/JdbcTableOperations.java
@@ -266,6 +266,20 @@ public abstract class JdbcTableOperations implements
TableOperation {
return false;
}
+ /**
+ * Returns the user-facing reason that this connector rejects table sort
orders.
+ *
+ * <p>Called by the catalog before database access when {@link
#supportsTableSortOrder()} is false
+ * and a create request contains sort orders. Connectors may override this
method to name the
+ * catalog type and suggest a supported alternative. The message must not
contain internal
+ * implementation class names.
+ *
+ * @return the rejection message, without the table identifier
+ */
+ public String unsupportedTableSortOrderMessage() {
+ return "The catalog does not support sort orders";
+ }
+
@Override
public JdbcTable load(String databaseName, String tableName) throws
NoSuchTableException {
// We should handle case sensitivity and wild card issue in some catalog
tables, take MySQL
diff --git
a/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/TestJdbcCatalogOperations.java
b/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/TestJdbcCatalogOperations.java
index 4a888ef3e8..96610185e0 100644
---
a/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/TestJdbcCatalogOperations.java
+++
b/catalogs/catalog-jdbc-common/src/test/java/org/apache/gravitino/catalog/jdbc/TestJdbcCatalogOperations.java
@@ -41,11 +41,58 @@ import
org.apache.gravitino.catalog.jdbc.operation.SqliteTableOperations;
import org.apache.gravitino.catalog.jdbc.utils.DataSourceUtils;
import org.apache.gravitino.exceptions.ConnectionFailedException;
import org.apache.gravitino.exceptions.GravitinoRuntimeException;
+import org.apache.gravitino.rel.Column;
+import org.apache.gravitino.rel.TableChange;
+import org.apache.gravitino.rel.expressions.sorts.SortOrder;
+import org.apache.gravitino.rel.expressions.transforms.Transform;
+import org.apache.gravitino.rel.indexes.Index;
+import org.apache.gravitino.rel.types.Types;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
public class TestJdbcCatalogOperations {
+ @Test
+ public void testRejectedChangesHaveReadableMessages() {
+ try (JdbcCatalogOperations operations =
+ new JdbcCatalogOperations(
+ new SqliteExceptionConverter(),
+ new SqliteTypeConverter(),
+ new SqliteDatabaseOperations("/unused"),
+ new SqliteTableOperations(),
+ new SqliteColumnDefaultValueConverter())) {
+ NameIdentifier table = NameIdentifier.of("metalake", "catalog",
"schema", "table");
+ IllegalArgumentException rename =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ operations.alterTable(
+ table,
+ TableChange.rename("new_table"),
+ TableChange.addColumn(new String[] {"nested", "c"},
Types.StringType.get()),
+ TableChange.deleteColumn(new String[] {"old"}, true)));
+ Assertions.assertEquals(
+ "A table rename cannot be combined with other changes: ADDCOLUMN
nested.c string, DELETECOLUMN old",
+ rename.getMessage());
+ IllegalArgumentException sort =
+ Assertions.assertThrows(
+ IllegalArgumentException.class,
+ () ->
+ operations.createTable(
+ table,
+ new Column[0],
+ null,
+ Collections.emptyMap(),
+ new Transform[0],
+ null,
+ new SortOrder[] {null},
+ new Index[0]));
+ Assertions.assertEquals(
+ "Cannot create table metalake.catalog.schema.table: The catalog does
not support sort orders",
+ sort.getMessage());
+ }
+ }
+
@Test
public void testExistingCatalogConnectionFailure() {
SQLException cause = new SQLException("connection refused");
