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/04/27 01:11:27 UTC
[GitHub] [hive] saihemanth-cloudera opened a new pull request, #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
saihemanth-cloudera opened a new pull request, #3247:
URL: https://github.com/apache/hive/pull/3247
…ame command
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://cwiki.apache.org/confluence/display/Hive/HowToContribute
2. Ensure that you have created an issue on the Hive project JIRA: https://issues.apache.org/jira/projects/HIVE/summary
3. Ensure you have added or run the appropriate tests for your PR:
4. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP]HIVE-XXXXX: Your PR title ...'.
5. Be sure to keep the PR description updated to reflect all changes.
6. Please write your PR title to summarize what this PR proposes.
7. If possible, provide a concise example to reproduce the issue for a faster review.
-->
### What changes were proposed in this pull request?
Added the right HivePrivilegesObjects for Alter table rename command.
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
### Why are the changes needed?
So that authorization happens correctly in ranger/sentry
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
### Does this PR introduce _any_ user-facing change?
No.
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description, screenshot and/or a reproducable example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Hive versions or within the unreleased branches such as master.
If no, write 'No'.
-->
### How was this patch tested?
Manual testing, Added unit tests.
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
-->
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r906402476
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/misc/rename/AbstractAlterTableRenameAnalyzer.java:
##########
@@ -49,6 +55,11 @@ protected void analyzeCommand(TableName tableName, Map<String, String> partition
setAcidDdlDesc(desc);
}
addInputsOutputsAlterTable(tableName, null, desc, desc.getType(), false);
+ String newDatabaseName = target.getDb() != null ? target.getDb() : table.getDbName(); // extract new database name from new table name, if not specified, then src dbname is used
+ Database newDatabase = getDatabase(newDatabaseName);
+ outputs.add(new WriteEntity(newDatabase, WriteEntity.WriteType.DDL_SHARED));
+ Table newTable = new Table(target.getDb(), target.getTable());
Review Comment:
I am not 100% sure, but shouldn't we propagate back from original table it's parameters, type, temp attribute, etc. See `DDLUtils.addDbAndTableToOutputs` & `SemanticAnalyzer.addDbAndTabToOutputs`
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r911066995
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/misc/rename/AbstractAlterTableRenameAnalyzer.java:
##########
@@ -49,6 +55,11 @@ protected void analyzeCommand(TableName tableName, Map<String, String> partition
setAcidDdlDesc(desc);
}
addInputsOutputsAlterTable(tableName, null, desc, desc.getType(), false);
+ String newDatabaseName = target.getDb() != null ? target.getDb() : table.getDbName(); // extract new database name from new table name, if not specified, then src dbname is used
+ Database newDatabase = getDatabase(newDatabaseName);
+ outputs.add(new WriteEntity(newDatabase, WriteEntity.WriteType.DDL_SHARED));
+ Table newTable = new Table(target.getDb(), target.getTable());
Review Comment:
why not just call
````
DDLUtils.addDbAndTableToOutputs(
getDatabase(newDbName), newTable
table.getTableType(), table.isTemporary(), table.getParameters(),
outputs)`
````
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/misc/rename/AbstractAlterTableRenameAnalyzer.java:
##########
@@ -49,6 +55,14 @@ protected void analyzeCommand(TableName tableName, Map<String, String> partition
setAcidDdlDesc(desc);
}
addInputsOutputsAlterTable(tableName, null, desc, desc.getType(), false);
+ String newDatabaseName = target.getDb() != null ? target.getDb() : table.getDbName(); // extract new database name from new table name, if not specified, then src dbname is used
+ Database newDatabase = getDatabase(newDatabaseName);
+ outputs.add(new WriteEntity(newDatabase, WriteEntity.WriteType.DDL_SHARED));
+ Table newTable = new Table(target.getDb(), target.getTable());
Review Comment:
should be
````
Table newTable = new Table(newDbName, target.getTable());
````
--
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
[GitHub] [hive] github-actions[bot] commented on pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #3247:
URL: https://github.com/apache/hive/pull/3247#issuecomment-1155659686
# @check-spelling-bot Report
### :red_circle: Please review
See the [files](3247/files/) view or the [action log](https://github.com/apache/hive/actions/runs/2497810415) for details.
#### Unrecognized words (3)
api
esri
wkid
<details><summary>Previously acknowledged words that are now absent
</summary>aarry timestamplocal yyyy </details>
<details><summary>Available dictionaries could cover words not in the dictionary</summary>
[cspell:cpp/cpp.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/cpp/cpp.txt) (104293) covers 81 of them
[cspell:django/django.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/django/django.txt) (2342) covers 14 of them
[cspell:golang/go.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/golang/go.txt) (7745) covers 12 of them
[cspell:java/java.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/java/java.txt) (33524) covers 11 of them
[cspell:filetypes/filetypes.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/filetypes/filetypes.txt) (337) covers 10 of them
[cspell:aws/aws.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/aws/aws.txt) (1485) covers 10 of them
[cspell:css/css.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/css/css.txt) (993) covers 9 of them
[cspell:rust/rust.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/rust/rust.txt) (112) covers 8 of them
[cspell:npm/npm.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/npm/npm.txt) (671) covers 8 of them
[cspell:html/html.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/html/html.txt) (542) covers 8 of them
[cspell:scala/scala.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/scala/scala.txt) (2752) covers 7 of them
[cspell:php/php.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/php/php.txt) (9785) covers 6 of them
[cspell:fullstack/fullstack.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/fullstack/fullstack.txt) (181) covers 5 of them
[cspell:csharp/csharp.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/csharp/csharp.txt) (123) covers 5 of them
[cspell:python/python.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/python/python.txt) (364) covers 3 of them
[cspell:lua/lua.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/lua/lua.txt) (391) covers 3 of them
[cspell:dotnet/dotnet.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/dotnet/dotnet.txt) (9824) covers 2 of them
[cspell:ruby/ruby.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/ruby/ruby.txt) (354) covers 1 of them
[cspell:bash/bash-words.txt](https://raw.githubusercontent.com/check-spelling/cspell-dicts/HEAD/dictionaries/bash/bash-words.txt) (22) covers 1 of them
Consider adding them using:
```
with:
extra_dictionaries:
cspell:cpp/cpp.txt
cspell:django/django.txt
cspell:golang/go.txt
cspell:java/java.txt
cspell:filetypes/filetypes.txt
cspell:aws/aws.txt
cspell:css/css.txt
cspell:rust/rust.txt
cspell:npm/npm.txt
cspell:html/html.txt
cspell:scala/scala.txt
cspell:php/php.txt
cspell:fullstack/fullstack.txt
cspell:csharp/csharp.txt
cspell:python/python.txt
cspell:lua/lua.txt
cspell:dotnet/dotnet.txt
cspell:ruby/ruby.txt
cspell:bash/bash-words.txt
```
To stop checking additional dictionaries, add:
```
with:
check_extra_dictionaries: ''
```
</details>
<details><summary>To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words),
run the following commands</summary>
... in a clone of the [git@github.com:saihemanth-cloudera/hive.git](https://github.com/saihemanth-cloudera/hive.git) repository
on the `hive-26055` branch:
```
update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}
comment_json=$(mktemp)
curl -L -s -S \
-H "Content-Type: application/json" \
"COMMENT_URL" > "$comment_json"
comment_body=$(mktemp)
jq -r ".body // empty" "$comment_json" > $comment_body
rm $comment_json
patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
patch_add=$(perl -e '$/=undef; $_=<>; if (m{Unrecognized words[^<]*</summary>\n*```\n*([^<]*)```\n*</details>$}m) { print "$1" } elsif (m{Unrecognized words[^<]*\n\n((?:\w.*\n)+)\n}m) { print "$1" };' < "$comment_body")
update_files
rm $comment_body
git add -u
```
</details>
<!-- See https://github.com/check-spelling/check-spelling/wiki/Configuration-Examples%3A-advice --> <!-- markdownlint-disable MD033 MD041 -->
<details><summary>If the flagged items do not appear to be text</summary>
If items relate to a ...
* well-formed pattern.
If you can write a [pattern](https://github.com/check-spelling/check-spelling/wiki/Configuration-Examples:-patterns) that would match it,
try adding it to the `patterns.txt` file.
Patterns are Perl 5 Regular Expressions - you can [test](
https://www.regexplanet.com/advanced/perl/) yours before committing to verify it will match your lines.
Note that patterns can't match multiline strings.
* binary file.
Please add a file path to the `excludes.txt` file matching the containing file.
File paths are Perl 5 Regular Expressions - you can [test](
https://www.regexplanet.com/advanced/perl/) yours before committing to verify it will match your files.
`^` refers to the file's path from the root of the repository, so `^README\.md$` would exclude [README.md](
../tree/HEAD/README.md) (on whichever branch you're using).
</details>
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r909671917
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java:
##########
@@ -211,8 +211,15 @@ static RequiredPrivileges getPrivilegesFromMetaStore(IMetaStoreClient metastoreC
RequiredPrivileges privs = getRequiredPrivsFromThrift(thrifPrivs);
// add owner privilege if user is owner of the object
- if (isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
- privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
+ try {
+ if (metastoreClient.tableExists(hivePrivObject.getDbname(), hivePrivObject.getObjectName()) &&
Review Comment:
- didn't you return 'true' in isOwner() when table doesn't exist in your initial patch and set the owner privilege?
--
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
[GitHub] [hive] kgyrtkirk commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
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: 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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r905448252
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/misc/rename/AbstractAlterTableRenameAnalyzer.java:
##########
@@ -49,6 +55,11 @@ protected void analyzeCommand(TableName tableName, Map<String, String> partition
setAcidDdlDesc(desc);
}
addInputsOutputsAlterTable(tableName, null, desc, desc.getType(), false);
+ String newDatabaseName = target.getDb() != null ? target.getDb() : table.getDbName(); // extract new database name from new table name, if not specified, then src dbname is used
+ Database newDatabase = getDatabase(newDatabaseName);
+ outputs.add(new WriteEntity(newDatabase, WriteEntity.WriteType.DDL_NO_LOCK));
+ Table newTable = new Table(target.getDb(), target.getTable());
+ outputs.add(new WriteEntity(newTable, WriteEntity.WriteType.DDL_EXCLUSIVE));
Review Comment:
Why the change in locking? Why do we need an `EXCLUSIVE` lock on not yet existing table + `NO_LOCK` on the DB level?
Note that `addInputsOutputsAlterTable(...)` already adds an `EXCLUSIVE` or `EXCL_WRITE`, depending on a config, to the original table.
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r906413548
##########
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java:
##########
@@ -3754,7 +3754,7 @@ private void testRenameTable(boolean blocking) throws Exception {
}
driver2.lockAndRespond();
locks = getLocks();
- Assert.assertEquals("Unexpected lock count", 1, locks.size());
+ Assert.assertEquals("Unexpected lock count", 2, locks.size());
Review Comment:
please add the corresponding check in a test
--
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
[GitHub] [hive] saihemanth-cloudera commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
saihemanth-cloudera commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r875334956
##########
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 tried to implement essential logic in that method here which is organizing input and output object lists. But I now added back the method and added the output list in a way that we would need it in Apache Ranger.
##########
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:
Ack
##########
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:
Yeah, this change is an intended one. This class is only used in rename_table_location.q file and it is used to verify the table object and its location. Previously, output and input had the old table objects for authorization. Now, we would like to have the old table in the input list and the new database and table object in the output list.
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r917626359
##########
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:
in that case, I don't see any meaning in that check at all, should we remove it?
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r906420448
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java:
##########
@@ -211,8 +211,15 @@ static RequiredPrivileges getPrivilegesFromMetaStore(IMetaStoreClient metastoreC
RequiredPrivileges privs = getRequiredPrivsFromThrift(thrifPrivs);
// add owner privilege if user is owner of the object
- if (isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
- privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
+ try {
+ if (metastoreClient.tableExists(hivePrivObject.getDbname(), hivePrivObject.getObjectName()) &&
Review Comment:
Wait, don't you need to add the privilege when the table DOESN'T exist? Also won't it hide the problem with the non-existing table for other operations besides RENAME??
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r906413400
##########
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java:
##########
@@ -3754,7 +3754,7 @@ private void testRenameTable(boolean blocking) throws Exception {
}
driver2.lockAndRespond();
locks = getLocks();
- Assert.assertEquals("Unexpected lock count", 1, locks.size());
+ Assert.assertEquals("Unexpected lock count", 2, locks.size());
Review Comment:
please add the corresponding check in a test
--
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
[GitHub] [hive] saihemanth-cloudera commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
saihemanth-cloudera commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r909621284
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/misc/rename/AbstractAlterTableRenameAnalyzer.java:
##########
@@ -49,6 +55,11 @@ protected void analyzeCommand(TableName tableName, Map<String, String> partition
setAcidDdlDesc(desc);
}
addInputsOutputsAlterTable(tableName, null, desc, desc.getType(), false);
+ String newDatabaseName = target.getDb() != null ? target.getDb() : table.getDbName(); // extract new database name from new table name, if not specified, then src dbname is used
+ Database newDatabase = getDatabase(newDatabaseName);
+ outputs.add(new WriteEntity(newDatabase, WriteEntity.WriteType.DDL_SHARED));
+ Table newTable = new Table(target.getDb(), target.getTable());
Review Comment:
That's a good point. I'll add them.
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r906412900
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java:
##########
@@ -211,8 +211,15 @@ static RequiredPrivileges getPrivilegesFromMetaStore(IMetaStoreClient metastoreC
RequiredPrivileges privs = getRequiredPrivsFromThrift(thrifPrivs);
// add owner privilege if user is owner of the object
- if (isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
- privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
+ try {
+ if (metastoreClient.tableExists(hivePrivObject.getDbname(), hivePrivObject.getObjectName()) &&
Review Comment:
PrivilageObjectType here might not be a TABLE_OR_VIEW.
````
try {
if (HivePrivilegeObjectType.TABLE_OR_VIEW != hivePrivObject.getType() ||
metastoreClient.tableExists(hivePrivObject.getDbname(), hivePrivObject.getObjectName())) {
if (isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
}
}
} catch (TException e) {
throwGetPrivErr(e, hivePrivObject, userName);
}
````
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r906412900
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java:
##########
@@ -211,8 +211,15 @@ static RequiredPrivileges getPrivilegesFromMetaStore(IMetaStoreClient metastoreC
RequiredPrivileges privs = getRequiredPrivsFromThrift(thrifPrivs);
// add owner privilege if user is owner of the object
- if (isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
- privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
+ try {
+ if (metastoreClient.tableExists(hivePrivObject.getDbname(), hivePrivObject.getObjectName()) &&
Review Comment:
PrivilageObjectType here might not be a TABLE_OR_VIEW.
````
try {
if (HivePrivilegeObjectType.TABLE_OR_VIEW != hivePrivObject.getType() ||
metastoreClient.tableExists(hivePrivObject.getDbname(), hivePrivObject.getObjectName())) {
if (isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
}
}
} catch (TException e) {
throwGetPrivErr(e, hivePrivObject, userName);
}
````
--
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
[GitHub] [hive] saihemanth-cloudera commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
saihemanth-cloudera commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r918614645
##########
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:
I have updated the patch to verify the output table i.e..., the old table which is the desired condition.
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r911106279
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java:
##########
@@ -211,8 +213,24 @@ static RequiredPrivileges getPrivilegesFromMetaStore(IMetaStoreClient metastoreC
RequiredPrivileges privs = getRequiredPrivsFromThrift(thrifPrivs);
// add owner privilege if user is owner of the object
- if (isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
- privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
+ try {
Review Comment:
maybe just:
````
try {
if (isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
}
} catch (HiveAuthzPluginException ex) {
if (ex.getCause() instanceof NoSuchObjectException && ignoreUnknown) {
privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
} else {
throw ex;
}
}
````
--
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
[GitHub] [hive] saihemanth-cloudera commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
saihemanth-cloudera commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r910353586
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java:
##########
@@ -211,8 +211,15 @@ static RequiredPrivileges getPrivilegesFromMetaStore(IMetaStoreClient metastoreC
RequiredPrivileges privs = getRequiredPrivsFromThrift(thrifPrivs);
// add owner privilege if user is owner of the object
- if (isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
- privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
+ try {
+ if (metastoreClient.tableExists(hivePrivObject.getDbname(), hivePrivObject.getObjectName()) &&
Review Comment:
ack
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r911066995
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/misc/rename/AbstractAlterTableRenameAnalyzer.java:
##########
@@ -49,6 +55,11 @@ protected void analyzeCommand(TableName tableName, Map<String, String> partition
setAcidDdlDesc(desc);
}
addInputsOutputsAlterTable(tableName, null, desc, desc.getType(), false);
+ String newDatabaseName = target.getDb() != null ? target.getDb() : table.getDbName(); // extract new database name from new table name, if not specified, then src dbname is used
+ Database newDatabase = getDatabase(newDatabaseName);
+ outputs.add(new WriteEntity(newDatabase, WriteEntity.WriteType.DDL_SHARED));
+ Table newTable = new Table(target.getDb(), target.getTable());
Review Comment:
why not just call
````
DDLUtils.addDbAndTableToOutputs(
newDatabase, newTable
table.getTableType(), table.isTemporary(), table.getParameters(),
outputs)`
````
--
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
[GitHub] [hive] saihemanth-cloudera commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
saihemanth-cloudera commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r911343855
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java:
##########
@@ -211,8 +213,24 @@ static RequiredPrivileges getPrivilegesFromMetaStore(IMetaStoreClient metastoreC
RequiredPrivileges privs = getRequiredPrivsFromThrift(thrifPrivs);
// add owner privilege if user is owner of the object
- if (isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
- privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
+ try {
Review Comment:
ack
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r906420448
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java:
##########
@@ -211,8 +211,15 @@ static RequiredPrivileges getPrivilegesFromMetaStore(IMetaStoreClient metastoreC
RequiredPrivileges privs = getRequiredPrivsFromThrift(thrifPrivs);
// add owner privilege if user is owner of the object
- if (isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
- privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
+ try {
+ if (metastoreClient.tableExists(hivePrivObject.getDbname(), hivePrivObject.getObjectName()) &&
Review Comment:
Wait, don't you need to add the privilege when the table DOESN'T exist?
Also won't it hide the problem with the non-existing table for other operations besides RENAME? Can we check somehow that we are within the rename operation context or pass through var like `ignoreUnknown`
--
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
[GitHub] [hive] saihemanth-cloudera commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
saihemanth-cloudera commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r906362961
##########
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java:
##########
@@ -3754,7 +3754,7 @@ private void testRenameTable(boolean blocking) throws Exception {
}
driver2.lockAndRespond();
locks = getLocks();
- Assert.assertEquals("Unexpected lock count", 1, locks.size());
+ Assert.assertEquals("Unexpected lock count", 2, locks.size());
Review Comment:
one lock on the source table and another one on the destination.
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r905219046
##########
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java:
##########
@@ -3754,7 +3754,7 @@ private void testRenameTable(boolean blocking) throws Exception {
}
driver2.lockAndRespond();
locks = getLocks();
- Assert.assertEquals("Unexpected lock count", 1, locks.size());
+ Assert.assertEquals("Unexpected lock count", 2, locks.size());
Review Comment:
what's the other lock we are expecting here, should we check this in a test?
--
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
[GitHub] [hive] saihemanth-cloudera commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
saihemanth-cloudera commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r912033165
##########
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:
I changed it for the following reason:
The output list now has a source table(old table name) and destination table(renamed table name). This class is used in the alter_table_location.q to check the schema of renamed table file location.
renamed table's getDataLocation() would be null at this stage because we'll be setting the table's location in the MetaStoreDefaultTransformer class in the HMS during execution. HMS will set the new location with the same schema. That's the reason why I have changed from outputs to inputs.
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r909671917
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java:
##########
@@ -211,8 +211,15 @@ static RequiredPrivileges getPrivilegesFromMetaStore(IMetaStoreClient metastoreC
RequiredPrivileges privs = getRequiredPrivsFromThrift(thrifPrivs);
// add owner privilege if user is owner of the object
- if (isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
- privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
+ try {
+ if (metastoreClient.tableExists(hivePrivObject.getDbname(), hivePrivObject.getObjectName()) &&
Review Comment:
- didn't you return 'true' in isOwner() when table doesn't exist in your initial patch and set the owner privilege?
- note, that hivePrivObject.getType() might not be always a TABLE_OR_VIEW, could be DB as well.
--
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
[GitHub] [hive] saihemanth-cloudera commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
saihemanth-cloudera commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r909602203
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java:
##########
@@ -211,8 +211,15 @@ static RequiredPrivileges getPrivilegesFromMetaStore(IMetaStoreClient metastoreC
RequiredPrivileges privs = getRequiredPrivsFromThrift(thrifPrivs);
// add owner privilege if user is owner of the object
- if (isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
- privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
+ try {
+ if (metastoreClient.tableExists(hivePrivObject.getDbname(), hivePrivObject.getObjectName()) &&
+ isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
+ privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
+ }
+ } catch (MetaException e) {
Review Comment:
ack.
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r911074669
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLStdHiveAuthorizationValidator.java:
##########
@@ -80,15 +80,15 @@ public void checkPrivileges(HiveOperationType hiveOpType, List<HivePrivilegeObje
// check privileges on input and output objects
List<String> deniedMessages = new ArrayList<String>();
- checkPrivileges(hiveOpType, inputHObjs, metastoreClient, userName, IOType.INPUT, deniedMessages);
- checkPrivileges(hiveOpType, outputHObjs, metastoreClient, userName, IOType.OUTPUT, deniedMessages);
+ checkPrivileges(hiveOpType, inputHObjs, metastoreClient, userName, IOType.INPUT, deniedMessages, false);
+ checkPrivileges(hiveOpType, outputHObjs, metastoreClient, userName, IOType.OUTPUT, deniedMessages, hiveOpType == HiveOperationType.ALTERTABLE_RENAME ? true: false);
Review Comment:
no need for a ternary operator here, just:
````
hiveOpType == HiveOperationType.ALTERTABLE_RENAME
````
--
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
[GitHub] [hive] saihemanth-cloudera commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
saihemanth-cloudera commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r906363926
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/misc/rename/AbstractAlterTableRenameAnalyzer.java:
##########
@@ -49,6 +55,11 @@ protected void analyzeCommand(TableName tableName, Map<String, String> partition
setAcidDdlDesc(desc);
}
addInputsOutputsAlterTable(tableName, null, desc, desc.getType(), false);
+ String newDatabaseName = target.getDb() != null ? target.getDb() : table.getDbName(); // extract new database name from new table name, if not specified, then src dbname is used
+ Database newDatabase = getDatabase(newDatabaseName);
+ outputs.add(new WriteEntity(newDatabase, WriteEntity.WriteType.DDL_NO_LOCK));
+ Table newTable = new Table(target.getDb(), target.getTable());
+ outputs.add(new WriteEntity(newTable, WriteEntity.WriteType.DDL_EXCLUSIVE));
Review Comment:
addInputsOutputsAlterTable(...) only adds lock on source table. we need to add locks on the destination as well. I have updated the patch to include a shared lock on the database and no lock on the newly created table object.
--
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
[GitHub] [hive] nrg4878 commented on pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
nrg4878 commented on PR #3247:
URL: https://github.com/apache/hive/pull/3247#issuecomment-1184846114
Thank you for the review @deniskuzZ Much appreciated.
--
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
[GitHub] [hive] kgyrtkirk commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
kgyrtkirk commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r879312194
##########
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:
doing `outputs.clear()` right after `addInputsOutputsAlterTable` is pretty strange
could you please put this code into that method - isn't there where it supposed to live?
##########
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:
it would be better to place this check at the call site-s to this method - so that the explanation will not be needed at all
##########
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:
Why we don't need an exclusive lock during a rename?
* what if we are reading data from a table in some dag
* in the meantime someone renames the table
with `SHARED_READ` the table will be renamed instantly - which will cause the dag to fail; or not?
--
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
[GitHub] [hive] saihemanth-cloudera commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
saihemanth-cloudera commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r880692701
##########
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:
Yeah, I tried to do it but it is becoming somewhat hacky because there are multiple places that I would have to instantiate the hivemetastoreclient object to see if the table exists. So I think this would be a good place to call this as add this at one place.
--
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
[GitHub] [hive] saihemanth-cloudera commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
saihemanth-cloudera commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r880680410
##########
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:
Yeah, I had to do this here. Reason: When a user executes Alter table foo rename to otherdatabase.foo2; we never send the otherdatabase and new table name (foo2) objects for authorization. We need to check whether the end-user has privileges to create a table in otherdatabase. So I think this special logic has to be in AbstractTableRenameAnalyzer instead of AbstractTableAnalyzer.
--
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
[GitHub] [hive] saihemanth-cloudera commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
saihemanth-cloudera commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r912038639
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/misc/rename/AbstractAlterTableRenameAnalyzer.java:
##########
@@ -49,6 +55,11 @@ protected void analyzeCommand(TableName tableName, Map<String, String> partition
setAcidDdlDesc(desc);
}
addInputsOutputsAlterTable(tableName, null, desc, desc.getType(), false);
+ String newDatabaseName = target.getDb() != null ? target.getDb() : table.getDbName(); // extract new database name from new table name, if not specified, then src dbname is used
+ Database newDatabase = getDatabase(newDatabaseName);
+ outputs.add(new WriteEntity(newDatabase, WriteEntity.WriteType.DDL_SHARED));
+ Table newTable = new Table(target.getDb(), target.getTable());
Review Comment:
ack
--
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
[GitHub] [hive] nrg4878 merged pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
nrg4878 merged PR #3247:
URL: https://github.com/apache/hive/pull/3247
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r906412900
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java:
##########
@@ -211,8 +211,15 @@ static RequiredPrivileges getPrivilegesFromMetaStore(IMetaStoreClient metastoreC
RequiredPrivileges privs = getRequiredPrivsFromThrift(thrifPrivs);
// add owner privilege if user is owner of the object
- if (isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
- privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
+ try {
+ if (metastoreClient.tableExists(hivePrivObject.getDbname(), hivePrivObject.getObjectName()) &&
Review Comment:
PrivilageObjectType here might not be a TABLE_OR_VIEW.
````
try {
if (HivePrivilegeObjectType.TABLE_OR_VIEW != hivePrivObject.getType() ||
!metastoreClient.tableExists(hivePrivObject.getDbname(), hivePrivObject.getObjectName())) {
if (isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
}
}
} catch (TException e) {
throwGetPrivErr(e, hivePrivObject, userName);
}
````
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r906420448
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java:
##########
@@ -211,8 +211,15 @@ static RequiredPrivileges getPrivilegesFromMetaStore(IMetaStoreClient metastoreC
RequiredPrivileges privs = getRequiredPrivsFromThrift(thrifPrivs);
// add owner privilege if user is owner of the object
- if (isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
- privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
+ try {
+ if (metastoreClient.tableExists(hivePrivObject.getDbname(), hivePrivObject.getObjectName()) &&
Review Comment:
Wait, don't you need to add the privilege when the table DOESN'T exist?
Also won't it hide the problem with the non-existing table for other operations besides RENAME? Can we check somehow that we are within the rename operation context?
--
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
[GitHub] [hive] kgyrtkirk commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r906404873
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java:
##########
@@ -211,8 +211,15 @@ static RequiredPrivileges getPrivilegesFromMetaStore(IMetaStoreClient metastoreC
RequiredPrivileges privs = getRequiredPrivsFromThrift(thrifPrivs);
// add owner privilege if user is owner of the object
- if (isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
- privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
+ try {
+ if (metastoreClient.tableExists(hivePrivObject.getDbname(), hivePrivObject.getObjectName()) &&
+ isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
+ privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
+ }
+ } catch (MetaException e) {
Review Comment:
You could pipe multiple exceptions, i.e
````
catch (MetaException | TException e) {
throwGetPrivErr(e, hivePrivObject, userName);
}
--
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
[GitHub] [hive] saihemanth-cloudera commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
saihemanth-cloudera commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r909613946
##########
ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/sqlstd/SQLAuthorizationUtils.java:
##########
@@ -211,8 +211,15 @@ static RequiredPrivileges getPrivilegesFromMetaStore(IMetaStoreClient metastoreC
RequiredPrivileges privs = getRequiredPrivsFromThrift(thrifPrivs);
// add owner privilege if user is owner of the object
- if (isOwner(metastoreClient, userName, curRoles, hivePrivObject)) {
- privs.addPrivilege(SQLPrivTypeGrant.OWNER_PRIV);
+ try {
+ if (metastoreClient.tableExists(hivePrivObject.getDbname(), hivePrivObject.getObjectName()) &&
Review Comment:
I don't think we would need any privileges when the table doesn't exist. That's the reason why we are moving this piece of code out of isOwner() method. We don't have any information on what type of operation is this at that point.
Also, this check is about whether we need to assign owner privileges to the current user or not, when the table itself doesn't exist there is no point in assigning ownership privileges irrespective of operation.
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r911952989
##########
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:
Why is this change? switch from outputs -> inputs?
--
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
[GitHub] [hive] deniskuzZ commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
deniskuzZ commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r911069160
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/misc/rename/AbstractAlterTableRenameAnalyzer.java:
##########
@@ -49,6 +55,14 @@ protected void analyzeCommand(TableName tableName, Map<String, String> partition
setAcidDdlDesc(desc);
}
addInputsOutputsAlterTable(tableName, null, desc, desc.getType(), false);
+ String newDatabaseName = target.getDb() != null ? target.getDb() : table.getDbName(); // extract new database name from new table name, if not specified, then src dbname is used
+ Database newDatabase = getDatabase(newDatabaseName);
+ outputs.add(new WriteEntity(newDatabase, WriteEntity.WriteType.DDL_SHARED));
+ Table newTable = new Table(target.getDb(), target.getTable());
Review Comment:
should be
Table newTable = new Table(newDatabaseName, target.getTable());
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/misc/rename/AbstractAlterTableRenameAnalyzer.java:
##########
@@ -49,6 +55,14 @@ protected void analyzeCommand(TableName tableName, Map<String, String> partition
setAcidDdlDesc(desc);
}
addInputsOutputsAlterTable(tableName, null, desc, desc.getType(), false);
+ String newDatabaseName = target.getDb() != null ? target.getDb() : table.getDbName(); // extract new database name from new table name, if not specified, then src dbname is used
+ Database newDatabase = getDatabase(newDatabaseName);
+ outputs.add(new WriteEntity(newDatabase, WriteEntity.WriteType.DDL_SHARED));
+ Table newTable = new Table(target.getDb(), target.getTable());
Review Comment:
should be
````
Table newTable = new Table(newDatabaseName, target.getTable());
````
--
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
[GitHub] [hive] saihemanth-cloudera commented on a diff in pull request #3247: HIVE-26055: Added the right HivePrivilegesObjects for Alter table ren…
Posted by GitBox <gi...@apache.org>.
saihemanth-cloudera commented on code in PR #3247:
URL: https://github.com/apache/hive/pull/3247#discussion_r909800089
##########
ql/src/test/org/apache/hadoop/hive/ql/lockmgr/TestDbTxnManager2.java:
##########
@@ -3754,7 +3754,7 @@ private void testRenameTable(boolean blocking) throws Exception {
}
driver2.lockAndRespond();
locks = getLocks();
- Assert.assertEquals("Unexpected lock count", 1, locks.size());
+ Assert.assertEquals("Unexpected lock count", 2, locks.size());
Review Comment:
ack
--
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