HIVE-29072:Cleanup IMetaStoreClient#getTables APIs

[ramitg254]

What changes were proposed in this pull request?

cleanup of unnecessary wrapper methods in IMetastoreClient

Why are the changes needed?

There are different variations of same methods which are not required as per HIVE-23971

Does this PR introduce any user-facing change?

No

How was this patch tested?

via local build and unit tests

[deniskuzZ]

why not keep simplified API that doesn't require tableType?

[ramitg254]

due to this part of code

if (tableType == TableType.MANAGED_TABLE || tableType == TableType.EXTERNAL_TABLE) {
        for (Map.Entry<String, org.apache.hadoop.hive.ql.metadata.Table> tableData : tables.entrySet()) {
          matcher.reset(tableData.getKey());
          if (matcher.matches()) {
            if (tableData.getValue().getTableType() == tableType) {
              // If tableType is the same that we are requesting,
              // add table the the list
              combinedTableNames.add(tableData.getKey());
            } else {
              // If tableType is not the same that we are requesting,
              // remove it in case it was added before, as temp table
              // overrides original table
              combinedTableNames.remove(tableData.getKey());
            }
          }
        }
      }

in SessionHiveMetaStoreClient.java

and occurences like

List<String> viewNames =
             clients.run(client -> client.getTables(database, "*", TableType.VIRTUAL_VIEW));

List<String> materializedViews =
          getTables(req.getCatalogName(), req.getName(), ".*", TableType.MATERIALIZED_VIEW);

which require tableType

[ramitg254]

and these simplified api are just wrappers around the current methods which was asked to be removed in (https://issues.apache.org/jira/browse/HIVE-23971#:~:text=Also%20affects-,getTables%20API,-.%20Bumping%20this%20up)
and all of these parameters can be dealt by https://github.com/apache/hive/pull/5947/files#:~:text=static%20GetTablesRequest-,convertToGetTablesRequest,-(String%20catName other than tableType cause it is not part of GetTablesRequest object

[deniskuzZ]

isn't it more convenient to use msc.getTables(dbName, "*") -> msc.getTables(dbName, "*", null) than hardcoding nulls?
btw, you can just drop/modify the public API, you have to deprecate it first

[ramitg254]

yeah your point is also correct but I referenced this earlier pr regarding cleanup: https://github.com/apache/hive/pull/3072/files
and their also:

client.getPartitionsByNames(catName, dbName, tableName,
                       Collections.singletonList(partName), true, ENGINE);

was replaced with:

GetPartitionsByNamesRequest req = convertToGetPartitionsByNamesRequest(database, tableName,
          Collections.singletonList(partName), true, ENGINE, null, null);
      Partition partition = client.getPartitionsByNames(req).getPartitions().get(0);

with two extra null values
so what I understood from that is despite having a separate method it is better to have an extra param which can deal with both null as well as non null args

[ramitg254]

If this seems right from maintainability perspective then I'll deprecate the other methods otherwise I'll close this pr if current state of getTables is more convenient

[sonarqubecloud]

Quality Gate passed

Issues
16 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[deniskuzZ]

@ramitg254, we can't just go ahead and change the client API. If we have an agreement that this could be dropped , we should first deprecate it
cc @dengzhhu653

[deniskuzZ]

@ramitg254, there are number of deprecated getTable methods, maybe it's time to drop them. It seems that Hive-26537 skipped the client interface cleanup.

  @Deprecated
  Table getTable(String dbName, String tableName);

  @Deprecated
  Table getTable(String catName, String dbName, String tableName);

  @Deprecated
  Table getTable(String dbName, String tableName, 
      boolean getColumnStats, String engine);
  
  @Deprecated
  Table getTable(String catName, String dbName, String tableName, 
      String validWriteIdList);

  @Deprecated
  Table getTable(String catName, String dbName, String tableName,
      String validWriteIdList, boolean getColumnStats, String engine) 

cc @saihemanth-cloudera, @dengzhhu653

[saihemanth-cloudera]

@deniskuzZ - I have deprecated those APIs in a major release Hive 4.0. So I thought it may be better to remove the deprecated APIs in the next major release Hive 5.0. Do you believe that removing these deprecated APIs in a minor release like 4.2.0 is a good idea?

[deniskuzZ]

@deniskuzZ - I have deprecated those APIs in a major release Hive 4.0. So I thought it may be better to remove the deprecated APIs in the next major release Hive 5.0. Do you believe that removing these deprecated APIs in a minor release like 4.2.0 is a good idea?

@saihemanth-cloudera, but you dropped it from the thrift API, see apache/iceberg#12878

[saihemanth-cloudera]

@saihemanth-cloudera, but you dropped it from the thrift API, see apache/iceberg#12878
@deniskuzZ - I have removed the get_table() APIs that were already deprecated in HIVE 3.x release. I have modified the data connector related APIs because they were not in the Hive 3.x release.

[deniskuzZ]

@saihemanth-cloudera, but you dropped it from the thrift API, see apache/iceberg#12878
@deniskuzZ - I have removed the get_table() APIs that were already deprecated in HIVE 3.x release. I have modified the data connector related APIs because they were not in the Hive 3.x release.

that's fine, but why keep the client implementing that API (also marked as deprecated) ?
I think it make sense to expose the HMS API using Swagger or a similar tool to improve documentation