Refactor HiveMetastore API and Implementations #4748

rash67 · 2020-08-08T01:27:16Z

This commit changes the HiveMetastore API to remove methods to methods
getPartionNames() and getPartitionNamesByParts() and replace with
getPartitionNamesByFilter(). Each implementation is updated as well as
all callsites. The previous behavior is maintained as far as
translation of any non-equal Domain values to the wildcard value.
A subsequent commit will implement this for the GlueHiveMetastore and
pass the supported translation of the Domain per column using the
GetPartitions API in the Glue Data Catalog.

rash67 · 2020-08-10T23:04:03Z

fix merge conflict and resulting compiler errors; minor format fix

rash67 · 2020-08-12T22:04:11Z

debugging failed tests; will update today

rash67 · 2020-08-14T21:27:27Z

split out the single property in HiveConfig

hive.assume-canonical-partition-keys

to per-metastore.

also debugging some various test failures

electrum

Still reviewing, but some initial commits. Overall looks good. Great work.

electrum · 2020-08-18T18:19:08Z

presto-hive/src/main/java/io/prestosql/plugin/hive/spi/block/BlockJsonSerde.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.prestosql.plugin.hive.spi.block;


Move to io.prestosql.plugin.hive.util package

electrum · 2020-08-26T23:41:57Z

presto-hive/src/main/java/io/prestosql/plugin/hive/spi/block/HiveBlockEncodingSerde.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.prestosql.plugin.hive.spi.block;


Move to io.prestosql.plugin.hive.util package

electrum · 2020-08-18T18:22:31Z

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveModule.java

@@ -124,6 +128,11 @@ public void configure(Binder binder)

        jsonBinder(binder).addDeserializerBinding(Type.class).to(TypeDeserializer.class);

+        binder.bind(HiveBlockEncodingSerde.class).in(Scopes.SINGLETON);


We shouldn't bind RecordingHiveMetastore here unconditionally.

There are three duplicated bindings for RecordingHiveMetastore in ThriftMetastoreModule and GlueMetastoreModule. Existing duplicated code:

binder.bind(HiveMetastore.class) .annotatedWith(ForCachingHiveMetastore.class) .to(RecordingHiveMetastore.class) .in(Scopes.SINGLETON); binder.bind(RecordingHiveMetastore.class).in(Scopes.SINGLETON); newExporter(binder).export(RecordingHiveMetastore.class).withGeneratedName();

Create a RecordingHiveMetastoreModule and move that plus this code into the new module. Then install the module in those metastore modules.

We now need additional code when binding RecordingHiveMetastore, so it makes sense to extract the module for it.

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/MetastoreUtil.java

electrum · 2020-08-18T20:22:48Z

presto-hive/src/test/java/io/prestosql/plugin/hive/HiveQueryRunner.java

@@ -297,5 +296,21 @@ public static void main(String[] args)
        Thread.sleep(10);
        log.info("======== SERVER STARTED ========");
        log.info("\n====\n%s\n====", queryRunner.getCoordinator().getBaseUrl());
+
+        Session session = createSession(Optional.empty());


Make sure to remove later

oh shoot, yes, done

electrum

Looks great. Remove WIP and "closes xxx" from the commit message, then we can merge it.

electrum · 2020-08-20T18:34:56Z

presto-hive/src/main/java/io/prestosql/plugin/hive/HivePartitionManager.java

@@ -266,70 +241,17 @@ public static boolean partitionMatches(List<HiveColumnHandle> partitionColumns,
        return true;
    }

-    private List<String> getFilteredPartitionNames(SemiTransactionalHiveMetastore metastore, HiveIdentity identity, SchemaTableName tableName, List<HiveColumnHandle> partitionKeys, TupleDomain<ColumnHandle> effectivePredicate)
+    private List<String> getFilteredPartitionNames(


It looks like this method signature didn't change, so keep the original formatting for the arguments.

electrum · 2020-08-20T18:43:01Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/MetastoreUtil.java

+    /**
+     * @param columnNames
+     * @param partitionKeysFilter
+     * @param assumeCanonicalPartitionKeys allow conversion of


This seems to be missing something. Maybe remove all the @param and just explain what the method does.

electrum · 2020-08-20T18:43:15Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/MetastoreUtil.java

+    }
+
+    /**
+     * @param columnNames


We avoid having empty/useless documentation for obvious parameters.

my mistake, i left this half-finished

electrum · 2020-08-20T18:44:06Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/MetastoreUtil.java

+        checkArgument(partitionKeysFilter.getDomains().isPresent());
+
+        Map<String, Domain> domainMap = partitionKeysFilter.getDomains().orElse(ImmutableMap.of());
+        List<String> partitionList = columnNames.stream().map(cn -> domainToString(domainMap.get(cn), assumeCanonicalPartitionKeys, HIVE_PARTITION_VALUE_WILDCARD)).collect(toImmutableList());


Nit: wrap stream operations

List x = stream() .map(...) .collect(...);

will do. i'll check my other uses of stream()

electrum · 2020-08-20T18:44:58Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/MetastoreUtil.java

+        if (domain != null && domain.isNullableSingleValue()) {
+            return sqlScalarToString(domain.getType(), domain.getNullableSingleValue(), assumeCanonicalPartitionKeys, partitionWildcardString);
+        }
+        else { // null or not a single value


Nit: else is redundant

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/MetastoreUtil.java

electrum · 2020-08-20T18:52:42Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/alluxio/AlluxioHiveMetastore.java

    {
        this.client = requireNonNull(client);
+        this.assumeCanonicalPartitionKeys = requireNonNull(metastoreConfig).isAssumeCanonicalPartitionKeys();


No need for requireNonNull since the . access will throw NPE anyway

ah good point

electrum · 2020-08-21T01:47:56Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/file/FileHiveMetastore.java

+        if (partitionKeysFilter.isNone()) {
+            return Optional.of(ImmutableList.of());
+        }
+        Optional<List<String>> parts = partitionKeyFilterToStringList(columnNames, partitionKeysFilter, assumeCanonicalPartitionKeys);


We shouldn't need the assumeCanonicalPartitionKeys config or filtering here at all, as filtering by the metastore is best effort. This isn't being pushed down, so we can let the caller do it.

I'm not sure I follow. previously, this information was indeed used by the caller to generate a List<String> parts that included the information. it would be a "regression" in the sense the caller would get a larger list.

are you saying just pass false and let the caller deal with it to avoid having this config for AlluxioHiveMetastore ?

the Glue and Thrift impls will still need it

talked on slack and i get this, i've made the change

isn't assumeCanonicalPartitionKeys applicable to some metastores and not the others -- eg depending whether the call is over string representation or interpreted values?

for example it may turn out to be HMS- (and file-?) metastore specific?

electrum · 2020-08-21T01:51:23Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/BridgingHiveMetastore.java

-    {
-        return delegate.getPartitionNamesByParts(identity, databaseName, tableName, parts);
+        if (partitionKeysFilter.isAll()) {
+            return delegate.getPartitionNames(identity, databaseName, tableName);


Should we drop the getPartitionNames() method from the ThriftMetastore interface? Then this can be a straight delegation.

I thought about this, but I didn't know what to pass to thrift to say "all". This should, in theory, be just TupleDomain.all(), but i hit some snags

i can look at it again

I made this change and I'm waiting for a new CI (pushed right now) to run to verify that my translation of TupleDomain.all() to a list of "" works. it should as this was the previous behavior.

I removed this and what happens is that once it's down to the thrift API, it seems like any value for a partitionKey in the 'parts' api that is "" (our wildcard value) gets removed. the parts api then fails when the list is empty

we might be able to solve this when we translate it to an expression, but for now, i think i need to restore the previous special case

tooptoop4 · 2020-08-21T06:29:14Z

does this change mean a higher minimum version of hive is required?

electrum · 2020-08-21T16:04:00Z

@tooptoop4 No, the subsequent PR that adds support for the new API will include a fallback for the old API.

findepi · 2020-08-21T16:09:54Z

...to-hive/src/main/java/io/prestosql/plugin/hive/metastore/SemiTransactionalHiveMetastore.java

@@ -635,34 +652,17 @@ public synchronized void truncateUnpartitionedTable(ConnectorSession session, St
        }
        // add newly-added partitions to the results from underlying metastore
        if (!partitionActionsOfTable.isEmpty()) {
-            List<String> columnNames = table.get().getPartitionColumns().stream().map(Column::getName).collect(Collectors.toList());
+            // the top of the function guards against TupleDomain.none() so this cannot be empty


"this" means what here?

removing this comment, holdover from when this still did filtering. it referred to the columnNames

I'll remove the comment

rash67 · 2020-08-22T00:27:21Z

I just pushed the latest update that should address all comments. i'll make one last pass locally, though
(and verify CI passes)

electrum · 2020-08-26T23:39:35Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/HiveMetastore.java

+     * @param partitionKeysFilter map of filters (Domain) for each partition column
+     * @return optionally, a list of strings where each entry is in the form of {key}={value}
+     * @see TupleDomain
+     */



Remove blank line before Javadoc

electrum · 2020-08-26T23:40:25Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/alluxio/AlluxioHiveMetastore.java

+        if (partitionKeysFilter.isNone()) {
+            return Optional.of(Collections.emptyList());
+        }
+        Optional<List<String>> optionalParts = partitionKeyFilterToStringList(columnNames, partitionKeysFilter, false);


We can remove all this client side filtering here since it's only a hint. So I think the whole method can be

return ProtoUtils.toPartitionInfoList( client.readTable(databaseName, tableName, Constraint.getDefaultInstance()));

got it. it looks like I need a mapping of PartitionInfo::getPartitionName as well

electrum · 2020-08-26T23:41:39Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/alluxio/AlluxioHiveMetastore.java

@@ -69,7 +72,7 @@
    private TableMasterClient client;

    @Inject
-    public AlluxioHiveMetastore(TableMasterClient client)
+    public AlluxioHiveMetastore(TableMasterClient client, AlluxioHiveMetastoreConfig metastoreConfig)


The config is not needed

electrum · 2020-08-26T23:41:57Z

presto-hive/src/main/java/io/prestosql/plugin/hive/spi/block/HiveBlockEncodingSerde.java

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package io.prestosql.plugin.hive.spi.block;


Move to io.prestosql.plugin.hive.util package

electrum · 2020-08-26T23:42:15Z

presto-hive/src/main/java/io/prestosql/plugin/hive/spi/block/HiveBlockEncodingSerde.java

+import static com.google.common.base.Preconditions.checkArgument;
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+// This class is exactly the same as BlockEncodingManager. They are in SPI and don't have access to InternalBlockEncodingSerde.


Remove the second sentence as it doesn't really fit here

i thought i did these, will update

electrum · 2020-08-26T23:43:12Z

presto-hive/src/test/java/io/prestosql/plugin/hive/metastore/TestMetastoreUtil.java

+            throws Exception
+    {
+        Optional<List<String>> ds = MetastoreUtil.partitionKeyFilterToStringList(Arrays.asList("ds1", "ds2", "ds3"), TupleDomain.all(), false);
+        System.err.println(ds.get());


Remove the printing. Probably add an assertion or remove the entire test method.

oops again, yea i'll assert or remove it

This commit changes the HiveMetastore API to remove methods to methods getPartionNames() and getPartitionNamesByParts() and replace with getPartitionNamesByFilter(). Each implementation is updated as well as all callsites. The previous behavior is maintained as far as translation of any non-equal Domain values to the wildcard value. A subsequent commit will implement translation of a Domain into a proper Glue expression for the GlueHiveMetastore GetPartitions API in the Glue Data Catalog.

electrum · 2020-08-27T23:13:14Z

Thanks!

findepi · 2020-10-19T11:39:26Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftHiveMetastore.java

            return retry()
                    .stopOn(NoSuchObjectException.class)
                    .stopOnIllegalExceptions()
                    .run("getPartitionNamesByParts", stats.getGetPartitionNamesByParts().wrap(() -> {
                        try (ThriftMetastoreClient client = createMetastoreClient(identity)) {
-                            return Optional.of(client.getPartitionNamesFiltered(databaseName, tableName, parts));
+                            return Optional.of(client.getPartitionNamesFiltered(databaseName, tableName, parts.get()));


the "getPartitionNamesByParts", stats.getGetPartitionNamesByParts()... above should be renamed too.

nvrm, i see this is pre-existing... still worth updating

cla-bot bot added the cla-signed label Aug 8, 2020

rash67 force-pushed the hive-metastore-refactor branch 4 times, most recently from cfed8bc to efa50d4 Compare August 10, 2020 23:03

rash67 force-pushed the hive-metastore-refactor branch 2 times, most recently from 2ca72a4 to fa91389 Compare August 12, 2020 01:59

rash67 force-pushed the hive-metastore-refactor branch 6 times, most recently from dfa4ff4 to c906c6e Compare August 14, 2020 21:20

rash67 force-pushed the hive-metastore-refactor branch from c906c6e to 9b5a5d4 Compare August 14, 2020 21:34

findepi requested a review from electrum August 17, 2020 13:58

electrum reviewed Aug 18, 2020

View reviewed changes

electrum self-requested a review August 18, 2020 22:02

electrum approved these changes Aug 21, 2020

View reviewed changes

findepi reviewed Aug 21, 2020

View reviewed changes

rash67 changed the title ~~Refactor HiveMetastore API and Implementations (WIP)~~ Refactor HiveMetastore API and Implementations Aug 21, 2020

rash67 force-pushed the hive-metastore-refactor branch 3 times, most recently from 0ce0a8d to bb6119e Compare August 22, 2020 00:25

rash67 force-pushed the hive-metastore-refactor branch from bb6119e to 39faca6 Compare August 24, 2020 19:51

rash67 force-pushed the hive-metastore-refactor branch 8 times, most recently from a8327ae to c24a983 Compare August 25, 2020 23:38

electrum approved these changes Aug 26, 2020

View reviewed changes

rash67 force-pushed the hive-metastore-refactor branch 4 times, most recently from ecfd87c to a167bba Compare August 27, 2020 00:13

rash67 force-pushed the hive-metastore-refactor branch from a167bba to 43843b2 Compare August 27, 2020 01:24

electrum merged commit 975bd66 into trinodb:master Aug 27, 2020

rash67 deleted the hive-metastore-refactor branch September 3, 2020 01:28

Lewuathe mentioned this pull request Sep 8, 2020

Remove unused WILDCARD_EXPRESSION #5108

Merged

guyco33 mentioned this pull request Oct 15, 2020

Regression in Hive metastore performance #5567

Closed

findepi reviewed Oct 19, 2020

View reviewed changes

		@@ -124,6 +128,11 @@ public void configure(Binder binder)

		jsonBinder(binder).addDeserializerBinding(Type.class).to(TypeDeserializer.class);

		binder.bind(HiveBlockEncodingSerde.class).in(Scopes.SINGLETON);

Refactor HiveMetastore API and Implementations #4748

Refactor HiveMetastore API and Implementations #4748

Conversation

rash67 commented Aug 8, 2020 • edited Loading

rash67 commented Aug 10, 2020

rash67 commented Aug 12, 2020

rash67 commented Aug 14, 2020

electrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tooptoop4 commented Aug 21, 2020

electrum commented Aug 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rash67 commented Aug 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

electrum commented Aug 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rash67 commented Aug 8, 2020 •

edited

Loading