zabetak commented on a change in pull request #2734:
URL: https://github.com/apache/hive/pull/2734#discussion_r732724951



##########
File path: ql/src/java/org/apache/hadoop/hive/ql/parse/CalcitePlanner.java
##########
@@ -3011,9 +3011,14 @@ private RelNode genTableLogicalPlan(String tableAlias, 
QB qb) throws SemanticExc
             final String user = 
tabMetaData.getProperty(Constants.JDBC_USERNAME);
             String pswd = tabMetaData.getProperty(Constants.JDBC_PASSWORD);
             if (pswd == null) {
-              String keystore = 
tabMetaData.getProperty(Constants.JDBC_KEYSTORE);
-              String key = tabMetaData.getProperty(Constants.JDBC_KEY);
-              pswd = Utilities.getPasswdFromKeystore(keystore, key);
+              if(!(tabMetaData.getProperty(Constants.JDBC_PASSWORD_URI) == 
null)) {
+                  pswd = 
Utilities.getPasswdFromUri(tabMetaData.getProperty(Constants.JDBC_PASSWORD_URI));
+              }
+              else {
+                String keystore = 
tabMetaData.getProperty(Constants.JDBC_KEYSTORE);
+                String key = tabMetaData.getProperty(Constants.JDBC_KEY);
+                pswd = Utilities.getPasswdFromKeystore(keystore, key);
+              }

Review comment:
       Maybe it makes sense to extract the code to a separate method in 
Utilities. For instance:
   `final String pswd = Utilities.getJdbcPasswordFromTableMetadata(table)`
   `final String pswd = 
Utilities.getJdbcPasswordFromProperties(table.getParameters())`
   
   Also it may be slightly more readable if there is no nesting in the if 
statements so that is clear in what priority the properties are checked. For 
instance:
   ```
   If (tabMetaData.getProperty(Constants.JDBC_PASSWORD) != null) {
   } else if ((tabMetaData.getProperty(Constants.JDBC_PASSWORD_URI) != null) {
   } else if (tabMetaData.getProperty(Constants.JDBC_KEYSTORE) != null) {
   } else {
     LOG.warn(...)
   }
   ```
   I think that at the moment we don't forbid somebody to set multiple password 
related properties at the same time when creating the JDBC table. Do you think 
it is worth throwing an error when the DDL statement contains multiple props 
set? If yes then we can log a JIRA and possibly tackle it in the future.




-- 
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]

Reply via email to