zabetak commented on code in PR #4304:
URL: https://github.com/apache/hive/pull/4304#discussion_r1257851021
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/drop/DropTableAnalyzer.java:
##########
@@ -46,9 +48,16 @@ public DropTableAnalyzer(QueryState queryState) throws
SemanticException {
@Override
public void analyzeInternal(ASTNode root) throws SemanticException {
- String tableName = getUnescapedName((ASTNode) root.getChild(0));
+ TableName qualTabName = getQualifiedTableName((ASTNode) root.getChild(0));
+ String tableName = qualTabName.getNotEmptyDbTable();
boolean ifExists = (root.getFirstChildWithType(HiveParser.TOK_IFEXISTS) !=
null);
boolean throwException = !ifExists && !HiveConf.getBoolVar(conf,
ConfVars.DROP_IGNORES_NON_EXISTENT);
+ //Authorize database for drop table command
+ // Skip db object if database doesn't exist
+ Database database = getDatabase(qualTabName.getDb(),false);
Review Comment:
I think that the second parameter of the `getDatabase` call should be
`throwException` and not hardcoded to `false`. This is the feeling that I get
by looking other places in the code where this method is used and also by the
presence of `hive.exec.drop.ignorenonexistent` property.
##########
ql/src/java/org/apache/hadoop/hive/ql/ddl/table/drop/DropTableAnalyzer.java:
##########
@@ -46,9 +48,16 @@ public DropTableAnalyzer(QueryState queryState) throws
SemanticException {
@Override
public void analyzeInternal(ASTNode root) throws SemanticException {
- String tableName = getUnescapedName((ASTNode) root.getChild(0));
+ TableName qualTabName = getQualifiedTableName((ASTNode) root.getChild(0));
+ String tableName = qualTabName.getNotEmptyDbTable();
boolean ifExists = (root.getFirstChildWithType(HiveParser.TOK_IFEXISTS) !=
null);
boolean throwException = !ifExists && !HiveConf.getBoolVar(conf,
ConfVars.DROP_IGNORES_NON_EXISTENT);
+ //Authorize database for drop table command
+ // Skip db object if database doesn't exist
+ Database database = getDatabase(qualTabName.getDb(),false);
Review Comment:
Moreover, since there are no test failures due to this part then it means
that we don't have sufficient coverage and we should add a few along with this
change.
##########
ql/src/test/org/apache/hadoop/hive/ql/TestTxnExIm.java:
##########
@@ -62,11 +62,12 @@ public void testExportCustomDb() throws Exception {
private void testExport(boolean useDefualtDb) throws Exception {
int[][] rows1 = {{1, 2}, {3, 4}};
final String tblName = useDefualtDb ? "T" : "foo.T";
- runStatementOnDriver("drop table if exists " + tblName);
- runStatementOnDriver("drop table if exists TImport ");
if(!useDefualtDb) {
runStatementOnDriver("create database foo");
}
+ runStatementOnDriver("drop table if exists " + tblName);
+ runStatementOnDriver("drop table if exists TImport ");
+
Review Comment:
Why do need to change the order of operations? If things fail with the
existing order then it means that we are introducing a breaking change; better
not do that unless we have to. Same comment applies to other places where order
or commands changed in the tests.
##########
ql/src/test/queries/clientpositive/drop_with_concurrency.q:
##########
@@ -5,4 +5,4 @@ set
hive.lock.manager=org.apache.hadoop.hive.ql.lockmgr.EmbeddedLockManager;
drop table if exists drop_with_concurrency_1;
create table drop_with_concurrency_1 (c1 int);
-drop table drop_with_concurrency_1;
+drop table drop_with_concurrency_1;
Review Comment:
nit:
https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline
##########
itests/hive-unit/src/test/java/org/apache/hadoop/hive/ql/TestDatabaseTableDefault.java:
##########
@@ -140,7 +140,7 @@ private void dropTables() throws Exception {
public void tearDown() throws Exception {
try {
if (d != null) {
- dropTables();
+ //dropTables();
Review Comment:
Why is this change needed?
If it is really needed then the code should be removed; it is generally best
to avoid leaving code commented out.
--
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]
