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 2021/06/14 07:59:13 UTC
[GitHub] [hive] dantongdong opened a new pull request #2389: Hive-24970: Reject location and managed locations in DDL for REMOTE databases.
dantongdong opened a new pull request #2389:
URL: https://github.com/apache/hive/pull/2389
explicitly reject location and managed locations in DDL for REMOTE databases instead of silently ignore.
--
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:
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 a change in pull request #2389: HIVE-24970: Reject location and managed locations in DDL for REMOTE databases.
Posted by GitBox <gi...@apache.org>.
nrg4878 commented on a change in pull request #2389:
URL: https://github.com/apache/hive/pull/2389#discussion_r652049945
##########
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:
Let me ponder on the first part.
create database localdb using connector xyz
If this does not throw an exception, we should change this behavior to force users to be explicit and specify REMOTE.
--
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:
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 a change in pull request #2389: HIVE-24970: Reject location and managed locations in DDL for REMOTE databases.
Posted by GitBox <gi...@apache.org>.
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:
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 #2389: HIVE-24970: Reject location and managed locations in DDL for REMOTE databases.
Posted by GitBox <gi...@apache.org>.
nrg4878 commented on pull request #2389:
URL: https://github.com/apache/hive/pull/2389#issuecomment-874972263
Fix has been committed. Please close the PR. Thank you
--
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] dantongdong closed pull request #2389: HIVE-24970: Reject location and managed locations in DDL for REMOTE databases.
Posted by GitBox <gi...@apache.org>.
dantongdong closed pull request #2389:
URL: https://github.com/apache/hive/pull/2389
--
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] dantongdong commented on a change in pull request #2389: HIVE-24970: Reject location and managed locations in DDL for REMOTE databases.
Posted by GitBox <gi...@apache.org>.
dantongdong commented on a change in pull request #2389:
URL: https://github.com/apache/hive/pull/2389#discussion_r651998832
##########
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:
It was not the correct reference indeed. Will 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.
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 a change in pull request #2389: HIVE-24970: Reject location and managed locations in DDL for REMOTE databases.
Posted by GitBox <gi...@apache.org>.
nrg4878 commented on a change in pull request #2389:
URL: https://github.com/apache/hive/pull/2389#discussion_r656226944
##########
File path: parser/src/java/org/apache/hadoop/hive/ql/parse/HiveParser.g
##########
@@ -1042,6 +1042,7 @@ ddlStatement
@init { pushMsg("ddl statement", state); }
@after { popMsg(state); }
: createDatabaseStatement
+ | createRemoteDatabaseStatement
Review comment:
instead of adding a new top level definition called "createRemoteDatabaseStatement", we should add an alternate syntax definition within "createDatabaseStatement". Please refer to createTableStatement syntax.
--
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:
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] dantongdong commented on a change in pull request #2389: HIVE-24970: Reject location and managed locations in DDL for REMOTE databases.
Posted by GitBox <gi...@apache.org>.
dantongdong commented on a change in pull request #2389:
URL: https://github.com/apache/hive/pull/2389#discussion_r652014451
##########
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:
It doesn't throw an exception because this line is not in charge of the actual parsing and compiling. This line is in charge of generating ASTNode based on the pared and compiled result. Not including dbLocation and dbManagedLocation in this line will only make it ignore dbLocation and dbManaged when generating ASTNode. The actual parsing and compiling is done in the lines above it(line1131-1139). All the create database statement shares the same parse and compile code, which means that all create database(including remote)statement will parse and compile dbLocation and dbManagedLocation.
Likewise, if you do something like: "create database localdb using connector xyz" will not throw an error neither. I can also fix this if it is not desired behavior.
--
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:
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] dantongdong commented on a change in pull request #2389: HIVE-24970: Reject location and managed locations in DDL for REMOTE databases.
Posted by GitBox <gi...@apache.org>.
dantongdong commented on a change in pull request #2389:
URL: https://github.com/apache/hive/pull/2389#discussion_r651999419
##########
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:
Will 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.
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