nrg4878 commented on a change in pull request #2389:
URL: https://github.com/apache/hive/pull/2389#discussion_r651208929
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/ddl/database/create/CreateDatabaseAnalyzer.java
##########
@@ -78,10 +78,17 @@ public void analyzeInternal(ASTNode root) throws
SemanticException {
ASTNode nextNode = (ASTNode) root.getChild(i);
connectorName = ((ASTNode)nextNode).getChild(0).getText();
outputs.add(toWriteEntity(connectorName));
- if (managedLocationUri != null) {
- outputs.remove(toWriteEntity(managedLocationUri));
- managedLocationUri = null;
+
+ // HIVE-2436: Reject location and managed locations in DDL for REMOTE
databases.
Review comment:
Is this jira reference correct? seems like old jira.
##########
File path: ql/src/test/queries/clientnegative/remoteDB_locationUri_reject.q
##########
@@ -0,0 +1,12 @@
+-- CREATE IF NOT EXISTS already
+CREATE CONNECTOR IF NOT EXISTS mysql_test
+TYPE 'mysql'
+URL 'jdbc:mysql://nightly1.apache.org:3306/hive1'
+COMMENT 'test connector'
+WITH DCPROPERTIES (
+"hive.sql.dbcp.username"="hive1",
+"hive.sql.dbcp.password"="hive1");
+SHOW CONNECTORS;
+
+-- reject location and managedlocation config in remote database
+create REMOTE database mysql_rej location '/tmp/rej1.db' managedlocation
'/tmp/rej2.db' using mysql_test with
DBPROPERTIES("connector.remoteDbName"="hive1");
Review comment:
can we split this into 2 (or even 3) scenarios? we want to make sure if
fails in all scenarios.
create REMOTE db with location
create REMOTE db with managedlocation
create REMOTE db with location and managedlocation
##########
File path:
ql/src/java/org/apache/hadoop/hive/ql/ddl/database/create/CreateDatabaseAnalyzer.java
##########
@@ -78,10 +78,17 @@ public void analyzeInternal(ASTNode root) throws
SemanticException {
ASTNode nextNode = (ASTNode) root.getChild(i);
connectorName = ((ASTNode)nextNode).getChild(0).getText();
outputs.add(toWriteEntity(connectorName));
- if (managedLocationUri != null) {
- outputs.remove(toWriteEntity(managedLocationUri));
- managedLocationUri = null;
+
+ // HIVE-2436: Reject location and managed locations in DDL for REMOTE
databases.
+ if (locationUri != null || managedLocationUri != null ) {
+ if (locationUri == null) {
+ outputs.remove(toWriteEntity(locationUri));
Review comment:
so in HMS schema, "locationUri" is a required column. So I think
HiveServer2 may have to send a location for a database, regardless of its type.
The HiveMetastore might also use a default one. So a couple of things here
1) Seems like line 85 and 87 might result in a NullPointerException.
Shouldn't we remove it when the value is NOT null?
2) given we are throwing an exception from this condition, do we even have
to do outputs.remove() ? We are failing the operation anyway.
##########
File path: parser/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g
##########
@@ -1156,7 +1156,7 @@ createDatabaseStatement
dbManagedLocation?
dbConnectorName?
(KW_WITH KW_DBPROPERTIES dbprops=dbProperties)?
- -> {$remote != null}? ^(TOK_CREATEDATABASE $name ifNotExists?
databaseComment? $dbprops? dbConnectorName?)
+ -> {$remote != null}? ^(TOK_CREATEDATABASE $name ifNotExists? dbLocation?
dbManagedLocation? databaseComment? $dbprops? dbConnectorName?)
Review comment:
Seems like the grammer shouldn't support the accepting a location and
managedlocation when the dbtype is REMOTE. I am bit surprised that the antlr
parser wasn't throwing an exception when location or managedlocation was
specified.
Could you please see why we were not seeing an exception with the old
grammer?
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org
For additional commands, e-mail: gitbox-h...@hive.apache.org
