kgyrtkirk commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r882462556
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java:
##########
@@ -271,7 +271,16 @@ private static boolean isOwner(IMetaStoreClient
metastoreClient, String userName
thriftTableObj = metastoreClient.getTable(hivePrivObject.getDbname(),
hivePrivObject.getObjectName());
} catch (Exception e) {
- throwGetObjErr(e, hivePrivObject);
+ boolean isTableExists = true;
+ try {
+ if(!metastoreClient.tableExists(hivePrivObject.getDbname(),
hivePrivObject.getObjectName())) {
Review Comment:
I think this method should throw an exception if the table doesn't exists -
you can't own non-exiting items; that's misleading...
* this is a private method - so it could only be called from this class
* the `metastoreClient` is passed as an argument to this method - so I don't
think that should be a problem...
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/misc/rename/AbstractAlterTableRenameAnalyzer.java:
##########
@@ -49,6 +55,13 @@ protected void analyzeCommand(TableName tableName,
Map<String, String> partition
setAcidDdlDesc(desc);
}
addInputsOutputsAlterTable(tableName, null, desc, desc.getType(), false);
+// inputs.add(new ReadEntity(table));
+ outputs.clear();
Review Comment:
I see - but this will make it more diverse...what do you think about the
following:
* split the `addInputsOutputsAlterTable` into 2 methods which are taking
care of inputs/outputs
* you could add back `addInputsOutputsAlterTable` which is calling these 2
for backward compatibility
* and call these 2 methods with old/new args from here?
##########
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java:
##########
@@ -3612,8 +3612,8 @@ private void testRenameTable(boolean blocking) throws
Exception {
txnMgr2.acquireLocks(driver2.getPlan(), ctx, null, false);
locks = getLocks();
- ShowLocksResponseElement checkLock = checkLock(LockType.EXCLUSIVE,
- LockState.WAITING, "default", "tab_acid", null, locks);
+ ShowLocksResponseElement checkLock = checkLock(LockType.SHARED_READ,
Review Comment:
this is the most serious issue - without addressing this I'm against this
change
--
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]
