Copilot commented on code in PR #6314:
URL: https://github.com/apache/hive/pull/6314#discussion_r2796328855
##########
standalone-metastore/metastore-server/src/main/sql/mysql/upgrade-4.2.0-to-4.3.0.mysql.sql:
##########
@@ -1,5 +1,7 @@
SELECT 'Upgrading MetaStore schema from 4.2.0 to 4.3.0' AS MESSAGE;
+ALTER TABLE HIVE_LOCKS ADD HL_CATALOG varchar(128) NOT NULL DEFAULT 'hive';
+
Review Comment:
The 4.2.0->4.3.0 upgrade adds `HL_CATALOG`, but the 4.3.0 schema also adds
`MRL_CAT_NAME` to `MATERIALIZATION_REBUILD_LOCKS`. This upgrade script should
also add the new column (with an appropriate NOT NULL/default) to avoid runtime
failures after upgrade.
##########
standalone-metastore/metastore-server/src/main/sql/oracle/upgrade-4.2.0-to-4.3.0.oracle.sql:
##########
@@ -1,5 +1,7 @@
SELECT 'Upgrading MetaStore schema from 4.2.0 to 4.3.0' AS Status from dual;
+ALTER TABLE HIVE_LOCKS ADD (HL_CATALOG VARCHAR2(128) DEFAULT 'hive' NOT NULL);
+
Review Comment:
This upgrade script adds `HL_CATALOG` to `HIVE_LOCKS`, but the 4.3.0 schema
also introduces `MRL_CAT_NAME` on `MATERIALIZATION_REBUILD_LOCKS`. Please add
an `ALTER TABLE MATERIALIZATION_REBUILD_LOCKS ADD (MRL_CAT_NAME ... )` step
here as well, otherwise upgraded schemas will not match the code/schema-4.3.0
definition.
##########
standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift:
##########
@@ -1277,30 +1278,32 @@ struct UnlockRequest {
}
struct ShowLocksRequest {
- 1: optional string dbname,
- 2: optional string tablename,
- 3: optional string partname,
- 4: optional bool isExtended=false,
- 5: optional i64 txnid,
+ 1: optional string catname,
+ 2: optional string dbname,
+ 3: optional string tablename,
+ 4: optional string partname,
+ 5: optional bool isExtended=false,
+ 6: optional i64 txnid,
}
struct ShowLocksResponseElement {
1: required i64 lockid,
- 2: required string dbname,
- 3: optional string tablename,
- 4: optional string partname,
- 5: required LockState state,
- 6: required LockType type,
- 7: optional i64 txnid,
- 8: required i64 lastheartbeat,
- 9: optional i64 acquiredat,
- 10: required string user,
- 11: required string hostname,
- 12: optional i32 heartbeatCount = 0,
- 13: optional string agentInfo,
- 14: optional i64 blockedByExtId,
- 15: optional i64 blockedByIntId,
- 16: optional i64 lockIdInternal,
+ 2: required string catname,
+ 3: required string dbname,
+ 4: optional string tablename,
+ 5: optional string partname,
+ 6: required LockState state,
+ 7: required LockType type,
+ 8: optional i64 txnid,
+ 9: required i64 lastheartbeat,
+ 10: optional i64 acquiredat,
+ 11: required string user,
+ 12: required string hostname,
+ 13: optional i32 heartbeatCount = 0,
+ 14: optional string agentInfo,
+ 15: optional i64 blockedByExtId,
+ 16: optional i64 blockedByIntId,
+ 17: optional i64 lockIdInternal,
}
Review Comment:
In `hive_metastore.thrift`, the existing field IDs for `LockComponent`,
`ShowLocksRequest`, and `ShowLocksResponseElement` have been renumbered when
adding catalog fields. Renumbering Thrift field IDs breaks wire compatibility
(older clients will deserialize fields into the wrong members). To preserve
compatibility, keep all existing field IDs unchanged and add the new catalog
fields using new, previously-unused IDs (and handle missing values by
defaulting to the default catalog).
##########
standalone-metastore/metastore-server/src/main/sql/derby/upgrade-4.2.0-to-4.3.0.derby.sql:
##########
@@ -1,2 +1,4 @@
+ALTER TABLE HIVE_LOCKS ADD COLUMN HL_CATALOG varchar(128) NOT NULL DEFAULT
'hive';
+
Review Comment:
This upgrade script adds `HL_CATALOG` to `HIVE_LOCKS`, but the 4.3.0 schema
also adds `MRL_CAT_NAME` to `MATERIALIZATION_REBUILD_LOCKS`. Please add the
corresponding `ALTER TABLE MATERIALIZATION_REBUILD_LOCKS ...` step here so
upgraded Derby schemas match the new code expectations.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/lock/show/ShowLocksOperation.java:
##########
@@ -191,6 +194,8 @@ public static void dumpLockInfo(DataOutputStream os,
ShowLocksResponse response)
if (!sessionState.isHiveServerQuery()) {
os.writeBytes("Lock ID");
os.write(Utilities.tabCode);
+ os.writeBytes("Catalog");
+ os.write(Utilities.tabCode);
os.writeBytes("Database");
os.write(Utilities.tabCode);
Review Comment:
PR description says "No user facing change", but `SHOW LOCKS` output format
is changed by adding a new `Catalog` column/header here (and tests are updated
accordingly). Please update the PR description/release notes accordingly, or
clarify why this is considered non-user-facing.
##########
standalone-metastore/metastore-common/src/main/protobuf/org/apache/hadoop/hive/metastore/hive_metastore.proto:
##########
@@ -2950,9 +2951,10 @@ message GetLockMaterializationRebuildResponse {
}
message HeartbeatLockMaterializationRebuildRequest {
- string db_name = 1;
- string table_name = 2;
- int64 txn_id = 3;
+ string cat_name = 1;
+ string db_name = 2;
+ string table_name = 3;
+ int64 txn_id = 4;
}
Review Comment:
In `hive_metastore.proto`, the field numbers for the materialization rebuild
lock request messages were shifted when adding `cat_name`. In protobuf,
changing existing field numbers is wire-breaking; older/newer clients will
misinterpret serialized data. Keep existing field numbers stable and add
`cat_name` with a new field number (and reserve removed numbers if any).
##########
standalone-metastore/metastore-server/src/main/sql/mssql/upgrade-4.2.0-to-4.3.0.mssql.sql:
##########
@@ -1,5 +1,7 @@
SELECT 'Upgrading MetaStore schema from 4.2.0 to 4.3.0' AS MESSAGE;
+ALTER TABLE HIVE_LOCKS ADD HL_CATALOG nvarchar(128) NOT NULL DEFAULT 'hive';
+
Review Comment:
The upgrade adds `HL_CATALOG`, but the 4.3.0 schema also adds `MRL_CAT_NAME`
to `MATERIALIZATION_REBUILD_LOCKS`. The upgrade path needs to add that column
too (NOT NULL/default) to prevent failures in materialization rebuild lock code
after upgrading an existing metastore.
##########
standalone-metastore/metastore-server/src/main/sql/hive/upgrade-4.2.0-to-4.3.0.hive.sql:
##########
@@ -1,3 +1,5 @@
SELECT 'Upgrading MetaStore schema from 4.2.0 to 4.3.0';
+ALTER TABLE `HIVE_LOCKS` ADD COLUMNS (`HL_CATALOG` string);
+
Review Comment:
The 4.3.0 schema adds `MRL_CAT_NAME` to `MATERIALIZATION_REBUILD_LOCKS`, but
this upgrade script only alters `HIVE_LOCKS`. The upgrade should also evolve
`MATERIALIZATION_REBUILD_LOCKS` to include the catalog column, otherwise
upgraded installations can fail when the new materialization rebuild lock code
runs.
##########
ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java:
##########
@@ -481,6 +483,7 @@ private Collection<LockComponent>
getGlobalLocks(Configuration conf) {
LockComponentBuilder compBuilder = new LockComponentBuilder();
compBuilder.setExclusive();
compBuilder.setOperationType(DataOperationType.NO_TXN);
+ compBuilder.setCatName(currentCatalog);
compBuilder.setDbName(GLOBAL_LOCKS);
Review Comment:
`getGlobalLocks` now calls `SessionState.get().getCurrentCatalog()` without
checking for a null `SessionState`. This can throw NPE when the txn manager is
used in non-session contexts (tests/background execution). Consider falling
back to
`MetaStoreUtils.getDefaultCatalog(conf)`/`Warehouse.DEFAULT_CATALOG_NAME` when
`SessionState.get()` is null.
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/Msck.java:
##########
@@ -59,6 +59,7 @@
import org.apache.hadoop.hive.metastore.txn.TxnUtils;
import org.apache.hadoop.hive.metastore.utils.FileUtils;
import org.apache.hadoop.hive.metastore.utils.MetaStoreServerUtils;
+import org.apache.hadoop.hive.metastore.utils.MetaStoreUtils;
import org.apache.hadoop.hive.metastore.utils.RetryUtilities;
Review Comment:
`MetaStoreUtils` is imported but not used in this class. Please remove the
unused import to keep the import list clean.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/lock/show/ShowLocksAnalyzer.java:
##########
@@ -44,29 +48,56 @@ public ShowLocksAnalyzer(QueryState queryState) throws
SemanticException {
public void analyzeInternal(ASTNode root) throws SemanticException {
ctx.setResFile(ctx.getLocalTmpPath());
- String tableName = null;
+ String fullyQualifiedTableName = null;
Map<String, String> partitionSpec = null;
boolean isExtended = false;
if (root.getChildCount() >= 1) {
// table for which show locks is being executed
for (int i = 0; i < root.getChildCount(); i++) {
ASTNode child = (ASTNode) root.getChild(i);
if (child.getType() == HiveParser.TOK_TABTYPE) {
- tableName = DDLUtils.getFQName((ASTNode) child.getChild(0));
+ fullyQualifiedTableName = DDLUtils.getFQName((ASTNode)
child.getChild(0));
// get partition metadata if partition specified
if (child.getChildCount() == 2) {
ASTNode partitionSpecNode = (ASTNode) child.getChild(1);
- partitionSpec = getValidatedPartSpec(getTable(tableName),
partitionSpecNode, conf, false);
+ partitionSpec =
getValidatedPartSpec(getTable(fullyQualifiedTableName), partitionSpecNode,
conf, false);
}
} else if (child.getType() == HiveParser.KW_EXTENDED) {
isExtended = true;
}
}
}
+ String catalogName = null;
+ String dbName = null;
+ String tableName = null;
+
+ if (fullyQualifiedTableName != null) {
+ List<String> splitFullyQualifiedTableName =
Arrays.stream(fullyQualifiedTableName.split("\\.")).toList();
+ if (splitFullyQualifiedTableName.size() == 1) {
+ catalogName = SessionState.get().getCurrentCatalog();
+ dbName = SessionState.get().getCurrentDatabase();
+ tableName = splitFullyQualifiedTableName.get(0);
+ } else if (splitFullyQualifiedTableName.size() == 2) {
+ catalogName = SessionState.get().getCurrentCatalog();
+ dbName = splitFullyQualifiedTableName.get(0);
+ tableName = splitFullyQualifiedTableName.get(1);
+ } else {
+ catalogName = splitFullyQualifiedTableName.get(0);
+ dbName = splitFullyQualifiedTableName.get(1);
+ tableName = splitFullyQualifiedTableName.get(2);
+ }
Review Comment:
`ShowLocksAnalyzer` parses the fully qualified name by doing a raw
`split("\\.")`. This is fragile because Hive identifiers can be quoted/escaped
and may legally contain '.' characters, which would be incorrectly treated as
catalog/db/table separators. Prefer using the existing table-name parsing
utilities (e.g., `TableName.fromString(...)` / `HiveTableName.fromString(...)`
or parsing from the AST node) instead of splitting the rendered string.
##########
standalone-metastore/metastore-server/src/main/sql/postgres/upgrade-4.2.0-to-4.3.0.postgres.sql:
##########
@@ -1,5 +1,7 @@
SELECT 'Upgrading MetaStore schema from 4.2.0 to 4.3.0';
+ALTER TABLE "HIVE_LOCKS" ADD COLUMN "HL_CATALOG" varchar(128) NOT NULL DEFAULT
'hive';
+
Review Comment:
The 4.2.0->4.3.0 upgrade adds `HL_CATALOG`, but the 4.3.0 schema also adds
`MRL_CAT_NAME` to `MATERIALIZATION_REBUILD_LOCKS`. Without an `ALTER TABLE
MATERIALIZATION_REBUILD_LOCKS ADD ... MRL_CAT_NAME ...` here, upgraded
metastores will fail when the code starts querying/inserting with the new
column.
##########
streaming/src/java/org/apache/hive/streaming/TransactionBatch.java:
##########
@@ -23,6 +23,7 @@
import org.apache.hadoop.hive.common.JavaUtils;
import org.apache.hadoop.hive.conf.HiveConf;
import org.apache.hadoop.hive.metastore.LockComponentBuilder;
+import org.apache.hadoop.hive.metastore.utils.MetaStoreUtils;
import org.apache.hadoop.hive.metastore.LockRequestBuilder;
Review Comment:
`MetaStoreUtils` is imported but not referenced anywhere in this file.
Please remove the unused import (or use it if the intent was to default the
catalog name).
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
