Fixed issue where thrift methods returning partitions were returning …

…partitions with partition values escaped. (#265)

* Fixed issue where thrift methods returning partitions were returning partitions with partition values escaped. Hive partition names are escaped where as the partition values contain the original value.

metacat-functional-tests/src/functionalTest/groovy/com/netflix/metacat/MetacatSmokeThriftSpec.groovy

@@ -492,4 +492,47 @@ class MetacatSmokeThriftSpec extends Specification {
  '' | 'invalid=xyz' | -1
  'end' | "one='xyz' and (total=11 or total=12)" | 2
  }
+
+ @Unroll
+ def "Test: Embedded Fast Thrift connector: getPartitionsByNames with escape values"() {
+ given:
+ def catalogName = 'localfast'
+ def client = clients.get(catalogName)
+ def databaseName = 'test_db5_' + catalogName
+ def tableName = 'parts'
+ def hiveTable = createTable(client, catalogName, databaseName, tableName)
+ def uri = isLocalEnv ? 'file:/tmp/abc' : null;
+ def dto = converter.toTableDto(hiveConverter.toTableInfo(QualifiedName.ofTable(catalogName, databaseName, tableName), hiveTable.getTTable()))
+ def partitionDtos = DataDtoProvider.getPartitions(catalogName, databaseName, tableName, 'one=xy^:z/total=1', uri, 10)
+ def partitions = partitionDtos.collect {
+ new Partition(hiveTable, hiveConverter.fromPartitionInfo(converter.fromTableDto(dto), converter.fromPartitionDto(it)))
+ }
+ client.alterPartitions(databaseName + '.' + tableName, partitions)
+ when:
+ def result = client.getPartitionsByNames(hiveTable, ['one=xy%5E%3Az/total=10'])
+ then:
+ result.size() == 1
+ result.get(0).getValues() == ['xy^:z', '10']
+ when:
+ result = client.getPartitionsByNames(hiveTable, ['one=xy^:z/total=10'])
+ then:
+ result.size() == 0
+ when:
+ result = client.getPartitionsByNames(hiveTable, ['total':'10'])
+ then:
+ result.size() == 1
+ result.get(0).getValues() == ['xy^:z', '10']
+ when:
+ result = client.getPartitionsByNames(hiveTable, ['one':'xy^:z'])
+ then:
+ result.size() == 0
+ when:
+ result = client.getPartitionsByNames(hiveTable, ['one':'xy%5E%3Az'])
+ then:
+ result.size() == 10
+ cleanup:
+ client.getPartitions(hiveTable).each {
+ client.dropPartition(databaseName, tableName, it.getValues(), false)
+ }
+ }
 }

metacat-thrift/src/main/java/com/netflix/metacat/thrift/CatalogThriftHiveMetastore.java

@@ -299,7 +299,8 @@ private List<Partition> addPartitionsCore(final String dbName, final String tblN
  throws TException {
  log.debug("Ignoring {} since metacat save partitions will do an update if it already exists", ifNotExists);
  final TableDto tableDto = v1.getTable(catalogName, dbName, tblName, true, false, false);
- if (tableDto.getPartition_keys() == null || tableDto.getPartition_keys().isEmpty()) {
+ final List<String> partitionKeys = tableDto.getPartition_keys();
+ if (partitionKeys == null || partitionKeys.isEmpty()) {
  throw new MetaException("Unable to add partition to unpartitioned table: " + tableDto.getName());
  }
 

metacat-thrift/src/main/java/com/netflix/metacat/thrift/HiveConvertersImpl.java

@@ -17,9 +17,7 @@
 package com.netflix.metacat.thrift;
 
 import com.google.common.annotations.VisibleForTesting;
-import com.google.common.base.Splitter;
 import com.google.common.base.Strings;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
 import com.netflix.metacat.common.QualifiedName;
@@ -30,6 +28,7 @@
 import com.netflix.metacat.common.dto.StorageDto;
 import com.netflix.metacat.common.dto.TableDto;
 import com.netflix.metacat.common.dto.ViewDto;
+import org.apache.hadoop.hive.common.FileUtils;
 import org.apache.hadoop.hive.metastore.TableType;
 import org.apache.hadoop.hive.metastore.Warehouse;
 import org.apache.hadoop.hive.metastore.api.Database;
@@ -54,8 +53,6 @@
  * Hive converter.
  */
 public class HiveConvertersImpl implements HiveConverters {
- private static final Splitter SLASH_SPLITTER = Splitter.on('/');
- private static final Splitter EQUAL_SPLITTER = Splitter.on('=').limit(2);
 
  @VisibleForTesting
  Integer dateToEpochSeconds(@Nullable final Date date) {
@@ -322,7 +319,7 @@ public PartitionDto hiveToMetacatPartition(final TableDto tableDto, final Partit
  * {@inheritDoc}
  */
  @Override
- public List<String> getPartValsFromName(final TableDto tableDto, final String partName) {
+ public List<String> getPartValsFromName(@Nullable final TableDto tableDto, final String partName) {
  // Unescape the partition name
 
  final LinkedHashMap<String, String> hm;
@@ -331,16 +328,25 @@ public List<String> getPartValsFromName(final TableDto tableDto, final String pa
  } catch (MetaException e) {
  throw new IllegalArgumentException("Invalid partition name", e);
  }
-
- final List<String> partVals = Lists.newArrayList();
- for (String key : tableDto.getPartition_keys()) {
- final String val = hm.get(key);
- if (val == null) {
- throw new IllegalArgumentException("Invalid partition name - missing " + key);
+ // Get the partition keys.
+ List<String> partitionKeys = null;
+ if (tableDto != null) {
+ partitionKeys = tableDto.getPartition_keys();
+ }
+ // If table has not been provided, return the values without validating.
+ if (partitionKeys != null) {
+ final List<String> partVals = Lists.newArrayListWithCapacity(partitionKeys.size());
+ for (String key : partitionKeys) {
+ final String val = hm.get(key);
+ if (val == null) {
+ throw new IllegalArgumentException("Invalid partition name - missing " + key);
+ }
+ partVals.add(val);
  }
- partVals.add(val);
+ return partVals;
+ } else {
+ return Lists.newArrayList(hm.values());
  }
- return partVals;
  }
 
  /**
@@ -352,18 +358,7 @@ public String getNameFromPartVals(final TableDto tableDto, final List<String> pa
  if (partitionKeys.size() != partVals.size()) {
  throw new IllegalArgumentException("Not the same number of partition columns and partition values");
  }
-
- final StringBuilder builder = new StringBuilder();
- for (int i = 0; i < partitionKeys.size(); i++) {
- if (builder.length() > 0) {
- builder.append('/');
- }
-
- builder.append(partitionKeys.get(i));
- builder.append('=');
- builder.append(partVals.get(i));
- }
- return builder.toString();
+ return FileUtils.makePartName(partitionKeys, partVals, "");
  }
 
  /**
@@ -374,19 +369,16 @@ public Partition metacatToHivePartition(final PartitionDto partitionDto, @Nullab
  final Partition result = new Partition();
 
  final QualifiedName name = partitionDto.getName();
- final List<String> values = Lists.newArrayListWithCapacity(16);
+ List<String> values = Lists.newArrayListWithCapacity(16);
  String databaseName = "";
  String tableName = "";
  if (name != null) {
  if (name.getPartitionName() != null) {
- for (String partialPartName : SLASH_SPLITTER.split(partitionDto.getName().getPartitionName())) {
- final List<String> nameValues = ImmutableList.copyOf(EQUAL_SPLITTER.split(partialPartName));
- if (nameValues.size() != 2) {
- throw new IllegalStateException("Unrecognized partition name: " + partitionDto.getName());
- }
- final String value = nameValues.get(1);
- values.add(value);
- }
+ //
+ // Unescape the partition name to get the right partition values.
+ // Partition name always are escaped where as the parition values are not.
+ //
+ values = getPartValsFromName(tableDto, name.getPartitionName());
  }
 
  if (name.getDatabaseName() != null) {

metacat-thrift/src/test/groovy/com/netflix/metacat/thrift/HiveConvertersSpec.groovy

@@ -292,7 +292,7 @@ class HiveConvertersSpec extends Specification {
  def partition = converter.metacatToHivePartition(dto, tableDto)
 
  then:
- partition.values == ['CAPS', 'lower', '3']
+ partition.values == ['CAPS', 'lower']
  partition.tableName == tableName
  partition.dbName == databaseName
  partition.createTime == Instant.parse('2016-02-25T14:47:27').getMillis() / 1000
@@ -315,7 +315,7 @@ class HiveConvertersSpec extends Specification {
  where:
  databaseName = 'database'
  tableName = 'table'
- partitionName = 'key1=CAPS/key2=lower/key3=3'
+ partitionName = 'field_0=CAPS/field_1=lower'
  owner = 'owner'
  createDate = Instant.parse('2016-02-25T14:47:27').toDate()
  location = 'location'
@@ -352,21 +352,21 @@ class HiveConvertersSpec extends Specification {
  def partition = converter.metacatToHivePartition(dto, tableDto)
 
  then:
- partition.values == ['weird=true', '', 'monk']
+ partition.values == ['true', 'monk']
 
  where:
  dto = new PartitionDto(
- name: QualifiedName.ofPartition('c', 'd', 't', 'this=weird=true/bob=/someone=monk')
+ name: QualifiedName.ofPartition('c', 'd', 't', 'this=weird=true/someone=monk')
  )
  tableDto = new TableDto()
  }
 
- def 'test metacatToHivePartition throws an error on invalid partition name'() {
+ def 'test metacatToHivePartition throws no error on invalid partition name'() {
  when:
  converter.metacatToHivePartition(dto, tableDto)
 
  then:
- thrown(IllegalStateException)
+ noExceptionThrown()
 
  where:
  dto = new PartitionDto(