Copilot commented on code in PR #13351:
URL: https://github.com/apache/gravitino/pull/13351#discussion_r4056423023
##########
catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/client/HiveShimV3.java:
##########
@@ -331,6 +455,22 @@ public void alterTable(
databaseName,
tableName,
tb);
+ } catch (RuntimeException e) {
+ // The table is unchanged, so the dropped constraints can be put back as
they were.
+ restoreColumnConstraints(databaseName, tableName, existing, e);
+ throw e;
+ }
+ try {
+ addColumnConstraints(alteredHiveTable.name(), desired);
+ } catch (RuntimeException e) {
+ LOG.error(
+ "Table {}.{} was altered but its column constraints {} could not be
re-created; "
+ + "the NOT NULL and DEFAULT constraints must be re-applied
manually",
+ alteredHiveTable.databaseName(),
+ alteredHiveTable.name(),
Review Comment:
If re-adding constraints fails after alter_table succeeds, the table has
already been altered but the caller only receives the original exception (the
guidance is only in server logs). Consider rethrowing an exception whose
message explicitly states the table was altered but constraints were not
re-created, so callers/users understand the partial-success state.
##########
catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/integration/test/CatalogHive3IT.java:
##########
@@ -39,4 +50,128 @@ protected void startNecessaryContainer() {
containerSuite.getHiveContainer().getContainerIpAddress(),
HiveContainer.HIVE_METASTORE_PORT);
}
+
+ /** Hive 3.x metastores support NOT NULL and DEFAULT constraints, which must
round-trip to HMS. */
+ @Override
+ protected void checkColumnConstraintsOnCreate(
+ NameIdentifier nameIdentifier, Map<String, String> properties) {
+ NameIdentifier constraintsIdent =
+ NameIdentifier.of(schemaName, nameIdentifier.name() + "_constraints");
+ Column notNullColumn =
+ Column.of("not_null_column", Types.StringType.get(), "not null
column", false, false, null);
+ Column defaultColumn =
+ Column.of(
+ "default_column",
+ Types.IntegerType.get(),
+ "default column",
+ true,
+ false,
+ Literals.integerLiteral(42));
+ Column defaultStringColumn =
+ Column.of(
+ "default_string_column",
+ Types.StringType.get(),
+ null,
+ false,
+ false,
+ Literals.stringLiteral("it's"));
+ Column plainColumn = Column.of("plain_column", Types.StringType.get(),
"plain column");
+
+ catalog
+ .asTableCatalog()
+ .createTable(
+ constraintsIdent,
+ new Column[] {notNullColumn, defaultColumn, defaultStringColumn,
plainColumn},
+ TABLE_COMMENT,
+ properties,
+ Transforms.EMPTY_TRANSFORM);
+
+ Table loaded = catalog.asTableCatalog().loadTable(constraintsIdent);
+ assertColumnConstraints(loaded.columns());
+ // Read back straight from HMS to make sure the constraints were persisted
+ assertColumnConstraints(loadHiveTableColumns(schemaName,
constraintsIdent.name()));
+
+ // Property-only alters must leave the constraints untouched
+ catalog.asTableCatalog().alterTable(constraintsIdent,
TableChange.setProperty("k1", "v1"));
+ assertColumnConstraints(loadHiveTableColumns(schemaName,
constraintsIdent.name()));
+
+ // Constraints follow the table when it is renamed
+ NameIdentifier renamedIdent =
+ NameIdentifier.of(schemaName, constraintsIdent.name() + "_renamed");
+ catalog.asTableCatalog().alterTable(constraintsIdent,
TableChange.rename(renamedIdent.name()));
+
assertColumnConstraints(catalog.asTableCatalog().loadTable(renamedIdent).columns());
+ assertColumnConstraints(loadHiveTableColumns(schemaName,
renamedIdent.name()));
+
+ catalog.asTableCatalog().dropTable(renamedIdent);
+ }
+
+ private Column[] loadHiveTableColumns(String schema, String table) {
+ try {
+ return loadHiveTable(schema, table).columns();
+ } catch (InterruptedException e) {
+ throw new RuntimeException(e);
+ }
Review Comment:
InterruptedException is wrapped into RuntimeException without restoring the
thread interrupt flag. This can cause higher-level test infrastructure to miss
the interruption and continue running unexpectedly.
##########
catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/converter/HiveTableConverter.java:
##########
@@ -343,6 +358,22 @@ public static Transform[]
getPartitioning(org.apache.hadoop.hive.metastore.api.T
}
public static Column[] getColumns(org.apache.hadoop.hive.metastore.api.Table
table) {
+ return getColumns(table, Collections.emptySet(), Collections.emptyMap());
+ }
+
+ /**
+ * Converts the storage and partition columns of a Hive metastore table,
applying the given column
+ * constraints.
+ *
+ * @param table The Hive metastore table.
+ * @param notNullColumns Names of the columns that carry a NOT NULL
constraint.
+ * @param defaultValues Column name to Hive SQL default value expression.
+ * @return The converted columns, storage columns first followed by
partition columns.
+ */
+ public static Column[] getColumns(
+ org.apache.hadoop.hive.metastore.api.Table table,
+ Set<String> notNullColumns,
+ Map<String, String> defaultValues) {
StorageDescriptor sd = table.getSd();
Review Comment:
The new getColumns(Table, Set, Map) overload assumes
notNullColumns/defaultValues are non-null; passing null will NPE at
notNullColumns.contains(...) or defaultValues.get(...). Since this is a public
utility, defensively normalize null inputs (and optionally validate table !=
null) to avoid unexpected NPEs.
--
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]
