hive-gitbox mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [hive] kishendas commented on a change in pull request #2211: HIVE-23931: Send ValidWriteIdList and tableId to get_*_constraints HMS APIs
Date Wed, 05 May 2021 00:48:10 GMT

kishendas commented on a change in pull request #2211:
URL: https://github.com/apache/hive/pull/2211#discussion_r626190188



##########
File path: standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift
##########
@@ -740,7 +752,9 @@ struct CheckConstraintsResponse {
 struct AllTableConstraintsRequest {
   1: required string dbName,
   2: required string tblName,
-  3: required string catName
+  3: required string catName,
+  4: optional string validWriteIdList,
+  5: optional i64 tableId=-1 // table id

Review comment:
       You can probably remove // table id comment.

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -9479,43 +9479,38 @@ void updateMetrics() throws MetaException {
 
   @Override
   public PrimaryKeysResponse get_primary_keys(PrimaryKeysRequest request) throws TException
{
-    String catName = request.isSetCatName() ? request.getCatName() : getDefaultCatalog(conf);
-    String db_name = request.getDb_name();
-    String tbl_name = request.getTbl_name();
-    startTableFunction("get_primary_keys", catName, db_name, tbl_name);
+    if (!request.isSetCatName())

Review comment:
       Please use flower brackets, even for single statement if blocks. 

##########
File path: ql/src/java/org/apache/hadoop/hive/ql/lockmgr/DbTxnManager.java
##########
@@ -777,13 +777,14 @@ public ValidTxnList getValidTxns(List<TxnType> excludeTxnTypes)
throws LockExcep
   public ValidTxnWriteIdList getValidWriteIds(List<String> tableList,
                                               String validTxnList) throws LockException {
     assert isTxnOpen();
-    assert validTxnList != null && !validTxnList.isEmpty();
-    try {
-      return TxnCommonUtils.createValidTxnWriteIdList(
-          txnId, getMS().getValidWriteIds(tableList, validTxnList));
-    } catch (TException e) {
-      throw new LockException(ErrorMsg.METASTORE_COMMUNICATION_FAILED.getMsg(), e);
+    if (!StringUtils.isEmpty(validTxnList)) {

Review comment:
       Why do we need this change ? Please don't mix new code/feature with refactoring, as
it makes it harder to review. 

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
##########
@@ -11978,8 +12027,26 @@ private String getPrimaryKeyConstraintName(String catName, String
dbName, String
    * @throws MetaException
    */
   @Override
+  @Deprecated
   public SQLAllTableConstraints getAllTableConstraints(String catName, String dbName, String
tblName)
       throws MetaException,NoSuchObjectException {
+    AllTableConstraintsRequest request = new AllTableConstraintsRequest(dbName,tblName,catName);
+    return getAllTableConstraints(request);
+  }
+
+  /**
+   * Api to fetch all constraints at once
+   * @param tableConstraintsRequest request object
+   * @return all table constraints
+   * @throws MetaException
+   * @throws NoSuchObjectException
+   */
+  @Override
+  public SQLAllTableConstraints getAllTableConstraints(AllTableConstraintsRequest tableConstraintsRequest)
+      throws MetaException, NoSuchObjectException {
+    String catName = tableConstraintsRequest.getCatName();

Review comment:
       Directly use them from the request object. 

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
##########
@@ -11565,11 +11579,21 @@ private String getPrimaryKeyConstraintName(String catName, String
dbName, String
    }
 
   @Override
-  public List<SQLForeignKey> getForeignKeys(String catName, String parent_db_name,
-    String parent_tbl_name, String foreign_db_name, String foreign_tbl_name) throws MetaException
{
+  public List<SQLForeignKey> getForeignKeys(String catName, String parent_db_name,
String parent_tbl_name,
+      String foreign_db_name, String foreign_tbl_name) throws MetaException {
+    ForeignKeysRequest request =
+        new ForeignKeysRequest(parent_db_name, parent_tbl_name, foreign_db_name, foreign_tbl_name);

Review comment:
       You can overload the constructor also. 

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java
##########
@@ -1572,9 +1579,20 @@ void getFileMetadataByExpr(List<Long> fileIds, FileMetadataExprType
type, byte[]
    * @return list of primary key columns or an empty list if the table does not have a primary
key
    * @throws MetaException error accessing the RDBMS
    */
+  @Deprecated
   List<SQLPrimaryKey> getPrimaryKeys(String catName, String db_name, String tbl_name)
       throws MetaException;
 
+  /**
+   * Get the primary associated with a table.  Strangely enough each SQLPrimaryKey is actually
a

Review comment:
       "Get the primary key/s associated with a table." Please do not write personal opinion
in the comments, unless they are purely technical. You can simply say "SQLPrimaryKey represents
a single primary key column. Since a table can have one or more primary keys ( in case of
composite primary key ), this method returns List< SQLPrimaryKey>".

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
##########
@@ -2584,57 +2584,89 @@ long getPartsFound() {
   }
 
   @Override
+  @Deprecated
   public List<SQLPrimaryKey> getPrimaryKeys(String catName, String dbName, String tblName)
throws MetaException {
-    catName = StringUtils.normalizeIdentifier(catName);
-    dbName = StringUtils.normalizeIdentifier(dbName);
-    tblName = StringUtils.normalizeIdentifier(tblName);
+    PrimaryKeysRequest request = new PrimaryKeysRequest(dbName, tblName);
+    request.setCatName(catName);
+    return getPrimaryKeys(request);
+  }
+
+  @Override
+  public List<SQLPrimaryKey> getPrimaryKeys(PrimaryKeysRequest request) throws MetaException
{
+    String catName = StringUtils.normalizeIdentifier(request.getCatName());
+    String dbName = StringUtils.normalizeIdentifier(request.getDb_name());
+    String tblName = StringUtils.normalizeIdentifier(request.getTbl_name());
     if (shouldGetConstraintFromRawStore(catName, dbName, tblName)) {
-      return rawStore.getPrimaryKeys(catName, dbName, tblName);
+      return rawStore.getPrimaryKeys(request);
     }
     return sharedCache.listCachedPrimaryKeys(catName, dbName, tblName);
   }
 
   @Override
+  @Deprecated
   public List<SQLForeignKey> getForeignKeys(String catName, String parentDbName, String
parentTblName,
       String foreignDbName, String foreignTblName) throws MetaException {
+    ForeignKeysRequest request = new ForeignKeysRequest(parentDbName, parentTblName, foreignDbName,
foreignTblName);
+    request.setCatName(catName);
+    return getForeignKeys(request);
+  }
+
+  @Override
+  public List<SQLForeignKey> getForeignKeys(ForeignKeysRequest request) throws MetaException
{
     // Get correct ForeignDBName and TableName
-    if (StringUtils.isEmpty(foreignDbName) || StringUtils.isEmpty(foreignTblName) || StringUtils.isEmpty(parentDbName)
-        || StringUtils.isEmpty(parentTblName)) {
-      return rawStore.getForeignKeys(catName, parentDbName, parentTblName, foreignDbName,
foreignTblName);
+    if (StringUtils.isEmpty(request.getParent_db_name()) || StringUtils.isEmpty(request.getParent_tbl_name())
+        || StringUtils.isEmpty(request.getForeign_db_name()) || StringUtils.isEmpty(request.getForeign_tbl_name()))
{
+      return rawStore.getForeignKeys(request);
     }
 
-    catName = StringUtils.normalizeIdentifier(catName);
-    foreignDbName = StringUtils.normalizeIdentifier(foreignDbName);
-    foreignTblName = StringUtils.normalizeIdentifier(foreignTblName);
-    parentDbName = StringUtils.isEmpty(parentDbName) ? "" : normalizeIdentifier(parentDbName);
-    parentTblName = StringUtils.isEmpty(parentTblName) ? "" : StringUtils.normalizeIdentifier(parentTblName);
+    String catName = StringUtils.normalizeIdentifier(request.getCatName());
+    String foreignDbName = StringUtils.normalizeIdentifier(request.getForeign_db_name());
+    String foreignTblName = StringUtils.normalizeIdentifier(request.getForeign_tbl_name());
+    String parentDbName =
+        StringUtils.isEmpty(request.getParent_db_name()) ? "" : normalizeIdentifier(request.getParent_db_name());
+    String parentTblName = StringUtils.isEmpty(request.getParent_tbl_name()) ? "" : StringUtils
+        .normalizeIdentifier(request.getParent_tbl_name());
 
     if (shouldGetConstraintFromRawStore(catName, foreignDbName, foreignTblName)) {
-      return rawStore.getForeignKeys(catName, parentDbName, parentTblName, foreignDbName,
foreignTblName);
+      return rawStore.getForeignKeys(request);
     }
     return sharedCache.listCachedForeignKeys(catName, foreignDbName, foreignTblName, parentDbName,
parentTblName);
   }
 
   @Override
+  @Deprecated
   public List<SQLUniqueConstraint> getUniqueConstraints(String catName, String dbName,
String tblName)
       throws MetaException {
-    catName = StringUtils.normalizeIdentifier(catName);
-    dbName = StringUtils.normalizeIdentifier(dbName);
-    tblName = StringUtils.normalizeIdentifier(tblName);
+    UniqueConstraintsRequest request = new UniqueConstraintsRequest(catName, dbName, tblName);
+    return getUniqueConstraints(request);
+  }
+
+  @Override
+  public List<SQLUniqueConstraint> getUniqueConstraints(UniqueConstraintsRequest request)
throws MetaException {
+    String catName = StringUtils.normalizeIdentifier(request.getCatName());
+    String dbName = StringUtils.normalizeIdentifier(request.getDb_name());
+    String tblName = StringUtils.normalizeIdentifier(request.getTbl_name());
     if (shouldGetConstraintFromRawStore(catName, dbName, tblName)) {
-      return rawStore.getUniqueConstraints(catName, dbName, tblName);
+      return rawStore.getUniqueConstraints(request);
     }
     return sharedCache.listCachedUniqueConstraint(catName, dbName, tblName);
   }
 
   @Override
+  @Deprecated
   public List<SQLNotNullConstraint> getNotNullConstraints(String catName, String dbName,
String tblName)
       throws MetaException {
-    catName = normalizeIdentifier(catName);
-    dbName = StringUtils.normalizeIdentifier(dbName);
-    tblName = StringUtils.normalizeIdentifier(tblName);
+    NotNullConstraintsRequest request = new NotNullConstraintsRequest(catName, dbName, tblName);
+    return getNotNullConstraints(request);
+  }
+
+  @Override
+  public List<SQLNotNullConstraint> getNotNullConstraints(NotNullConstraintsRequest
request) throws MetaException {
+    String catName = normalizeIdentifier(request.getCatName());

Review comment:
       StringUtils. is missing ! Is this code compiling ?

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -9532,93 +9527,85 @@ private void throwMetaException(Exception e) throws MetaException,
   }
 
   @Override
-  public UniqueConstraintsResponse get_unique_constraints(UniqueConstraintsRequest request)
-      throws TException {
-    String catName = request.isSetCatName() ? request.getCatName() : getDefaultCatalog(conf);
-    String db_name = request.getDb_name();
-    String tbl_name = request.getTbl_name();
-    startTableFunction("get_unique_constraints", catName, db_name, tbl_name);
+  public UniqueConstraintsResponse get_unique_constraints(UniqueConstraintsRequest request)
throws TException {
+    if (!request.isSetCatName())

Review comment:
       Please follow this instruction for rest of the changes. 

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java
##########
@@ -1588,10 +1606,22 @@ void getFileMetadataByExpr(List<Long> fileIds, FileMetadataExprType
type, byte[]
    * matches the arguments the results here will be all mixed together into a single list.
    * @throws MetaException error access the RDBMS.
    */
+  @Deprecated
   List<SQLForeignKey> getForeignKeys(String catName, String parent_db_name,
     String parent_tbl_name, String foreign_db_name, String foreign_tbl_name)
     throws MetaException;
 
+  /**
+   * Get the foreign keys for a table.  All foreign keys for a particular table can be fetched
by
+   * passing null for the last two arguments.

Review comment:
       What do you mean by this ? - "All foreign keys for a particular table can be fetched
by
       * passing null for the last two arguments." . You are only passing one argument here.


##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -9532,93 +9527,85 @@ private void throwMetaException(Exception e) throws MetaException,
   }
 
   @Override
-  public UniqueConstraintsResponse get_unique_constraints(UniqueConstraintsRequest request)
-      throws TException {
-    String catName = request.isSetCatName() ? request.getCatName() : getDefaultCatalog(conf);
-    String db_name = request.getDb_name();
-    String tbl_name = request.getTbl_name();
-    startTableFunction("get_unique_constraints", catName, db_name, tbl_name);
+  public UniqueConstraintsResponse get_unique_constraints(UniqueConstraintsRequest request)
throws TException {
+    if (!request.isSetCatName())

Review comment:
       Use flower brackets. 

##########
File path: ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
##########
@@ -5838,10 +5838,20 @@ public void dropConstraint(String dbName, String tableName, String
constraintNam
     }
   }
 
-  public SQLAllTableConstraints getTableConstraints(String dbName, String tblName)
+  public SQLAllTableConstraints getTableConstraints(String dbName, String tblName, long tableId)
       throws HiveException, NoSuchObjectException {
     try {
-      return getMSC().getAllTableConstraints(new AllTableConstraintsRequest(dbName, tblName,
getDefaultCatalog(conf)));
+
+      ValidWriteIdList validWriteIdList = null;

Review comment:
       Please move this logic from line 5845 - 5848 to a separate method as its getting repeated.


##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HMSHandler.java
##########
@@ -9479,43 +9479,38 @@ void updateMetrics() throws MetaException {
 
   @Override
   public PrimaryKeysResponse get_primary_keys(PrimaryKeysRequest request) throws TException
{
-    String catName = request.isSetCatName() ? request.getCatName() : getDefaultCatalog(conf);
-    String db_name = request.getDb_name();
-    String tbl_name = request.getTbl_name();
-    startTableFunction("get_primary_keys", catName, db_name, tbl_name);
+    if (!request.isSetCatName())
+      request.setCatName(getDefaultCatalog(conf));
+    startTableFunction("get_primary_keys", request.getCatName(), request.getDb_name(), request.getTbl_name());
     List<SQLPrimaryKey> ret = null;
     Exception ex = null;
     try {
-      ret = getMS().getPrimaryKeys(catName, db_name, tbl_name);
+      ret = getMS().getPrimaryKeys(request);
     } catch (Exception e) {
       ex = e;
       throwMetaException(e);
     } finally {
-      endFunction("get_primary_keys", ret != null, ex, tbl_name);
+      endFunction("get_primary_keys", ret != null, ex, request.getTbl_name());
     }
     return new PrimaryKeysResponse(ret);
   }
 
   @Override
   public ForeignKeysResponse get_foreign_keys(ForeignKeysRequest request) throws TException
{
-    String catName = request.isSetCatName() ? request.getCatName() : getDefaultCatalog(conf);
-    String parent_db_name = request.getParent_db_name();
-    String parent_tbl_name = request.getParent_tbl_name();
-    String foreign_db_name = request.getForeign_db_name();
-    String foreign_tbl_name = request.getForeign_tbl_name();
-    startFunction("get_foreign_keys", " : parentdb=" + parent_db_name +
-        " parenttbl=" + parent_tbl_name + " foreigndb=" + foreign_db_name +
-        " foreigntbl=" + foreign_tbl_name);
+    if (!request.isSetCatName())

Review comment:
       Use flower brackets. 

##########
File path: standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
##########
@@ -1230,7 +1230,7 @@ public void createTable(Table tbl, EnvironmentContext envContext) throws
Already
     boolean success = false;
     try {
       // Subclasses can override this step (for example, for temporary tables)
-      create_table_with_environment_context(tbl, envContext);
+       create_table_with_environment_context(tbl, envContext);

Review comment:
       Please do not include formatting only changes. 

##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ObjectStore.java
##########
@@ -11978,8 +12027,26 @@ private String getPrimaryKeyConstraintName(String catName, String
dbName, String
    * @throws MetaException
    */
   @Override
+  @Deprecated
   public SQLAllTableConstraints getAllTableConstraints(String catName, String dbName, String
tblName)
       throws MetaException,NoSuchObjectException {
+    AllTableConstraintsRequest request = new AllTableConstraintsRequest(dbName,tblName,catName);
+    return getAllTableConstraints(request);
+  }
+
+  /**
+   * Api to fetch all constraints at once
+   * @param tableConstraintsRequest request object
+   * @return all table constraints
+   * @throws MetaException
+   * @throws NoSuchObjectException
+   */
+  @Override
+  public SQLAllTableConstraints getAllTableConstraints(AllTableConstraintsRequest tableConstraintsRequest)
+      throws MetaException, NoSuchObjectException {
+    String catName = tableConstraintsRequest.getCatName();

Review comment:
       You can rename tableConstraintsRequest to request, so that the code is more readable.





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


Mime
View raw message