Copilot commented on code in PR #13021:
URL: https://github.com/apache/gravitino/pull/13021#discussion_r3965275180
##########
catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/client/HiveShimV2.java:
##########
@@ -294,4 +309,18 @@ public void createCatalog(String catalogName, String
location, String descriptio
public void close() throws Exception {
client.close();
}
+
+ /**
+ * Builds an {@link EnvironmentContext} that tells the metastore not to
recompute table statistics
+ * during an alter, avoiding an access to the table's storage location.
+ *
+ * @return An environment context with {@code DO_NOT_UPDATE_STATS} set to
{@code true}.
+ */
+ protected EnvironmentContext doNotUpdateStatsContext() {
+ // Use the literal "true" rather than StatsSetupConst.TRUE, which is not
present in all
+ // supported
+ // Hive versions (e.g. Hive 3.1.3).
Review Comment:
The inline comment about supported Hive versions is wrapped awkwardly (a
standalone "supported" line), which hurts readability and can conflict with
Google Java Style wrapping conventions. Please reflow it into complete
sentences on each line.
##########
catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/client/HiveShimV2.java:
##########
@@ -148,10 +151,22 @@ public HiveTable getTable(String catalogName, String
databaseName, String tableN
@Override
public void alterTable(
- String catalogName, String databaseName, String tableName, HiveTable
alteredHiveTable) {
+ String catalogName,
+ String databaseName,
+ String tableName,
+ HiveTable alteredHiveTable,
+ boolean skipStatsUpdate) {
try {
var tb = HiveTableConverter.toHiveTable(alteredHiveTable);
- client.alter_table(databaseName, tableName, tb);
+ if (skipStatsUpdate) {
+ // Instruct the metastore not to recompute statistics for this alter,
so it does not access
+ // the table's storage location. Hive 2.x has no catalog-aware alter,
so the database name
+ // is used directly.
+ client.alter_table_with_environmentContext(
+ databaseName, tableName, tb, doNotUpdateStatsContext());
+ } else {
+ client.alter_table(databaseName, tableName, tb);
+ }
Review Comment:
New `skipStatsUpdate` behavior (using `alter_table_with_environmentContext`
/ `alter_table(..., EnvironmentContext)`) is not covered by tests; the added
unit test only validates the decision predicate, not that the Hive client/shim
actually sends `DO_NOT_UPDATE_STATS=true` when requested. Please add a unit
test in `hive-metastore-common` that mocks `IMetaStoreClient` (or
`HiveShimV2`/`HiveShimV3`) and verifies the correct metastore method is invoked
with an `EnvironmentContext` containing `StatsSetupConst.DO_NOT_UPDATE_STATS`
set to "true".
--
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]
