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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
