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: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: gitbox-unsubscr...@hive.apache.org For additional commands, e-mail: gitbox-h...@hive.apache.org