sankarh commented on a change in pull request #1419:
URL: https://github.com/apache/hive/pull/1419#discussion_r487540360
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java
##########
@@ -1176,145 +1166,101 @@ public void setStatsStateLikeNewTable() {
* Note that set apis are used by DESCRIBE only, although get apis return
RELY or ENABLE
* constraints DESCRIBE could set all type of constraints
* */
-
- /* This only return PK which are created with RELY */
- public PrimaryKeyInfo getPrimaryKeyInfo() {
- if(!this.isPKFetched) {
+ public TableConstraintsInfo getTableConstraintsInfo() {
+ if (!this.isTableConstraintsFetched) {
try {
- pki = Hive.get().getReliablePrimaryKeys(this.getDbName(),
this.getTableName());
- this.isPKFetched = true;
+ tableConstraintsInfo =
Hive.get().getTableConstraints(this.getDbName(), this.getTableName(), true,
true);
+ this.isTableConstraintsFetched = true;
} catch (HiveException e) {
- LOG.warn("Cannot retrieve PK info for table : " + this.getTableName()
- + " ignoring exception: " + e);
+ LOG.warn(
Review comment:
Callers are assuming tableConstraintsInfo won't be null after invoking
this method but it is not if there is an exception.
Can initialize it with default constructor in this flow.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java
##########
@@ -116,22 +116,12 @@
private transient Boolean outdatedForRewritingMaterializedView;
/** Constraint related objects */
- private transient PrimaryKeyInfo pki;
- private transient ForeignKeyInfo fki;
- private transient UniqueConstraint uki;
- private transient NotNullConstraint nnc;
- private transient DefaultConstraint dc;
- private transient CheckConstraint cc;
+ private transient TableConstraintsInfo tableConstraintsInfo;
/** Constraint related flags
* This is to track if constraints are retrieved from metastore or not
*/
- private transient boolean isPKFetched=false;
- private transient boolean isFKFetched=false;
- private transient boolean isUniqueFetched=false;
- private transient boolean isNotNullFetched=false;
- private transient boolean isDefaultFetched=false;
- private transient boolean isCheckFetched=false;
+ private transient boolean isTableConstraintsFetched = false;
Review comment:
Shall remove this flag as we can check tableConstraintsInfo != null
instead. Btw, we need this flag if we use default constructor in exception flow
of getTableConstraintsInfo() method.
##########
File path:
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/IMetaStoreClient.java
##########
@@ -3631,6 +3631,17 @@ boolean cacheFileMetadata(String dbName, String
tableName, String partName,
List<SQLCheckConstraint> getCheckConstraints(CheckConstraintsRequest
request) throws MetaException,
NoSuchObjectException, TException;
+ /**
+ * Get all constraints of given table
+ * @param request Request info
+ * @return all constraints of this table
+ * @throws MetaException
+ * @throws NoSuchObjectException
+ * @throws TException
+ */
+ SQLAllTableConstraints getAllTableConstraints(AllTableConstraintsRequest
request)
Review comment:
I can still see it here.
##########
File path:
standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift
##########
@@ -720,6 +729,15 @@ struct CheckConstraintsResponse {
1: required list<SQLCheckConstraint> checkConstraints
}
+struct AllTableConstraintsRequest {
+ 1: required string dbName,
+ 2: required string tblName,
+ 3: required string catName
Review comment:
catName can be optional.
##########
File path:
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java
##########
@@ -9330,6 +9330,31 @@ public CheckConstraintsResponse
get_check_constraints(CheckConstraintsRequest re
return new CheckConstraintsResponse(ret);
}
+ /**
+ * Api to fetch all table constraints at once
+ * @param request it consist of catalog name, database name and table name
to identify the table in metastore
+ * @return all constraints attached to given table
+ * @throws TException
+ */
+ @Override
+ public AllTableConstraintsResponse
get_all_table_constraints(AllTableConstraintsRequest request) throws TException
{
Review comment:
This method also throws MetaException.
##########
File path:
standalone-metastore/metastore-common/src/main/thrift/hive_metastore.thrift
##########
@@ -2502,6 +2520,9 @@ PartitionsResponse get_partitions_req(1:PartitionsRequest
req)
throws(1:MetaException o1, 2:NoSuchObjectException o2)
CheckConstraintsResponse get_check_constraints(1:CheckConstraintsRequest
request)
throws(1:MetaException o1, 2:NoSuchObjectException o2)
+ // All table constrains
+ AllTableConstraintsResponse
get_all_table_constraints(1:AllTableConstraintsRequest request)
Review comment:
Here it shows, this method returns NoSuchObjectException but actually
not. I think, the implementation should return this exception and handle in HMS
client as well.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/metadata/Table.java
##########
@@ -1176,145 +1166,101 @@ public void setStatsStateLikeNewTable() {
* Note that set apis are used by DESCRIBE only, although get apis return
RELY or ENABLE
* constraints DESCRIBE could set all type of constraints
* */
-
- /* This only return PK which are created with RELY */
- public PrimaryKeyInfo getPrimaryKeyInfo() {
- if(!this.isPKFetched) {
+ public TableConstraintsInfo getTableConstraintsInfo() {
+ if (!this.isTableConstraintsFetched) {
try {
- pki = Hive.get().getReliablePrimaryKeys(this.getDbName(),
this.getTableName());
- this.isPKFetched = true;
+ tableConstraintsInfo =
Hive.get().getReliableAndEnableTableConstraints(this.getDbName(),
this.getTableName());
+ this.isTableConstraintsFetched = true;
} catch (HiveException e) {
- LOG.warn("Cannot retrieve PK info for table : " + this.getTableName()
- + " ignoring exception: " + e);
+ LOG.warn(
+ "Cannot retrieve table constraints info for table : " +
this.getTableName() + " ignoring exception: " + e);
}
}
- return pki;
+ return tableConstraintsInfo;
}
- public void setPrimaryKeyInfo(PrimaryKeyInfo pki) {
- this.pki = pki;
- this.isPKFetched = true;
+ /**
+ * TableConstraintsInfo setter
+ * @param tableConstraintsInfo
+ */
+ public void setTableConstraintsInfo(TableConstraintsInfo
tableConstraintsInfo) {
+ this.tableConstraintsInfo = tableConstraintsInfo;
+ this.isTableConstraintsFetched = true;
}
- /* This only return FK constraints which are created with RELY */
- public ForeignKeyInfo getForeignKeyInfo() {
- if(!isFKFetched) {
- try {
- fki = Hive.get().getReliableForeignKeys(this.getDbName(),
this.getTableName());
- this.isFKFetched = true;
- } catch (HiveException e) {
- LOG.warn(
- "Cannot retrieve FK info for table : " + this.getTableName()
- + " ignoring exception: " + e);
- }
+ /**
+ * This only return PK which are created with RELY
+ * @return primary key constraint list
+ */
+ public PrimaryKeyInfo getPrimaryKeyInfo() {
+ if (!this.isTableConstraintsFetched) {
+ getTableConstraintsInfo();
}
- return fki;
+ return tableConstraintsInfo.getPrimaryKeyInfo();
}
- public void setForeignKeyInfo(ForeignKeyInfo fki) {
- this.fki = fki;
- this.isFKFetched = true;
+ /**
+ * This only return FK constraints which are created with RELY
+ * @return foreign key constraint list
+ */
+ public ForeignKeyInfo getForeignKeyInfo() {
+ if (!isTableConstraintsFetched) {
+ getTableConstraintsInfo();
+ }
+ return tableConstraintsInfo.getForeignKeyInfo();
}
- /* This only return UNIQUE constraint defined with RELY */
+ /**
+ * This only return UNIQUE constraint defined with RELY
+ * @return unique constraint list
+ */
public UniqueConstraint getUniqueKeyInfo() {
- if(!isUniqueFetched) {
- try {
- uki = Hive.get().getReliableUniqueConstraints(this.getDbName(),
this.getTableName());
- this.isUniqueFetched = true;
- } catch (HiveException e) {
- LOG.warn(
- "Cannot retrieve Unique Key info for table : " +
this.getTableName()
- + " ignoring exception: " + e);
- }
+ if (!isTableConstraintsFetched) {
+ getTableConstraintsInfo();
}
- return uki;
- }
-
- public void setUniqueKeyInfo(UniqueConstraint uki) {
- this.uki = uki;
- this.isUniqueFetched = true;
+ return tableConstraintsInfo.getUniqueConstraint();
}
- /* This only return NOT NULL constraint defined with RELY */
+ /**
+ * This only return NOT NULL constraint defined with RELY
+ * @return not null constraint list
+ */
public NotNullConstraint getNotNullConstraint() {
- if(!isNotNullFetched) {
- try {
- nnc = Hive.get().getReliableNotNullConstraints(this.getDbName(),
this.getTableName());
- this.isNotNullFetched = true;
- } catch (HiveException e) {
- LOG.warn("Cannot retrieve Not Null constraint info for table : "
- + this.getTableName() + " ignoring exception: " + e);
- }
+ if (!isTableConstraintsFetched) {
+ getTableConstraintsInfo();
}
- return nnc;
- }
-
- public void setNotNullConstraint(NotNullConstraint nnc) {
- this.nnc = nnc;
- this.isNotNullFetched = true;
+ return tableConstraintsInfo.getNotNullConstraint();
}
- /* This only return DEFAULT constraint defined with ENABLE */
+ /**
+ * This only return DEFAULT constraint defined with ENABLE
+ * @return default constraint list
+ */
public DefaultConstraint getDefaultConstraint() {
- if(!isDefaultFetched) {
- try {
- dc = Hive.get().getEnabledDefaultConstraints(this.getDbName(),
this.getTableName());
- this.isDefaultFetched = true;
- } catch (HiveException e) {
- LOG.warn("Cannot retrieve Default constraint info for table : "
- + this.getTableName() + " ignoring exception: " + e);
- }
+ if (!isTableConstraintsFetched) {
+ getTableConstraintsInfo();
}
- return dc;
+ return tableConstraintsInfo.getDefaultConstraint();
}
- public void setDefaultConstraint(DefaultConstraint dc) {
- this.dc = dc;
- this.isDefaultFetched = true;
- }
-
- /* This only return CHECK constraint defined with ENABLE */
+ /**
+ * This only return CHECK constraint defined with ENABLE
+ * @return check constraint list
+ */
public CheckConstraint getCheckConstraint() {
- if(!isCheckFetched) {
- try{
- cc = Hive.get().getEnabledCheckConstraints(this.getDbName(),
this.getTableName());
- this.isCheckFetched = true;
- } catch (HiveException e) {
- LOG.warn("Cannot retrieve Check constraint info for table : "
- + this.getTableName() + " ignoring exception: " + e);
- }
+ if (!isTableConstraintsFetched) {
+ getTableConstraintsInfo();
}
- return cc;
- }
-
- public void setCheckConstraint(CheckConstraint cc) {
- this.cc = cc;
- this.isCheckFetched = true;
+ return tableConstraintsInfo.getCheckConstraint();
}
/** This shouldn't use get apis because those api call metastore
* to fetch constraints.
* getMetaData only need to make a copy of existing constraints, even if
those are not fetched
*/
public void copyConstraints(final Table tbl) {
- this.pki = tbl.pki;
- this.isPKFetched = tbl.isPKFetched;
-
- this.fki = tbl.fki;
- this.isFKFetched = tbl.isFKFetched;
-
- this.uki = tbl.uki;
- this.isUniqueFetched = tbl.isUniqueFetched;
-
- this.nnc = tbl.nnc;
- this.isNotNullFetched = tbl.isNotNullFetched;
-
- this.dc = tbl.dc;
- this.isDefaultFetched = tbl.isDefaultFetched;
-
- this.cc = tbl.cc;
- this.isCheckFetched = tbl.isCheckFetched;
+ this.tableConstraintsInfo = tbl.tableConstraintsInfo;
Review comment:
Need to understand from the caller on the usage and add appropriate
comment to mark this behavior. Deep-copy would add additional overhead.
----------------------------------------------------------------
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]
