You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@hive.apache.org by GitBox <gi...@apache.org> on 2022/05/11 07:23:13 UTC
[GitHub] [hive] kgyrtkirk commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
kgyrtkirk commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r869945626
##########
itests/util/src/main/java/org/apache/hadoop/hive/ql/hooks/VerifyOutputTableLocationSchemeIsFileHook.java:
##########
@@ -24,10 +24,10 @@ public class VerifyOutputTableLocationSchemeIsFileHook implements ExecuteWithHoo
@Override
public void run(HookContext hookContext) {
- for (WriteEntity output : hookContext.getOutputs()) {
- if (output.getType() == WriteEntity.Type.TABLE) {
- String scheme = output.getTable().getDataLocation().toUri().getScheme();
- Assert.assertTrue(output.getTable().getTableName() + " has a location which has a " +
+ for (ReadEntity input : hookContext.getInputs()) {
Review Comment:
the logic here seem to have changed ; and its not aligned anymore with the name of the class
is this change intended? maybe you wanted to add a new hook which check for the input?
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java:
##########
@@ -269,7 +269,19 @@ 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 {
+ isTableExists = metastoreClient.tableExists(hivePrivObject.getDbname(), hivePrivObject.getObjectName());
+ } catch (TException ex){
+ throwGetObjErr(ex, hivePrivObject);
+ }
+ if(!isTableExists){
+ // Do not throw any exception when table object is not present.
+ // return true since the current user the owner of new table.
+ return true;
Review Comment:
I think instead of returning `true` here for non-existent tables; the "call-site" should check if the table already exists and make a decision based on that
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/misc/rename/AbstractAlterTableRenameAnalyzer.java:
##########
@@ -48,7 +54,12 @@ protected void analyzeCommand(TableName tableName, Map<String, String> partition
if (AcidUtils.isTransactionalTable(table)) {
setAcidDdlDesc(desc);
}
- addInputsOutputsAlterTable(tableName, null, desc, desc.getType(), false);
Review Comment:
I think we are throwing out a lot of logic by not calling this method - and that might be the cause of the sheer volume of failures/test output changes
##########
ql/src/test/results/clientnegative/alter_table_wrong_db.q.out:
##########
@@ -18,8 +18,4 @@ POSTHOOK: query: create table rename1(a int)
POSTHOOK: type: CREATETABLE
POSTHOOK: Output: bad_rename1@rename1
POSTHOOK: Output: database:bad_rename1
-PREHOOK: query: alter table bad_rename1.rename1 rename to bad_db_notexists.rename1
-PREHOOK: type: ALTERTABLE_RENAME
-PREHOOK: Input: bad_rename1@rename1
-PREHOOK: Output: bad_rename1@rename1
-FAILED: Execution Error, return code 40013 from org.apache.hadoop.hive.ql.ddl.DDLTask. Unable to alter table. Unable to change partition or table. Object database hive.bad_db_notexists does not exist. Check metastore logs for detailed stack.
+FAILED: SemanticException [Error 10072]: Database does not exist: bad_db_notexists
Review Comment:
negative test fails with a different error
##########
ql/src/test/results/clientnegative/alter_view_failure8.q.out:
##########
@@ -6,4 +6,15 @@ POSTHOOK: query: CREATE TABLE invites (foo INT, bar STRING) PARTITIONED BY (ds S
POSTHOOK: type: CREATETABLE
POSTHOOK: Output: database:default
POSTHOOK: Output: default@invites
-FAILED: SemanticException [Error 10132]: To alter a base table you need to use the ALTER TABLE command.
+PREHOOK: query: ALTER VIEW invites RENAME TO invites2
+PREHOOK: type: ALTERVIEW_RENAME
+PREHOOK: Input: default@invites
+PREHOOK: Output: database:default
+PREHOOK: Output: default@invites2
+POSTHOOK: query: ALTER VIEW invites RENAME TO invites2
+POSTHOOK: type: ALTERVIEW_RENAME
+POSTHOOK: Input: default@invites
+POSTHOOK: Output: database:default
+POSTHOOK: Output: default@invites2
+FAILED: AssertionError java.lang.AssertionError: Client Execution was expected to fail, but succeeded with error code 0 for fname=alter_view_failure8.q
Review Comment:
negative test is failing
##########
ql/src/test/results/clientnegative/alter_non_native.q.out:
##########
@@ -8,4 +8,15 @@ STORED BY 'org.apache.hadoop.hive.ql.metadata.DefaultStorageHandler'
POSTHOOK: type: CREATETABLE
POSTHOOK: Output: database:default
POSTHOOK: Output: default@non_native1
-FAILED: SemanticException [Error 10134]: ALTER TABLE can only be used for [ADDPROPS, DROPPROPS, ADDCOLS] to a non-native table non_native1
+PREHOOK: query: ALTER TABLE non_native1 RENAME TO new_non_native
+PREHOOK: type: ALTERTABLE_RENAME
+PREHOOK: Input: default@non_native1
+PREHOOK: Output: database:default
+PREHOOK: Output: default@new_non_native
+POSTHOOK: query: ALTER TABLE non_native1 RENAME TO new_non_native
+POSTHOOK: type: ALTERTABLE_RENAME
+POSTHOOK: Input: default@non_native1
+POSTHOOK: Output: database:default
+POSTHOOK: Output: default@new_non_native
+FAILED: AssertionError java.lang.AssertionError: Client Execution was expected to fail, but succeeded with error code 0 for fname=alter_non_native.q
Review Comment:
negative test is failing
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java:
##########
@@ -269,7 +269,19 @@ 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 {
+ isTableExists = metastoreClient.tableExists(hivePrivObject.getDbname(), hivePrivObject.getObjectName());
Review Comment:
I think you don't need the `isTableExists` boolean ; you could just:
```
if(!metastoreClient.tableExists(hivePrivObject.getDbname(), hivePrivObject.getObjectName())) {
return 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: gitbox-unsubscribe@hive.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscribe@hive.apache.org
For additional commands, e-mail: gitbox-help@hive.apache.org