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/04/24 03:17:49 UTC
[GitHub] [hive] jcamachor commented on a change in pull request #990: HIVE-23216: Add new api as replacement of get_partitions_by_expr to return PartitionSpec instead of Partitions
jcamachor commented on a change in pull request #990:
URL: https://github.com/apache/hive/pull/990#discussion_r414240046
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
##########
@@ -3835,6 +3837,62 @@ public boolean dropPartition(String dbName, String tableName, List<String> parti
return results;
}
+ private List<Partition> convertFromPartSpec(Iterator<PartitionSpec> iterator, Table tbl)
Review comment:
Can this be made `static`?
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
##########
@@ -3835,6 +3837,62 @@ public boolean dropPartition(String dbName, String tableName, List<String> parti
return results;
}
+ private List<Partition> convertFromPartSpec(Iterator<PartitionSpec> iterator, Table tbl)
+ throws HiveException, TException {
+ if(!iterator.hasNext()) {
+ return Collections.emptyList();
+ }
+ List<Partition> results = new ArrayList<>();
+
+ while (iterator.hasNext()) {
+ PartitionSpec partitionSpec = iterator.next();
+ if (partitionSpec.getPartitionList() != null) {
+ // partitions outside table location
+ Iterator<org.apache.hadoop.hive.metastore.api.Partition> externalPartItr =
+ partitionSpec.getPartitionList().getPartitions().iterator();
+ while(externalPartItr.hasNext()) {
+ org.apache.hadoop.hive.metastore.api.Partition msPart =
+ externalPartItr.next();
+ results.add(new Partition(tbl, msPart));
+ }
+ } else {
+ // partitions within table location
+ for(PartitionWithoutSD partitionWithoutSD:partitionSpec.getSharedSDPartitionSpec().getPartitions()) {
+ org.apache.hadoop.hive.metastore.api.Partition part = new org.apache.hadoop.hive.metastore.api.Partition();
+ part.setTableName(partitionSpec.getTableName());
+ part.setDbName(partitionSpec.getDbName());
+ part.setCatName(partitionSpec.getCatName());
+ part.setCreateTime(partitionWithoutSD.getCreateTime());
+ part.setLastAccessTime(partitionWithoutSD.getLastAccessTime());
+ part.setParameters(partitionWithoutSD.getParameters());
+ part.setPrivileges(partitionWithoutSD.getPrivileges());
+ part.setSd(partitionSpec.getSharedSDPartitionSpec().getSd().deepCopy());
+ String partitionLocation = null;
+ if(partitionWithoutSD.getRelativePath() == null
+ || partitionWithoutSD.getRelativePath().isEmpty()) {
+ if (tbl.getDataLocation() != null) {
+ Path partPath = new Path(tbl.getDataLocation(),
+ Warehouse.makePartName(tbl.getPartCols(),
+ partitionWithoutSD.getValues()));
+ partitionLocation = partPath.toString();
+ }
+ } else {
+ partitionLocation = tbl.getSd().getLocation();
+ partitionLocation += partitionWithoutSD.getRelativePath();
+ }
+ part.getSd().setLocation(partitionLocation);
+ part.setValues(partitionWithoutSD.getValues());
+ part.setWriteId(partitionSpec.getWriteId());
+ Partition hivePart = new Partition(tbl, part);
+ //assert(partitionWithoutSD.getRelativePath() != null);
Review comment:
Remove the commented out code.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/metadata/SessionHiveMetaStoreClient.java
##########
@@ -1178,6 +1179,22 @@ public boolean listPartitionsByExpr(String catName, String dbName, String tblNam
return result.isEmpty();
}
+ @Override
+ public boolean listPartitionsSpecByExpr(String catName, String dbName, String tblName, byte[] expr,
+ String defaultPartitionName, short maxParts, List<PartitionSpec> result) throws TException {
+ org.apache.hadoop.hive.metastore.api.Table table = getTempTable(dbName, tblName);
+ if (table == null) {
+ return super.listPartitionsSpecByExpr(catName, dbName, tblName, expr, defaultPartitionName, maxParts, result);
+ }
+ assert result != null;
+
+ result.addAll(
+ MetaStoreServerUtils.getPartitionspecsGroupedByStorageDescriptor(table,
+ getPartitionsForMaxParts(tblName, getPartitionedTempTable(table).listPartitionsByFilter(
+ generateJDOFilter(table, expr, defaultPartitionName)), maxParts)));
Review comment:
Indentation seems off here.
##########
File path: standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
##########
@@ -1941,6 +1941,45 @@ public boolean listPartitionsByExpr(String catName, String db_name, String tbl_n
return !r.isSetHasUnknownPartitions() || r.isHasUnknownPartitions(); // Assume the worst.
}
+ @Override
+ public boolean listPartitionsSpecByExpr(String dbName, String tblName,
+ byte[] expr, String defaultPartName, short maxParts, List<PartitionSpec> result)
+ throws TException {
+ return listPartitionsSpecByExpr(getDefaultCatalog(conf), dbName, tblName, expr, defaultPartName,
+ maxParts, result);
+ }
+
+ @Override
+ public boolean listPartitionsSpecByExpr(String catName, String dbName, String tblName, byte[] expr,
+ String defaultPartitionName, short maxParts, List<PartitionSpec> result)
+ throws TException {
+ assert result != null;
+ PartitionsByExprRequest req = new PartitionsByExprRequest(
+ dbName, tblName, ByteBuffer.wrap(expr));
+ if (defaultPartitionName != null) {
+ req.setDefaultPartitionName(defaultPartitionName);
+ }
+ if (maxParts >= 0) {
+ req.setMaxParts(maxParts);
+ }
+ PartitionsSpecByExprResult r;
+ try {
+ r = client.get_partitions_spec_by_expr(req);
+ } catch (TApplicationException te) {
+ if (te.getType() != TApplicationException.UNKNOWN_METHOD
+ && te.getType() != TApplicationException.WRONG_METHOD_NAME) {
+ throw te;
+ }
+ throw new IncompatibleMetastoreException(
+ "Metastore doesn't support listPartitionsByExpr: " + te.getMessage());
+ }
+
+ //TODO: filtering if client side filtering isClientFilterEnabled on
Review comment:
What does this comment mean? Is there remaining work?
##########
File path: ql/src/test/results/clientpositive/partition_wise_fileformat2.q.out
##########
@@ -101,56 +101,6 @@ POSTHOOK: Input: default@partition_test_partitioned@dt=100
POSTHOOK: Input: default@partition_test_partitioned@dt=101
POSTHOOK: Input: default@partition_test_partitioned@dt=102
#### A masked pattern was here ####
-238 val_238 100
Review comment:
We should add an order by (or sort query results) to make this query deterministic for certain. Same for queries below.
##########
File path: ql/src/test/org/apache/hadoop/hive/ql/metadata/TestSessionHiveMetastoreClientListPartitionsTempTable.java
##########
@@ -243,65 +244,65 @@ public void testListPartitionsByExpr() throws Exception {
public void testListPartitionsByExprNullResult() throws Exception {
Review comment:
We should probably create different test methods so we can keep the coverage of the `listPartitionsByExpr` method since it is still part of the HMS API.
##########
File path: ql/src/java/org/apache/hadoop/hive/ql/metadata/Hive.java
##########
@@ -3835,6 +3837,62 @@ public boolean dropPartition(String dbName, String tableName, List<String> parti
return results;
}
+ private List<Partition> convertFromPartSpec(Iterator<PartitionSpec> iterator, Table tbl)
Review comment:
Please add a comment here describing what this method is useful for and what is a partition spec.
##########
File path: standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java
##########
@@ -1941,6 +1941,45 @@ public boolean listPartitionsByExpr(String catName, String db_name, String tbl_n
return !r.isSetHasUnknownPartitions() || r.isHasUnknownPartitions(); // Assume the worst.
}
+ @Override
+ public boolean listPartitionsSpecByExpr(String dbName, String tblName,
+ byte[] expr, String defaultPartName, short maxParts, List<PartitionSpec> result)
+ throws TException {
+ return listPartitionsSpecByExpr(getDefaultCatalog(conf), dbName, tblName, expr, defaultPartName,
+ maxParts, result);
+ }
+
+ @Override
+ public boolean listPartitionsSpecByExpr(String catName, String dbName, String tblName, byte[] expr,
+ String defaultPartitionName, short maxParts, List<PartitionSpec> result)
+ throws TException {
+ assert result != null;
+ PartitionsByExprRequest req = new PartitionsByExprRequest(
+ dbName, tblName, ByteBuffer.wrap(expr));
+ if (defaultPartitionName != null) {
+ req.setDefaultPartitionName(defaultPartitionName);
+ }
+ if (maxParts >= 0) {
+ req.setMaxParts(maxParts);
+ }
+ PartitionsSpecByExprResult r;
+ try {
+ r = client.get_partitions_spec_by_expr(req);
+ } catch (TApplicationException te) {
+ if (te.getType() != TApplicationException.UNKNOWN_METHOD
+ && te.getType() != TApplicationException.WRONG_METHOD_NAME) {
+ throw te;
+ }
+ throw new IncompatibleMetastoreException(
+ "Metastore doesn't support listPartitionsByExpr: " + te.getMessage());
+ }
+
+ //TODO: filtering if client side filtering isClientFilterEnabled on
+ r.setPartitionsSpec(r.getPartitionsSpec());
Review comment:
Is this a noop (getting and setting on same object)?
##########
File path: standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/utils/MetaStoreServerUtils.java
##########
@@ -1009,13 +1009,29 @@ public static String getTokenStoreClassName(Configuration conf) {
Collection<Partition> partitions) {
final String tablePath = table.getSd().getLocation();
+
Review comment:
You can remove new line.
----------------------------------------------------------------
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