zhangbutao commented on code in PR #6314:
URL: https://github.com/apache/hive/pull/6314#discussion_r2846096581
##########
standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift:
##########
@@ -3308,8 +3311,8 @@ PartitionsResponse get_partitions_req(1:PartitionsRequest
req)
void add_serde(1: SerDeInfo serde) throws(1:AlreadyExistsException o1,
2:MetaException o2)
SerDeInfo get_serde(1: GetSerdeRequest rqst) throws(1:NoSuchObjectException
o1, 2:MetaException o2)
- LockResponse get_lock_materialization_rebuild(1: string dbName, 2: string
tableName, 3: i64 txnId)
- bool heartbeat_lock_materialization_rebuild(1: string dbName, 2: string
tableName, 3: i64 txnId)
+ LockResponse get_lock_materialization_rebuild(1: string catName, 2: string
dbName, 3: string tableName, 4: i64 txnId)
Review Comment:
This is a compatibility-breaking change. If we can confirm that this method
is only used internally by Hive, then this change is safe. However, if other
open-source components also call this method, we need to consider retaining the
original method and adding a new one instead.
Based on my current understanding, this method is only used internally by
Hive. I’d like to ping other folks to take a look together. cc @dengzhhu653
@deniskuzZ
##########
standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift:
##########
@@ -1246,7 +1246,8 @@ struct LockComponent {
5: optional string partitionname,
6: optional DataOperationType operationType = DataOperationType.UNSET,
7: optional bool isTransactional = false,
- 8: optional bool isDynamicPartitionWrite = false
+ 8: optional bool isDynamicPartitionWrite = false,
+ 9: required string catName
Review Comment:
Why not `optional `?
##########
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));
Review Comment:
In future code improvements related to catalog, we recommend using the
`TableName `object to replace passing table names as Strings. This allows
storing catalog/database/table names in TableName, making the expression
clearer. Therefore, I would suggest replacing `DDLUtils::getFQName` with
`BaseSemanticAnalyzer::getQualifiedTableName` here. However,
`BaseSemanticAnalyzer::getQualifiedTableName` does not yet have the ability to
parse catalog. I have made improvements in
https://github.com/apache/hive/pull/6252/changes#diff-ecf0bd4d24a899907e8d368d37d3f4945fd1a323a9da9b607b8444ab9793140d,
but the code hasn't been fully refactored yet.
Using `BaseSemanticAnalyzer::getQualifiedTableName` might be difficult for
your current PR, so we can temporarily maintain the current changes.
##########
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();
Review Comment:
Please use `HiveUtils::getCurrentCatalogOrDefault` instead.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/lock/show/ShowLocksOperation.java:
##########
@@ -161,9 +161,12 @@ private ShowLocksResponse
getLocksForNewFormat(HiveLockManager lockMgr) throws H
throw new HiveException("New lock format only supported with db lock
manager.");
}
- // TODO catalog. Need to add catalog into ShowLocksRequest. But
ShowLocksRequest doesn't have catalog field.
- // Maybe we need to change hive_metastore.thrift to add catalog into
ShowLocksRequest struct. Depend on HIVE-29242.
ShowLocksRequest request = new ShowLocksRequest();
+ if (desc.getCatName() == null && (desc.getDbName() != null ||
desc.getTableName() != null)) {
+ request.setCatname(SessionState.get().getCurrentCatalog());
Review Comment:
Please use `HiveUtils::getCurrentCatalogOrDefault` instead.
##########
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/parse/BaseReplicationScenariosAcidTables.java:
##########
@@ -74,6 +74,7 @@ public class BaseReplicationScenariosAcidTables {
static WarehouseInstance primary;
static WarehouseInstance replica, replicaNonAcid;
static HiveConf conf;
+ String primaryCatName = "hive";
Review Comment:
Please use `Warehouse.DEFAULT_CATALOG_NAME;`
##########
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:
> Can identifiers have '.' in them ?
I think no. But using exsiting `TableName.fromString(...) /
HiveTableName.fromString(...)` is a good suggestion.
##########
ql/src/test/queries/clientpositive/dbtxnmgr_showlocks.q:
##########
@@ -19,6 +19,10 @@ create table partitioned_acid_table (a int, b int)
partitioned by (p string) clu
explain show locks database default;
show locks database default;
+show locks hive.default.partitioned_acid_table;
Review Comment:
I suggest creating a new native catalog and testing whether show locks takes
effect under this new catalog.
Just like what i did in `catalog_database.q`.
--
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]
