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 2020/10/23 11:28:39 UTC
[GitHub] [hive] sankarh commented on a change in pull request #1587: HIVE-24258: [CachedStore] Data miss match between cachedstore and rawstore
sankarh commented on a change in pull request #1587:
URL: https://github.com/apache/hive/pull/1587#discussion_r510806059
##########
File path: itests/hive-unit/src/test/java/org/apache/hadoop/hive/metastore/cache/TestCachedStoreUpdateUsingEvents.java
##########
@@ -419,18 +412,14 @@ public void testConstraintsForUpdateUsingEvents() throws Exception {
public void assertRawStoreAndCachedStoreConstraint(String catName, String dbName, String tblName)
throws MetaException, NoSuchObjectException {
SQLAllTableConstraints rawStoreConstraints = rawStore.getAllTableConstraints(catName, dbName, tblName);
- List<SQLPrimaryKey> primaryKeys = sharedCache.listCachedPrimaryKeys(catName, dbName, tblName);
- List<SQLNotNullConstraint> notNullConstraints = sharedCache.listCachedNotNullConstraints(catName, dbName, tblName);
- List<SQLUniqueConstraint> uniqueConstraints = sharedCache.listCachedUniqueConstraint(catName, dbName, tblName);
- List<SQLDefaultConstraint> defaultConstraints = sharedCache.listCachedDefaultConstraint(catName, dbName, tblName);
- List<SQLCheckConstraint> checkConstraints = sharedCache.listCachedCheckConstraint(catName, dbName, tblName);
- List<SQLForeignKey> foreignKeys = sharedCache.listCachedForeignKeys(catName, dbName, tblName, null, null);
- Assert.assertEquals(rawStoreConstraints.getPrimaryKeys(), primaryKeys);
- Assert.assertEquals(rawStoreConstraints.getNotNullConstraints(), notNullConstraints);
- Assert.assertEquals(rawStoreConstraints.getUniqueConstraints(), uniqueConstraints);
- Assert.assertEquals(rawStoreConstraints.getDefaultConstraints(), defaultConstraints);
- Assert.assertEquals(rawStoreConstraints.getCheckConstraints(), checkConstraints);
- Assert.assertEquals(rawStoreConstraints.getForeignKeys(), foreignKeys);
+ SQLAllTableConstraints cachedStoreConstraints = new SQLAllTableConstraints();
+ cachedStoreConstraints.setPrimaryKeys(sharedCache.listCachedPrimaryKeys(catName, dbName, tblName));
+ cachedStoreConstraints.setForeignKeys(sharedCache.listCachedForeignKeys(catName, dbName, tblName, null, null));
+ cachedStoreConstraints.setNotNullConstraints(sharedCache.listCachedNotNullConstraints(catName, dbName, tblName));
+ cachedStoreConstraints.setDefaultConstraints(sharedCache.listCachedDefaultConstraint(catName, dbName, tblName));
+ cachedStoreConstraints.setCheckConstraints(sharedCache.listCachedCheckConstraint(catName, dbName, tblName));
+ cachedStoreConstraints.setUniqueConstraints(sharedCache.listCachedUniqueConstraint(catName, dbName, tblName));
+ Assert.assertEquals(rawStoreConstraints,cachedStoreConstraints);
Review comment:
nit: Add space after ,
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
##########
@@ -2255,121 +2257,61 @@ private void create_table_core(final RawStore ms, final CreateTableRequest req)
tbl.putToParameters(hive_metastoreConstants.DDL_TIME, Long.toString(time));
}
- if (primaryKeys == null && foreignKeys == null
- && uniqueConstraints == null && notNullConstraints == null && defaultConstraints == null
- && checkConstraints == null) {
+ if (CollectionUtils.isEmpty(constraints.getPrimaryKeys()) && CollectionUtils.isEmpty(constraints.getForeignKeys())
+ && CollectionUtils.isEmpty(constraints.getUniqueConstraints())&& CollectionUtils.isEmpty(constraints.getNotNullConstraints())&& CollectionUtils.isEmpty(constraints.getDefaultConstraints())
+ && CollectionUtils.isEmpty(constraints.getCheckConstraints())) {
ms.createTable(tbl);
} else {
// Check that constraints have catalog name properly set first
- if (primaryKeys != null && !primaryKeys.isEmpty() && !primaryKeys.get(0).isSetCatName()) {
- for (SQLPrimaryKey pkcol : primaryKeys) pkcol.setCatName(tbl.getCatName());
+ if (CollectionUtils.isNotEmpty(constraints.getPrimaryKeys()) && !constraints.getPrimaryKeys().get(0).isSetCatName()) {
+ for (SQLPrimaryKey pkcol : constraints.getPrimaryKeys()) pkcol.setCatName(tbl.getCatName());
Review comment:
nit: Use for () {
--
}
even for single statement.
Or use this instead: constraints.getPrimaryKeys().forEach(pk -> pk.setCatName(tbl.getCatName()));
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
##########
@@ -2785,31 +2696,23 @@ public void add_not_null_constraint(AddNotNullConstraintRequest req)
@Override
public void add_default_constraint(AddDefaultConstraintRequest req)
throws MetaException, InvalidObjectException {
- List<SQLDefaultConstraint> defaultConstraintCols= req.getDefaultConstraintCols();
- String constraintName = (defaultConstraintCols != null && defaultConstraintCols.size() > 0) ?
- defaultConstraintCols.get(0).getDc_name() : "null";
+ List<SQLDefaultConstraint> defaultConstraints= req.getDefaultConstraintCols();
Review comment:
nit: Add space before =
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/cache/CachedStore.java
##########
@@ -2846,31 +2846,28 @@ public SQLAllTableConstraints getAllTableConstraints(String catName, String dbNa
return sqlAllTableConstraints;
}
- @Override public List<String> createTableWithConstraints(Table tbl, List<SQLPrimaryKey> primaryKeys,
- List<SQLForeignKey> foreignKeys, List<SQLUniqueConstraint> uniqueConstraints,
- List<SQLNotNullConstraint> notNullConstraints, List<SQLDefaultConstraint> defaultConstraints,
- List<SQLCheckConstraint> checkConstraints) throws InvalidObjectException, MetaException {
- List<String> constraintNames = rawStore
- .createTableWithConstraints(tbl, primaryKeys, foreignKeys, uniqueConstraints, notNullConstraints,
- defaultConstraints, checkConstraints);
+ @Override public SQLAllTableConstraints createTableWithConstraints(Table tbl, SQLAllTableConstraints constraints) throws InvalidObjectException, MetaException {
Review comment:
nit: Shall keep annotation and api declaration ins separate lines.
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/RawStore.java
##########
@@ -1499,20 +1499,11 @@ SQLAllTableConstraints getAllTableConstraints(String catName, String dbName, Str
/**
* Create a table with constraints
* @param tbl table definition
- * @param primaryKeys primary key definition, or null
- * @param foreignKeys foreign key definition, or null
- * @param uniqueConstraints unique constraints definition, or null
- * @param notNullConstraints not null constraints definition, or null
- * @param defaultConstraints default values definition, or null
* @return list of constraint names
Review comment:
nit: Update comments for change in return type for all apis.
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/cache/CacheUtils.java
##########
@@ -58,14 +59,11 @@ public static String buildDbKeyWithDelimiterSuffix(String catName, String dbName
*
*/
public static String buildPartitionCacheKey(List<String> partVals) {
- if (partVals == null || partVals.isEmpty()) {
- return "";
- }
- return String.join(delimit, partVals);
+ return CollectionUtils.isNotEmpty(partVals) ? String.join(delimit, partVals) : "";
}
public static String buildTableKey(String catName, String dbName, String tableName) {
- return buildKey(catName.toLowerCase(), dbName.toLowerCase(), tableName.toLowerCase());
+ return buildKey(StringUtils.normalizeIdentifier(catName),StringUtils.normalizeIdentifier(dbName),StringUtils.normalizeIdentifier(tableName));
Review comment:
nit: Space after ,
##########
File path: standalone-metastore/metastore-server/src/test/java/org/apache/hadoop/hive/metastore/cache/TestCachedStore.java
##########
@@ -1568,12 +1568,7 @@ public void testPrimaryKeys() {
List<SQLPrimaryKey> cachedKeys = sharedCache.listCachedPrimaryKeys(
DEFAULT_CATALOG_NAME, tbl.getDbName(), tbl.getTableName());
- Assert.assertEquals(cachedKeys.size(), 1);
- Assert.assertEquals(cachedKeys.get(0).getPk_name(), "pk1");
- Assert.assertEquals(cachedKeys.get(0).getTable_db(), "db");
- Assert.assertEquals(cachedKeys.get(0).getTable_name(), tbl.getTableName());
- Assert.assertEquals(cachedKeys.get(0).getColumn_name(), "col1");
- Assert.assertEquals(cachedKeys.get(0).getCatName(), DEFAULT_CATALOG_NAME);
+ Assert.assertEquals(origKeys,cachedKeys);
Review comment:
nit: Space after ,
----------------------------------------------------------------
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