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/09 01:55:46 UTC

[GitHub] [hive] dantongdong opened a new pull request #2371: HIVE-25213: Implement List getTables() for existing connectors
dantongdong opened a new pull request #2371:
URL: https://github.com/apache/hive/pull/2371


   [HIVE-25213](https://issues.apache.org/jira/browse/HIVE-25213): Implement List<Table> getTables() for existing connectors.


-- 
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 #2371: HIVE-25213: Implement List

getTables() for existing connectorsPosted by GitBox <gi...@apache.org>.
dantongdong commented on a change in pull request #2371:
URL: https://github.com/apache/hive/pull/2371#discussion_r651962841



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -3799,10 +3799,33 @@ public Table get_table_core(GetTableRequest getTableRequest) throws MetaExceptio
   @Override
   public GetTablesResult get_table_objects_by_name_req(GetTablesRequest req) throws TException {
     String catName = req.isSetCatName() ? req.getCatName() : getDefaultCatalog(conf);
+    if (isDatabaseRemote(req.getDbName())) {
+      return new GetTablesResult(getRemoteTableObjectsInternal(req.getDbName(), req.getTblNames(), req.getTablesPattern()));
+    }
     return new GetTablesResult(getTableObjectsInternal(catName, req.getDbName(),
         req.getTblNames(), req.getCapabilities(), req.getProjectionSpec(), req.getTablesPattern()));
   }
 
+  private String tableNames2regex(List<String> tableNames) {
+    return "/^(" + String.join("|", tableNames) + ")$/";

Review comment:
       Good catch. Will look into that. As a side note, it seems that at least mysql does not like ".*" to be passed in as a regex getTable API call and will throw an error. I have to manually changed the regex to null, is it normal?




-- 
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 #2371: HIVE-25213: Implement List

getTables() for existing connectorsPosted by GitBox <gi...@apache.org>.
dantongdong commented on a change in pull request #2371:
URL: https://github.com/apache/hive/pull/2371#discussion_r651997059



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/dataconnector/jdbc/AbstractJDBCConnectorProvider.java
##########
@@ -129,7 +129,30 @@ protected Connection getConnection() {
    * @throws MetaException To indicate any failures with executing this API
    * @param regex
    */
-  @Override public abstract List<Table> getTables(String regex) throws MetaException;
+  @Override public List<Table> getTables(String regex) throws MetaException {
+    ResultSet rs = null;
+    try {
+      rs = fetchTablesViaDBMetaData(regex);
+      if (rs != null) {
+        List<Table> tables = new ArrayList<Table>();
+        while(rs.next()) {
+          tables.add(getTable(rs.getString(3)));

Review comment:
       If I am looking at the right [place](https://javadoc.scijava.org/Java6/java/sql/DatabaseMetaData.html). It does not specify its behavior neither. But I think it makes more sense to just leave out the one with column exception and continue returning the rest instead of stopping? Because we want the behavior to be as close as getTables(), we can just filter out the ones that has corrupted columns and return as many as we can.




-- 
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 #2371: HIVE-25213: Implement List

getTables() for existing connectorsPosted by GitBox <gi...@apache.org>.
dantongdong commented on a change in pull request #2371:
URL: https://github.com/apache/hive/pull/2371#discussion_r653975366



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -3799,10 +3799,33 @@ public Table get_table_core(GetTableRequest getTableRequest) throws MetaExceptio
   @Override
   public GetTablesResult get_table_objects_by_name_req(GetTablesRequest req) throws TException {
     String catName = req.isSetCatName() ? req.getCatName() : getDefaultCatalog(conf);
+    if (isDatabaseRemote(req.getDbName())) {
+      return new GetTablesResult(getRemoteTableObjectsInternal(req.getDbName(), req.getTblNames(), req.getTablesPattern()));
+    }
     return new GetTablesResult(getTableObjectsInternal(catName, req.getDbName(),
         req.getTblNames(), req.getCapabilities(), req.getProjectionSpec(), req.getTablesPattern()));
   }
 
+  private String tableNames2regex(List<String> tableNames) {
+    return "/^(" + String.join("|", tableNames) + ")$/";

Review comment:
       After some digging around. It seems that tableNamePattern in DatabaseMetaData.getTables() is a bit different from regular regex we are using. In order to search for all tables, the regex should be "%" or null instead of ".*". And we cannot use tableNamePattern to match multiple tableNames([reference](url)). I have created another helper function to get around this issue, and it is included in the latest commit!

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -3799,10 +3799,33 @@ public Table get_table_core(GetTableRequest getTableRequest) throws MetaExceptio
   @Override
   public GetTablesResult get_table_objects_by_name_req(GetTablesRequest req) throws TException {
     String catName = req.isSetCatName() ? req.getCatName() : getDefaultCatalog(conf);
+    if (isDatabaseRemote(req.getDbName())) {
+      return new GetTablesResult(getRemoteTableObjectsInternal(req.getDbName(), req.getTblNames(), req.getTablesPattern()));
+    }
     return new GetTablesResult(getTableObjectsInternal(catName, req.getDbName(),
         req.getTblNames(), req.getCapabilities(), req.getProjectionSpec(), req.getTablesPattern()));
   }
 
+  private String tableNames2regex(List<String> tableNames) {
+    return "/^(" + String.join("|", tableNames) + ")$/";

Review comment:
       After some digging around. It seems that tableNamePattern in DatabaseMetaData.getTables() is a bit different from regular regex we are using. In order to search for all tables, the regex should be "%" or null instead of ".*". And we cannot use tableNamePattern to match multiple tableNames([reference](https://stackoverflow.com/questions/65707608/java-sql-pattern-to-find-some-tables-in-my-database)). I have created another helper function to get around this issue, and it is included in the latest commit!




-- 
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 #2371: HIVE-25213: Implement List

getTables() for existing connectorsPosted by GitBox <gi...@apache.org>.
nrg4878 commented on a change in pull request #2371:
URL: https://github.com/apache/hive/pull/2371#discussion_r651415437



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/dataconnector/jdbc/AbstractJDBCConnectorProvider.java
##########
@@ -129,7 +129,30 @@ protected Connection getConnection() {
    * @throws MetaException To indicate any failures with executing this API
    * @param regex
    */
-  @Override public abstract List<Table> getTables(String regex) throws MetaException;
+  @Override public List<Table> getTables(String regex) throws MetaException {
+    ResultSet rs = null;
+    try {
+      rs = fetchTablesViaDBMetaData(regex);
+      if (rs != null) {
+        List<Table> tables = new ArrayList<Table>();
+        while(rs.next()) {
+          tables.add(getTable(rs.getString(3)));

Review comment:
       so getTable() can throw an exception when retrieving column info for each table. What should happen when there is an exception on a certain table? Currently, we are ending the iteration and throwing an exception. I dont know what the right behavior should be, whether to return the remaining table or to throw an exception. Do you know what the JDBC spec/javadocs for getTables() says/does for this?

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -3799,10 +3799,33 @@ public Table get_table_core(GetTableRequest getTableRequest) throws MetaExceptio
   @Override
   public GetTablesResult get_table_objects_by_name_req(GetTablesRequest req) throws TException {
     String catName = req.isSetCatName() ? req.getCatName() : getDefaultCatalog(conf);
+    if (isDatabaseRemote(req.getDbName())) {
+      return new GetTablesResult(getRemoteTableObjectsInternal(req.getDbName(), req.getTblNames(), req.getTablesPattern()));
+    }
     return new GetTablesResult(getTableObjectsInternal(catName, req.getDbName(),
         req.getTblNames(), req.getCapabilities(), req.getProjectionSpec(), req.getTablesPattern()));
   }
 
+  private String tableNames2regex(List<String> tableNames) {
+    return "/^(" + String.join("|", tableNames) + ")$/";

Review comment:
       Are there any nuances around how different RDBMS database drivers process this regex? Do we know that this is truely a regex or do certain drivers expect other things?

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/dataconnector/jdbc/AbstractJDBCConnectorProvider.java
##########
@@ -208,7 +232,7 @@ protected Connection getConnection() {
       return table;
     } catch (Exception e) {
       LOG.warn("Exception retrieving remote table " + scoped_db + "." + tableName + " via data connector "
-          + connector.getName());
+              + connector.getName());

Review comment:
       nit: please remove extra spaces

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/dataconnector/jdbc/AbstractJDBCConnectorProvider.java
##########
@@ -176,7 +199,7 @@ protected Connection getConnection() {
     Table table = null;
     try {
       // rs = fetchTableMetadata(tableName);

Review comment:
       nit: I know this is old code. But can you please delete the commented line as well?

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/dataconnector/jdbc/AbstractJDBCConnectorProvider.java
##########
@@ -28,7 +28,7 @@
 
   // duplicate constants from Constants.java to avoid a dependency on hive-common
   public static final String JDBC_HIVE_STORAGE_HANDLER_ID =
-      "org.apache.hive.storage.jdbc.JdbcStorageHandler";
+          "org.apache.hive.storage.jdbc.JdbcStorageHandler";

Review comment:
       nit: please remove extra spaces

##########
File path: itests/qtest/target/db_for_connectortest.db/service.properties
##########
@@ -0,0 +1,23 @@
+#/private/tmp/db_for_connectortest.db

Review comment:
       All the derby db files included above are from the $root/itests/qtest/target directory. These will get deleted when you run “mvn clean”
   instead these files need to reside in $root/data/resources (I think we discussed this location) and then a change to itests/pom.xml to copy from there to itests/qtest/target directory when a build is run in “itests/” directory.

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/dataconnector/jdbc/AbstractJDBCConnectorProvider.java
##########
@@ -53,7 +53,7 @@
   private static final String JDBC_OUTPUTFORMAT_CLASS = "org.apache.hive.storage.jdbc.JdbcOutputFormat".intern();
 
   String type = null; // MYSQL, POSTGRES, ORACLE, DERBY, MSSQL, DB2 etc.
-  String driverClassName = null;
+  //String driverClassName = null;

Review comment:
       I thought this was set and used by sub-classes ? no? if it is not, instead of commenting, please delete. 




-- 
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 closed pull request #2371: HIVE-25213: Implement List

getTables() for existing connectorsPosted by GitBox <gi...@apache.org>.
dantongdong closed pull request #2371:
URL: https://github.com/apache/hive/pull/2371


   


-- 
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 #2371: HIVE-25213: Implement List

getTables() for existing connectorsPosted by GitBox <gi...@apache.org>.
dantongdong commented on a change in pull request #2371:
URL: https://github.com/apache/hive/pull/2371#discussion_r651955572



##########
File path: itests/qtest/target/db_for_connectortest.db/service.properties
##########
@@ -0,0 +1,23 @@
+#/private/tmp/db_for_connectortest.db

Review comment:
       Yes, we have discussed this over the meeting but I forgot to add it. Will change 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.

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 #2371: HIVE-25213: Implement List

getTables() for existing connectorsPosted by GitBox <gi...@apache.org>.
nrg4878 commented on a change in pull request #2371:
URL: https://github.com/apache/hive/pull/2371#discussion_r657601470



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -3799,10 +3799,37 @@ public Table get_table_core(GetTableRequest getTableRequest) throws MetaExceptio
   @Override
   public GetTablesResult get_table_objects_by_name_req(GetTablesRequest req) throws TException {
     String catName = req.isSetCatName() ? req.getCatName() : getDefaultCatalog(conf);
+    if (isDatabaseRemote(req.getDbName())) {
+      return new GetTablesResult(getRemoteTableObjectsInternal(req.getDbName(), req.getTblNames(), req.getTablesPattern()));
+    }
     return new GetTablesResult(getTableObjectsInternal(catName, req.getDbName(),
         req.getTblNames(), req.getCapabilities(), req.getProjectionSpec(), req.getTablesPattern()));
   }
 
+  private List<Table> filterTablesByName(List<Table> tables, List<String> tableNames) {
+    List<Table> filteredTables = new ArrayList<>();
+    for (Table table : tables) {
+      if (tableNames.contains(table.getTableName())) {
+        filteredTables.add(table);
+      }
+    }
+    return filteredTables;
+  }
+
+  private List<Table> getRemoteTableObjectsInternal(String dbname, List<String> tableNames, String pattern) throws MetaException {
+    String[] parsedDbName = parseDbName(dbname, conf);
+    try {
+      Database db = get_database_core(parsedDbName[CAT_NAME], parsedDbName[DB_NAME]);
+      List<Table> tables = DataConnectorProviderFactory.getDataConnectorProvider(db).getTables(null);
+      if (tableNames != null) {
+        tables = filterTablesByName(tables, tableNames);
+      }
+      return FilterUtils.filterTablesIfEnabled(isServerFilterEnabled, filterHook, tables);
+    } catch (Exception e) {
+      return new ArrayList<Table>();

Review comment:
       Could you log this exception as a WARN message with the message

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/dataconnector/jdbc/AbstractJDBCConnectorProvider.java
##########
@@ -192,6 +215,7 @@ protected Connection getConnection() {
       }
 
       table = buildTableFromColsList(tableName, cols);
+      table.setDbName(scoped_db);

Review comment:
       I think this might be wrong. 
   So scoped_db is the name of the remote database thats being mapped. and table.setDbName() is meant to be the name of the hive database for this table. So for example,
   MySQL has a database called "employees" with 2 tables (profiles and salaries).
   Using a MySQL dataconnector, we map this remote db to a hive database called "companyx_employees" like so
   create remote database companyx_employees using mysql_conn DBPROPERTIES("connector.remoteDbName"="employees");
   show tables in this db should show the 2 tables "profiiles" and "salaries"
   But to reference them in hive queries, you will have to use "companyx_employees.profiles" and NOT "employees.profiiles" as employees database in hive does not exist.

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/dataconnector/jdbc/AbstractJDBCConnectorProvider.java
##########
@@ -53,7 +53,6 @@
   private static final String JDBC_OUTPUTFORMAT_CLASS = "org.apache.hive.storage.jdbc.JdbcOutputFormat".intern();
 
   String type = null; // MYSQL, POSTGRES, ORACLE, DERBY, MSSQL, DB2 etc.
-  String driverClassName = null;

Review comment:
       This should not be removed as it is used by the providers to set the driver classname. Please add it back




-- 
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 #2371: HIVE-25213: Implement List

getTables() for existing connectorsPosted by GitBox <gi...@apache.org>.
dantongdong commented on a change in pull request #2371:
URL: https://github.com/apache/hive/pull/2371#discussion_r658196105



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/dataconnector/jdbc/AbstractJDBCConnectorProvider.java
##########
@@ -53,7 +53,6 @@
   private static final String JDBC_OUTPUTFORMAT_CLASS = "org.apache.hive.storage.jdbc.JdbcOutputFormat".intern();
 
   String type = null; // MYSQL, POSTGRES, ORACLE, DERBY, MSSQL, DB2 etc.
-  String driverClassName = null;

Review comment:
       AbstractJDBCConnectorProvider Class extends AbstractDataConnectorProvider(super) Class. driverClassName is already a field in the super class. Having driverClassName=null here will overwrite the passed in driverClassName by specific provider, as AbstractJDBCConnectorProvider constructor is calling its super class constructor. This line is the root cause of "Could not find a provider for remote database" error. Removing the line will stop the overwritten problem. All the provider can set driverClassName via calling AbstractDataConnectorProvider's constructor, which is what AbstractJDBCConnectorProvider is doing for all the providers.




-- 
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 #2371: HIVE-25213: Implement List

getTables() for existing connectorsPosted by GitBox <gi...@apache.org>.
nrg4878 commented on pull request #2371:
URL: https://github.com/apache/hive/pull/2371#issuecomment-874996414


   Fix has been committed to master. Please close the PR.


-- 
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 #2371: HIVE-25213: Implement List

getTables() for existing connectorsPosted by GitBox <gi...@apache.org>.
dantongdong commented on a change in pull request #2371:
URL: https://github.com/apache/hive/pull/2371#discussion_r658196705



##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/dataconnector/jdbc/AbstractJDBCConnectorProvider.java
##########
@@ -192,6 +215,7 @@ protected Connection getConnection() {
       }
 
       table = buildTableFromColsList(tableName, cols);
+      table.setDbName(scoped_db);

Review comment:
       Good catch. 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