diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/BridgingHiveMetastore.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/BridgingHiveMetastore.java index 2edd98bcdc54..59167216f737 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/BridgingHiveMetastore.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/BridgingHiveMetastore.java @@ -371,7 +371,7 @@ public Map> getPartitionsByNames(Table table, List schema = table.getDataColumns().stream() diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/CoalescingCounter.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/CoalescingCounter.java index 8fe0ceeee737..76c34c57a639 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/CoalescingCounter.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/CoalescingCounter.java @@ -50,7 +50,7 @@ public CoalescingCounter(Duration coalescingDuration) coalescingDurationMillis = requireNonNull(coalescingDuration, "coalescingDuration is null").toMillis(); } - public synchronized void increment() + private synchronized void increment() { long now = clock.instant().toEpochMilli(); if (lastUpdateTime + coalescingDurationMillis >= now) { diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/DefaultThriftMetastoreClientFactory.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/DefaultThriftMetastoreClientFactory.java index 8e8cd0ab44fc..dfaf89507a08 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/DefaultThriftMetastoreClientFactory.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/DefaultThriftMetastoreClientFactory.java @@ -43,6 +43,7 @@ import java.util.Arrays; import java.util.List; import java.util.Optional; +import java.util.concurrent.atomic.AtomicInteger; import static java.lang.Math.toIntExact; import static java.util.Collections.list; @@ -57,6 +58,11 @@ public class DefaultThriftMetastoreClientFactory private final HiveMetastoreAuthentication metastoreAuthentication; private final String hostname; + private final MetastoreSupportsDateStatistics metastoreSupportsDateStatistics = new MetastoreSupportsDateStatistics(); + private final AtomicInteger chosenGetTableAlternative = new AtomicInteger(Integer.MAX_VALUE); + private final AtomicInteger chosenTableParamAlternative = new AtomicInteger(Integer.MAX_VALUE); + private final AtomicInteger chosenGetAllViewsAlternative = new AtomicInteger(Integer.MAX_VALUE); + public DefaultThriftMetastoreClientFactory( Optional sslContext, Optional socksProxy, @@ -101,7 +107,11 @@ protected ThriftMetastoreClient create(TTransport transport, String hostname) { return new ThriftHiveMetastoreClient( transport, - hostname); + hostname, + metastoreSupportsDateStatistics, + chosenGetTableAlternative, + chosenTableParamAlternative, + chosenGetAllViewsAlternative); } private TTransport createTransport(HostAndPort address, Optional delegationToken) diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/FailureAwareThriftMetastoreClient.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/FailureAwareThriftMetastoreClient.java index 8491516cd21d..620ff09ee85e 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/FailureAwareThriftMetastoreClient.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/FailureAwareThriftMetastoreClient.java @@ -91,17 +91,17 @@ public List getAllTables(String databaseName) } @Override - public List getTableNamesByFilter(String databaseName, String filter) + public List getAllViews(String databaseName) throws TException { - return runWithHandle(() -> delegate.getTableNamesByFilter(databaseName, filter)); + return runWithHandle(() -> delegate.getAllViews(databaseName)); } @Override - public List getTableNamesByType(String databaseName, String tableType) + public List getTablesWithParameter(String databaseName, String parameterKey, String parameterValue) throws TException { - return runWithHandle(() -> delegate.getTableNamesByType(databaseName, tableType)); + return runWithHandle(() -> delegate.getTablesWithParameter(databaseName, parameterKey, parameterValue)); } @Override @@ -153,13 +153,6 @@ public Table getTable(String databaseName, String tableName) return runWithHandle(() -> delegate.getTable(databaseName, tableName)); } - @Override - public Table getTableWithCapabilities(String databaseName, String tableName) - throws TException - { - return runWithHandle(() -> delegate.getTableWithCapabilities(databaseName, tableName)); - } - @Override public List getFields(String databaseName, String tableName) throws TException diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/KerberosHiveMetastoreAuthentication.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/KerberosHiveMetastoreAuthentication.java index 35b76b4d3726..bf9f6a076f47 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/KerberosHiveMetastoreAuthentication.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/KerberosHiveMetastoreAuthentication.java @@ -131,14 +131,13 @@ private static class SaslClientCallbackHandler public void handle(Callback[] callbacks) { for (Callback callback : callbacks) { - if (callback instanceof NameCallback) { - ((NameCallback) callback).setName(username); + if (callback instanceof NameCallback nameCallback) { + nameCallback.setName(username); } - if (callback instanceof PasswordCallback) { - ((PasswordCallback) callback).setPassword(password.toCharArray()); + if (callback instanceof PasswordCallback passwordCallback) { + passwordCallback.setPassword(password.toCharArray()); } - if (callback instanceof RealmCallback) { - RealmCallback realmCallback = (RealmCallback) callback; + if (callback instanceof RealmCallback realmCallback) { realmCallback.setText(realmCallback.getDefaultText()); } } diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/MetastoreSupportsDateStatistics.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/MetastoreSupportsDateStatistics.java new file mode 100644 index 000000000000..bc87cf469bfa --- /dev/null +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/MetastoreSupportsDateStatistics.java @@ -0,0 +1,57 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package io.trino.plugin.hive.metastore.thrift; + +import io.airlift.units.Duration; + +import javax.annotation.concurrent.ThreadSafe; + +import java.util.concurrent.atomic.AtomicReference; + +import static io.trino.plugin.hive.metastore.thrift.MetastoreSupportsDateStatistics.DateStatisticsSupport.NOT_SUPPORTED; +import static io.trino.plugin.hive.metastore.thrift.MetastoreSupportsDateStatistics.DateStatisticsSupport.SUPPORTED; +import static io.trino.plugin.hive.metastore.thrift.MetastoreSupportsDateStatistics.DateStatisticsSupport.UNKNOWN; +import static java.util.concurrent.TimeUnit.SECONDS; + +@ThreadSafe +class MetastoreSupportsDateStatistics +{ + private static final int MAX_SET_DATE_STATISTICS_ATTEMPTS = 100; + + public enum DateStatisticsSupport { + SUPPORTED, NOT_SUPPORTED, UNKNOWN + } + + private final AtomicReference supported = new AtomicReference<>(UNKNOWN); + private final CoalescingCounter failures = new CoalescingCounter(new Duration(1, SECONDS)); + + public DateStatisticsSupport isSupported() + { + return supported.get(); + } + + public void succeeded() + { + supported.set(SUPPORTED); + } + + public void failed() + { + // When `dateStatistics.size() > 1` we expect something like "TApplicationException: Required field 'colName' is unset! Struct:ColumnStatisticsObj(colName:null, colType:null, statsData:null)" + // When `dateStatistics.size() == 1` we expect something like "TTransportException: java.net.SocketTimeoutException: Read timed out" + if (failures.incrementAndGet() >= MAX_SET_DATE_STATISTICS_ATTEMPTS) { + supported.set(NOT_SUPPORTED); + } + } +} diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java index 1e4d3a3d9f8a..53fca0d8425d 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastore.java @@ -50,7 +50,6 @@ import org.apache.hadoop.fs.Path; import org.apache.hadoop.hive.metastore.LockComponentBuilder; import org.apache.hadoop.hive.metastore.LockRequestBuilder; -import org.apache.hadoop.hive.metastore.TableType; import org.apache.hadoop.hive.metastore.api.AlreadyExistsException; import org.apache.hadoop.hive.metastore.api.ColumnStatisticsObj; import org.apache.hadoop.hive.metastore.api.ConfigValSecurityException; @@ -82,7 +81,6 @@ import org.apache.hadoop.hive.metastore.api.TxnToWriteId; import org.apache.hadoop.hive.metastore.api.UnknownDBException; import org.apache.hadoop.hive.metastore.api.UnknownTableException; -import org.apache.thrift.TApplicationException; import org.apache.thrift.TException; import javax.annotation.concurrent.ThreadSafe; @@ -96,19 +94,12 @@ import java.util.Optional; import java.util.OptionalLong; import java.util.Set; -import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; -import java.util.function.Predicate; -import java.util.regex.Pattern; import java.util.stream.Stream; import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Strings.isNullOrEmpty; -import static com.google.common.base.Throwables.propagateIfPossible; import static com.google.common.base.Throwables.throwIfUnchecked; -import static com.google.common.base.Verify.verify; -import static com.google.common.base.Verify.verifyNotNull; import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.collect.ImmutableMap.toImmutableMap; import static com.google.common.collect.ImmutableSet.toImmutableSet; @@ -132,17 +123,12 @@ import static io.trino.spi.StandardErrorCode.ALREADY_EXISTS; import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED; import static io.trino.spi.security.PrincipalType.USER; -import static java.lang.Boolean.FALSE; -import static java.lang.Boolean.TRUE; import static java.lang.String.format; import static java.lang.System.nanoTime; import static java.util.Objects.requireNonNull; -import static java.util.concurrent.TimeUnit.SECONDS; import static org.apache.hadoop.hive.common.FileUtils.makePartName; import static org.apache.hadoop.hive.metastore.TableType.MANAGED_TABLE; import static org.apache.hadoop.hive.metastore.api.HiveObjectType.TABLE; -import static org.apache.hadoop.hive.metastore.api.hive_metastoreConstants.HIVE_FILTER_FIELD_PARAMS; -import static org.apache.thrift.TApplicationException.UNKNOWN_METHOD; @ThreadSafe public class ThriftHiveMetastore @@ -150,12 +136,8 @@ public class ThriftHiveMetastore { private static final Logger log = Logger.get(ThriftHiveMetastore.class); - private static final int MAX_SET_DATE_STATISTICS_ATTEMPTS = 100; private static final String DEFAULT_METASTORE_USER = "presto"; - private static final Pattern TABLE_PARAMETER_SAFE_KEY_PATTERN = Pattern.compile("^[a-zA-Z_]+$"); - private static final Pattern TABLE_PARAMETER_SAFE_VALUE_PATTERN = Pattern.compile("^[a-zA-Z0-9\\s]*$"); - private final HdfsContext hdfsContext = new HdfsContext(ConnectorIdentity.ofUser(DEFAULT_METASTORE_USER)); private final Optional identity; @@ -172,13 +154,6 @@ public class ThriftHiveMetastore private final boolean assumeCanonicalPartitionKeys; private final ThriftMetastoreStats stats; - private final AtomicInteger chosenGetTableAlternative = new AtomicInteger(Integer.MAX_VALUE); - private final AtomicInteger chosenTableParamAlternative = new AtomicInteger(Integer.MAX_VALUE); - private final AtomicInteger chosesGetAllViewsAlternative = new AtomicInteger(Integer.MAX_VALUE); - - private final AtomicReference> metastoreSupportsDateStatistics = new AtomicReference<>(Optional.empty()); - private final CoalescingCounter metastoreSetDateStatisticsFailures = new CoalescingCounter(new Duration(1, SECONDS)); - public ThriftHiveMetastore( Optional identity, HdfsEnvironment hdfsEnvironment, @@ -290,8 +265,11 @@ public List getTablesWithParameter(String databaseName, String parameter return retry() .stopOn(UnknownDBException.class) .stopOnIllegalExceptions() - .run("getTablesWithParameter", stats.getGetTablesWithParameter().wrap( - () -> doGetTablesWithParameter(databaseName, parameterKey, parameterValue))); + .run("getTablesWithParameter", stats.getGetTablesWithParameter().wrap(() -> { + try (ThriftMetastoreClient client = createMetastoreClient()) { + return client.getTablesWithParameter(databaseName, parameterKey, parameterValue); + } + })); } catch (UnknownDBException e) { return ImmutableList.of(); @@ -312,8 +290,9 @@ public Optional getTable(String databaseName, String tableName) .stopOn(NoSuchObjectException.class) .stopOnIllegalExceptions() .run("getTable", stats.getGetTable().wrap(() -> { - Table table = getTableFromMetastore(databaseName, tableName); - return Optional.of(table); + try (ThriftMetastoreClient client = createMetastoreClient()) { + return Optional.of(client.getTable(databaseName, tableName)); + } })); } catch (NoSuchObjectException e) { @@ -327,17 +306,6 @@ public Optional
getTable(String databaseName, String tableName) } } - private Table getTableFromMetastore(String databaseName, String tableName) - throws TException - { - return alternativeCall( - this::createMetastoreClient, - ThriftHiveMetastore::defaultIsValidExceptionalResponse, - chosenGetTableAlternative, - client -> client.getTableWithCapabilities(databaseName, tableName), - client -> client.getTable(databaseName, tableName)); - } - @Override public Set getSupportedColumnStatistics(Type type) { @@ -523,11 +491,10 @@ private void setTableColumnStatistics(String databaseName, String tableName, Lis .stopOn(NoSuchObjectException.class, InvalidObjectException.class, MetaException.class, InvalidInputException.class) .stopOnIllegalExceptions() .run("setTableColumnStatistics", stats.getSetTableColumnStatistics().wrap(() -> { - setColumnStatistics( - format("table %s.%s", databaseName, tableName), - statistics, - (client, stats) -> client.setTableColumnStatistics(databaseName, tableName, stats)); - return null; + try (ThriftMetastoreClient client = createMetastoreClient()) { + client.setTableColumnStatistics(databaseName, tableName, statistics); + return null; + } })); } catch (NoSuchObjectException e) { @@ -615,10 +582,9 @@ private void setPartitionColumnStatistics(String databaseName, String tableName, .stopOn(NoSuchObjectException.class, InvalidObjectException.class, MetaException.class, InvalidInputException.class) .stopOnIllegalExceptions() .run("setPartitionColumnStatistics", stats.getSetPartitionColumnStatistics().wrap(() -> { - setColumnStatistics( - format("partition of table %s.%s", databaseName, tableName), - statistics, - (client, stats) -> client.setPartitionColumnStatistics(databaseName, tableName, partitionName, stats)); + try (ThriftMetastoreClient client = createMetastoreClient()) { + client.setPartitionColumnStatistics(databaseName, tableName, partitionName, statistics); + } return null; })); } @@ -657,58 +623,6 @@ private void deletePartitionColumnStatistics(String databaseName, String tableNa } } - private void setColumnStatistics(String objectName, List statistics, Call1> saveColumnStatistics) - throws TException - { - boolean containsDateStatistics = statistics.stream().anyMatch(stats -> stats.getStatsData().isSetDateStats()); - - Optional metastoreSupportsDateStatistics = this.metastoreSupportsDateStatistics.get(); - if (containsDateStatistics && metastoreSupportsDateStatistics.equals(Optional.of(FALSE))) { - log.debug("Skipping date statistics for %s because metastore does not support them", objectName); - statistics = statistics.stream() - .filter(stats -> !stats.getStatsData().isSetDateStats()) - .collect(toImmutableList()); - containsDateStatistics = false; - } - - if (!containsDateStatistics || metastoreSupportsDateStatistics.equals(Optional.of(TRUE))) { - try (ThriftMetastoreClient client = createMetastoreClient()) { - saveColumnStatistics.call(client, statistics); - } - return; - } - - List statisticsExceptDate = statistics.stream() - .filter(stats -> !stats.getStatsData().isSetDateStats()) - .collect(toImmutableList()); - - List dateStatistics = statistics.stream() - .filter(stats -> stats.getStatsData().isSetDateStats()) - .collect(toImmutableList()); - - verify(!dateStatistics.isEmpty() && metastoreSupportsDateStatistics.equals(Optional.empty())); - - try (ThriftMetastoreClient client = createMetastoreClient()) { - if (!statisticsExceptDate.isEmpty()) { - saveColumnStatistics.call(client, statisticsExceptDate); - } - - try { - saveColumnStatistics.call(client, dateStatistics); - } - catch (TException e) { - // When `dateStatistics.size() > 1` we expect something like "TApplicationException: Required field 'colName' is unset! Struct:ColumnStatisticsObj(colName:null, colType:null, statsData:null)" - // When `dateStatistics.size() == 1` we expect something like "TTransportException: java.net.SocketTimeoutException: Read timed out" - log.warn(e, "Failed to save date statistics for %s. Metastore might not support date statistics", objectName); - if (!statisticsExceptDate.isEmpty() && metastoreSetDateStatisticsFailures.incrementAndGet() >= MAX_SET_DATE_STATISTICS_ATTEMPTS) { - this.metastoreSupportsDateStatistics.set(Optional.of(FALSE)); - } - return; - } - } - this.metastoreSupportsDateStatistics.set(Optional.of(TRUE)); - } - @Override public void createRole(String role, String grantor) { @@ -893,16 +807,12 @@ public List getAllViews(String databaseName) .stopOn(UnknownDBException.class) .stopOnIllegalExceptions() .run("getAllViews", stats.getGetAllViews().wrap(() -> { - if (translateHiveViews) { - return alternativeCall( - this::createMetastoreClient, - exception -> !isUnknownMethodExceptionalResponse(exception), - chosesGetAllViewsAlternative, - client -> client.getTableNamesByType(databaseName, TableType.VIRTUAL_VIEW.name()), - // fallback to enumerating Presto views only (Hive views will still be executed, but will be listed as tables) - client -> doGetTablesWithParameter(databaseName, PRESTO_VIEW_FLAG, "true")); + try (ThriftMetastoreClient client = createMetastoreClient()) { + if (translateHiveViews) { + return client.getAllViews(databaseName); + } + return client.getTablesWithParameter(databaseName, PRESTO_VIEW_FLAG, "true"); } - return doGetTablesWithParameter(databaseName, PRESTO_VIEW_FLAG, "true"); })); } catch (UnknownDBException e) { @@ -916,34 +826,6 @@ public List getAllViews(String databaseName) } } - private List doGetTablesWithParameter(String databaseName, String parameterKey, String parameterValue) - throws TException - { - checkArgument(TABLE_PARAMETER_SAFE_KEY_PATTERN.matcher(parameterKey).matches(), "Parameter key contains invalid characters: '%s'", parameterKey); - /* - * The parameter value is restricted to have only alphanumeric characters so that it's safe - * to be used against HMS. When using with a LIKE operator, the HMS may want the parameter - * value to follow a Java regex pattern or a SQL pattern. And it's hard to predict the - * HMS's behavior from outside. Also, by restricting parameter values, we avoid the problem - * of how to quote them when passing within the filter string. - */ - checkArgument(TABLE_PARAMETER_SAFE_VALUE_PATTERN.matcher(parameterValue).matches(), "Parameter value contains invalid characters: '%s'", parameterValue); - /* - * Thrift call `get_table_names_by_filter` may be translated by Metastore to a SQL query against Metastore database. - * Hive 2.3 on some databases uses CLOB for table parameter value column and some databases disallow `=` predicate over - * CLOB values. At the same time, they allow `LIKE` predicates over them. - */ - String filterWithEquals = HIVE_FILTER_FIELD_PARAMS + parameterKey + " = \"" + parameterValue + "\""; - String filterWithLike = HIVE_FILTER_FIELD_PARAMS + parameterKey + " LIKE \"" + parameterValue + "\""; - - return alternativeCall( - this::createMetastoreClient, - ThriftHiveMetastore::defaultIsValidExceptionalResponse, - chosenTableParamAlternative, - client -> client.getTableNamesByFilter(databaseName, filterWithEquals), - client -> client.getTableNamesByFilter(databaseName, filterWithLike)); - } - @Override public void createDatabase(Database database) { @@ -1950,78 +1832,6 @@ private static boolean containsAllPrivilege(Set requestedPri .anyMatch(privilege -> privilege.getPrivilege().equalsIgnoreCase("all")); } - @SafeVarargs - private static T alternativeCall( - ClientSupplier clientSupplier, - Predicate isValidExceptionalResponse, - AtomicInteger chosenAlternative, - Call... alternatives) - throws TException - { - checkArgument(alternatives.length > 0, "No alternatives"); - int chosen = chosenAlternative.get(); - checkArgument(chosen == Integer.MAX_VALUE || (0 <= chosen && chosen < alternatives.length), "Bad chosen alternative value: %s", chosen); - - if (chosen != Integer.MAX_VALUE) { - try (ThriftMetastoreClient client = clientSupplier.createMetastoreClient()) { - return alternatives[chosen].callOn(client); - } - } - - Exception firstException = null; - for (int i = 0; i < alternatives.length; i++) { - int position = i; - try (ThriftMetastoreClient client = clientSupplier.createMetastoreClient()) { - T result = alternatives[i].callOn(client); - chosenAlternative.updateAndGet(currentChosen -> Math.min(currentChosen, position)); - return result; - } - catch (TException | RuntimeException exception) { - if (isValidExceptionalResponse.test(exception)) { - // This is likely a valid response. We are not settling on an alternative yet. - // We will do it later when we get a more obviously valid response. - throw exception; - } - if (firstException == null) { - firstException = exception; - } - else if (firstException != exception) { - firstException.addSuppressed(exception); - } - } - } - - verifyNotNull(firstException); - propagateIfPossible(firstException, TException.class); - throw propagate(firstException); - } - - // TODO we should recognize exceptions which we suppress and try different alternative call - // this requires product tests with HDP 3 - private static boolean defaultIsValidExceptionalResponse(Exception exception) - { - if (exception instanceof NoSuchObjectException) { - return true; - } - - if (exception.toString().contains("AccessControlException")) { - // e.g. org.apache.hadoop.hive.metastore.api.MetaException: org.apache.hadoop.security.AccessControlException: Permission denied: ... - return true; - } - - return false; - } - - private static boolean isUnknownMethodExceptionalResponse(Exception exception) - { - if (!(exception instanceof TApplicationException)) { - return false; - } - - TApplicationException applicationException = (TApplicationException) exception; - return applicationException.getType() == UNKNOWN_METHOD; - } - private ThriftMetastoreClient createMetastoreClient() throws TException { @@ -2044,25 +1854,4 @@ private static RuntimeException propagate(Throwable throwable) throwIfUnchecked(throwable); throw new RuntimeException(throwable); } - - @FunctionalInterface - private interface ClientSupplier - { - ThriftMetastoreClient createMetastoreClient() - throws TException; - } - - @FunctionalInterface - private interface Call - { - T callOn(ThriftMetastoreClient client) - throws TException; - } - - @FunctionalInterface - private interface Call1 - { - void call(ThriftMetastoreClient client, A arg) - throws TException; - } } diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java index a89c92ced2b1..31b996a7e206 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftHiveMetastoreClient.java @@ -19,7 +19,9 @@ import io.trino.plugin.base.util.LoggingInvocationHandler.AirliftParameterNamesProvider; import io.trino.plugin.base.util.LoggingInvocationHandler.ParameterNamesProvider; import io.trino.plugin.hive.acid.AcidOperation; +import io.trino.plugin.hive.metastore.thrift.MetastoreSupportsDateStatistics.DateStatisticsSupport; import org.apache.hadoop.hive.common.ValidTxnList; +import org.apache.hadoop.hive.metastore.TableType; import org.apache.hadoop.hive.metastore.api.AbortTxnRequest; import org.apache.hadoop.hive.metastore.api.AddDynamicPartitions; import org.apache.hadoop.hive.metastore.api.AllocateTableWriteIdsRequest; @@ -45,13 +47,13 @@ import org.apache.hadoop.hive.metastore.api.GrantRevokePrivilegeRequest; import org.apache.hadoop.hive.metastore.api.GrantRevokeRoleRequest; import org.apache.hadoop.hive.metastore.api.GrantRevokeRoleResponse; -import org.apache.hadoop.hive.metastore.api.GrantRevokeType; import org.apache.hadoop.hive.metastore.api.HeartbeatTxnRangeRequest; import org.apache.hadoop.hive.metastore.api.HiveObjectPrivilege; import org.apache.hadoop.hive.metastore.api.HiveObjectRef; import org.apache.hadoop.hive.metastore.api.LockRequest; import org.apache.hadoop.hive.metastore.api.LockResponse; import org.apache.hadoop.hive.metastore.api.MetaException; +import org.apache.hadoop.hive.metastore.api.NoSuchObjectException; import org.apache.hadoop.hive.metastore.api.OpenTxnRequest; import org.apache.hadoop.hive.metastore.api.Partition; import org.apache.hadoop.hive.metastore.api.PartitionsStatsRequest; @@ -65,6 +67,7 @@ import org.apache.hadoop.hive.metastore.api.TxnToWriteId; import org.apache.hadoop.hive.metastore.api.UnlockRequest; import org.apache.hadoop.hive.metastore.txn.TxnUtils; +import org.apache.thrift.TApplicationException; import org.apache.thrift.TException; import org.apache.thrift.protocol.TBinaryProtocol; import org.apache.thrift.transport.TTransport; @@ -73,15 +76,29 @@ import java.util.List; import java.util.Map; import java.util.OptionalLong; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Predicate; +import java.util.regex.Pattern; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Throwables.propagateIfPossible; +import static com.google.common.base.Throwables.throwIfUnchecked; +import static com.google.common.base.Verify.verify; +import static com.google.common.base.Verify.verifyNotNull; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.reflect.Reflection.newProxy; +import static io.trino.plugin.hive.ViewReaderUtil.PRESTO_VIEW_FLAG; import static io.trino.plugin.hive.metastore.MetastoreUtil.adjustRowCount; +import static io.trino.plugin.hive.metastore.thrift.MetastoreSupportsDateStatistics.DateStatisticsSupport.NOT_SUPPORTED; +import static io.trino.plugin.hive.metastore.thrift.MetastoreSupportsDateStatistics.DateStatisticsSupport.SUPPORTED; +import static io.trino.plugin.hive.metastore.thrift.MetastoreSupportsDateStatistics.DateStatisticsSupport.UNKNOWN; import static java.lang.String.format; import static java.util.Objects.requireNonNull; import static org.apache.hadoop.hive.metastore.api.GrantRevokeType.GRANT; import static org.apache.hadoop.hive.metastore.api.GrantRevokeType.REVOKE; +import static org.apache.hadoop.hive.metastore.api.hive_metastoreConstants.HIVE_FILTER_FIELD_PARAMS; import static org.apache.hadoop.hive.metastore.txn.TxnUtils.createValidTxnWriteIdList; +import static org.apache.thrift.TApplicationException.UNKNOWN_METHOD; public class ThriftHiveMetastoreClient implements ThriftMetastoreClient @@ -90,11 +107,25 @@ public class ThriftHiveMetastoreClient private static final ParameterNamesProvider PARAMETER_NAMES_PROVIDER = new AirliftParameterNamesProvider(ThriftHiveMetastore.Iface.class, ThriftHiveMetastore.Client.class); + private static final Pattern TABLE_PARAMETER_SAFE_KEY_PATTERN = Pattern.compile("^[a-zA-Z_]+$"); + private static final Pattern TABLE_PARAMETER_SAFE_VALUE_PATTERN = Pattern.compile("^[a-zA-Z0-9\\s]*$"); + private final TTransport transport; protected final ThriftHiveMetastore.Iface client; private final String hostname; - public ThriftHiveMetastoreClient(TTransport transport, String hostname) + private final MetastoreSupportsDateStatistics metastoreSupportsDateStatistics = new MetastoreSupportsDateStatistics(); + private final AtomicInteger chosenGetTableAlternative = new AtomicInteger(Integer.MAX_VALUE); + private final AtomicInteger chosenTableParamAlternative = new AtomicInteger(Integer.MAX_VALUE); + private final AtomicInteger chosenGetAllViewsAlternative = new AtomicInteger(Integer.MAX_VALUE); + + public ThriftHiveMetastoreClient( + TTransport transport, + String hostname, + MetastoreSupportsDateStatistics metastoreSupportsDateStatistics, + AtomicInteger chosenGetTableAlternative, + AtomicInteger chosenTableParamAlternative, + AtomicInteger chosenGetAllViewsAlternative) { this.transport = requireNonNull(transport, "transport is null"); ThriftHiveMetastore.Client client = new ThriftHiveMetastore.Client(new TBinaryProtocol(transport)); @@ -135,17 +166,43 @@ public List getAllTables(String databaseName) } @Override - public List getTableNamesByFilter(String databaseName, String filter) + public List getAllViews(String databaseName) throws TException { - return client.get_table_names_by_filter(databaseName, filter, (short) -1); + return alternativeCall( + exception -> !isUnknownMethodExceptionalResponse(exception), + chosenGetAllViewsAlternative, + () -> client.get_tables_by_type(databaseName, ".*", TableType.VIRTUAL_VIEW.name()), + // fallback to enumerating Presto views only (Hive views can still be executed, but will be listed as tables and not views) + () -> getTablesWithParameter(databaseName, PRESTO_VIEW_FLAG, "true")); } @Override - public List getTableNamesByType(String databaseName, String tableType) + public List getTablesWithParameter(String databaseName, String parameterKey, String parameterValue) throws TException { - return client.get_tables_by_type(databaseName, ".*", tableType); + checkArgument(TABLE_PARAMETER_SAFE_KEY_PATTERN.matcher(parameterKey).matches(), "Parameter key contains invalid characters: '%s'", parameterKey); + /* + * The parameter value is restricted to have only alphanumeric characters so that it's safe + * to be used against HMS. When using with a LIKE operator, the HMS may want the parameter + * value to follow a Java regex pattern or a SQL pattern. And it's hard to predict the + * HMS's behavior from outside. Also, by restricting parameter values, we avoid the problem + * of how to quote them when passing within the filter string. + */ + checkArgument(TABLE_PARAMETER_SAFE_VALUE_PATTERN.matcher(parameterValue).matches(), "Parameter value contains invalid characters: '%s'", parameterValue); + /* + * Thrift call `get_table_names_by_filter` may be translated by Metastore to a SQL query against Metastore database. + * Hive 2.3 on some databases uses CLOB for table parameter value column and some databases disallow `=` predicate over + * CLOB values. At the same time, they allow `LIKE` predicates over them. + */ + String filterWithEquals = HIVE_FILTER_FIELD_PARAMS + parameterKey + " = \"" + parameterValue + "\""; + String filterWithLike = HIVE_FILTER_FIELD_PARAMS + parameterKey + " LIKE \"" + parameterValue + "\""; + + return alternativeCall( + ThriftHiveMetastoreClient::defaultIsValidExceptionalResponse, + chosenTableParamAlternative, + () -> client.get_table_names_by_filter(databaseName, filterWithEquals, (short) -1), + () -> client.get_table_names_by_filter(databaseName, filterWithLike, (short) -1)); } @Override @@ -194,11 +251,18 @@ public void alterTableWithEnvironmentContext(String databaseName, String tableNa public Table getTable(String databaseName, String tableName) throws TException { - return client.get_table(databaseName, tableName); + return alternativeCall( + ThriftHiveMetastoreClient::defaultIsValidExceptionalResponse, + chosenGetTableAlternative, + () -> { + GetTableRequest request = new GetTableRequest(databaseName, tableName); + request.setCapabilities(new ClientCapabilities(ImmutableList.of(ClientCapability.INSERT_ONLY_TABLES))); + return client.get_table_req(request).getTable(); + }, + () -> client.get_table(databaseName, tableName)); } - @Override - public Table getTableWithCapabilities(String databaseName, String tableName) + private Table getTableWithCapabilities(String databaseName, String tableName) throws TException { GetTableRequest request = new GetTableRequest(); @@ -227,9 +291,14 @@ public List getTableColumnStatistics(String databaseName, S public void setTableColumnStatistics(String databaseName, String tableName, List statistics) throws TException { - ColumnStatisticsDesc statisticsDescription = new ColumnStatisticsDesc(true, databaseName, tableName); - ColumnStatistics request = new ColumnStatistics(statisticsDescription, statistics); - client.update_table_column_statistics(request); + setColumnStatistics( + format("table %s.%s", databaseName, tableName), + statistics, + stats -> { + ColumnStatisticsDesc statisticsDescription = new ColumnStatisticsDesc(true, databaseName, tableName); + ColumnStatistics request = new ColumnStatistics(statisticsDescription, stats); + client.update_table_column_statistics(request); + }); } @Override @@ -251,10 +320,15 @@ public Map> getPartitionColumnStatistics(Strin public void setPartitionColumnStatistics(String databaseName, String tableName, String partitionName, List statistics) throws TException { - ColumnStatisticsDesc statisticsDescription = new ColumnStatisticsDesc(false, databaseName, tableName); - statisticsDescription.setPartName(partitionName); - ColumnStatistics request = new ColumnStatistics(statisticsDescription, statistics); - client.update_partition_column_statistics(request); + setColumnStatistics( + format("partition of table %s.%s", databaseName, tableName), + statistics, + stats -> { + ColumnStatisticsDesc statisticsDescription = new ColumnStatisticsDesc(false, databaseName, tableName); + statisticsDescription.setPartName(partitionName); + ColumnStatistics request = new ColumnStatistics(statisticsDescription, stats); + client.update_partition_column_statistics(request); + }); } @Override @@ -264,6 +338,54 @@ public void deletePartitionColumnStatistics(String databaseName, String tableNam client.delete_partition_column_statistics(databaseName, tableName, partitionName, columnName); } + private void setColumnStatistics(String objectName, List statistics, UnaryCall> saveColumnStatistics) + throws TException + { + boolean containsDateStatistics = statistics.stream().anyMatch(stats -> stats.getStatsData().isSetDateStats()); + + DateStatisticsSupport dateStatisticsSupported = this.metastoreSupportsDateStatistics.isSupported(); + if (containsDateStatistics && dateStatisticsSupported == NOT_SUPPORTED) { + log.debug("Skipping date statistics for %s because metastore does not support them", objectName); + statistics = statistics.stream() + .filter(stats -> !stats.getStatsData().isSetDateStats()) + .collect(toImmutableList()); + containsDateStatistics = false; + } + + if (!containsDateStatistics || dateStatisticsSupported == SUPPORTED) { + saveColumnStatistics.call(statistics); + return; + } + + List statisticsExceptDate = statistics.stream() + .filter(stats -> !stats.getStatsData().isSetDateStats()) + .collect(toImmutableList()); + + List dateStatistics = statistics.stream() + .filter(stats -> stats.getStatsData().isSetDateStats()) + .collect(toImmutableList()); + + verify(!dateStatistics.isEmpty() && dateStatisticsSupported == UNKNOWN); + + if (!statisticsExceptDate.isEmpty()) { + saveColumnStatistics.call(statisticsExceptDate); + } + + try { + saveColumnStatistics.call(dateStatistics); + } + catch (TException e) { + // When `dateStatistics.size() > 1` we expect something like "TApplicationException: Required field 'colName' is unset! Struct:ColumnStatisticsObj(colName:null, colType:null, statsData:null)" + // When `dateStatistics.size() == 1` we expect something like "TTransportException: java.net.SocketTimeoutException: Read timed out" + log.warn(e, "Failed to save date statistics for %s. Metastore might not support date statistics", objectName); + if (!statisticsExceptDate.isEmpty()) { + this.metastoreSupportsDateStatistics.failed(); + } + return; + } + this.metastoreSupportsDateStatistics.succeeded(); + } + @Override public List getPartitionNames(String databaseName, String tableName) throws TException @@ -388,7 +510,7 @@ private void createGrant(String role, String granteeName, PrincipalType granteeT throws TException { GrantRevokeRoleRequest request = new GrantRevokeRoleRequest(); - request.setRequestType(GrantRevokeType.GRANT); + request.setRequestType(GRANT); request.setRoleName(role); request.setPrincipalName(granteeName); request.setPrincipalType(granteeType); @@ -491,8 +613,8 @@ public void abortTransaction(long transactionId) public void sendTransactionHeartbeat(long transactionId) throws TException { - HeartbeatTxnRangeRequest rqst = new HeartbeatTxnRangeRequest(transactionId, transactionId); - client.heartbeat_txn_range(rqst); + HeartbeatTxnRangeRequest request = new HeartbeatTxnRangeRequest(transactionId, transactionId); + client.heartbeat_txn_range(request); } @Override @@ -522,7 +644,7 @@ public String getValidWriteIds(List tableList, long currentTransactionId { // Pass currentTxn as 0L to get the recent snapshot of valid transactions in Hive // Do not pass currentTransactionId instead as it will break Hive's listing of delta directories if major compaction - // deletes deleta directories for valid transactions that existed at the time transaction is opened + // deletes delta directories for valid transactions that existed at the time transaction is opened ValidTxnList validTransactions = TxnUtils.createValidReadTxnList(client.get_open_txns(), 0L); GetValidWriteIdsRequest request = new GetValidWriteIdsRequest(tableList, validTransactions.toString()); return createValidTxnWriteIdList( @@ -579,7 +701,7 @@ public void addDynamicPartitions(String dbName, String tableName, List p throws TException { AddDynamicPartitions request = new AddDynamicPartitions(transactionId, writeId, dbName, tableName, partitionNames); - request.setOperationType(operation.getMetastoreOperationType().get()); + request.setOperationType(operation.getMetastoreOperationType().orElseThrow()); client.add_dynamic_partitions(request); } @@ -595,4 +717,95 @@ public void alterTransactionalTable(Table table, long transactionId, long writeI request.setEnvironmentContext(environmentContext); client.alter_table_req(request); } + + @SafeVarargs + private static T alternativeCall( + Predicate isValidExceptionalResponse, + AtomicInteger chosenAlternative, + AlternativeCall... alternatives) + throws TException + { + checkArgument(alternatives.length > 0, "No alternatives"); + int chosen = chosenAlternative.get(); + checkArgument(chosen == Integer.MAX_VALUE || (0 <= chosen && chosen < alternatives.length), "Bad chosen alternative value: %s", chosen); + + if (chosen != Integer.MAX_VALUE) { + return alternatives[chosen].execute(); + } + + Exception firstException = null; + for (int i = 0; i < alternatives.length; i++) { + int position = i; + try { + T result = alternatives[i].execute(); + chosenAlternative.updateAndGet(currentChosen -> Math.min(currentChosen, position)); + return result; + } + catch (TException | RuntimeException exception) { + if (isValidExceptionalResponse.test(exception)) { + // This is likely a valid response. We are not settling on an alternative yet. + // We will do it later when we get a more obviously valid response. + throw exception; + } + if (firstException == null) { + firstException = exception; + } + else if (firstException != exception) { + firstException.addSuppressed(exception); + } + } + } + + verifyNotNull(firstException); + propagateIfPossible(firstException, TException.class); + throw propagate(firstException); + } + + // TODO we should recognize exceptions which we suppress and try different alternative call + // this requires product tests with HDP 3 + private static boolean defaultIsValidExceptionalResponse(Exception exception) + { + if (exception instanceof NoSuchObjectException) { + return true; + } + + if (exception.toString().contains("AccessControlException")) { + // e.g. org.apache.hadoop.hive.metastore.api.MetaException: org.apache.hadoop.security.AccessControlException: Permission denied: ... + return true; + } + + return false; + } + + private static boolean isUnknownMethodExceptionalResponse(Exception exception) + { + if (!(exception instanceof TApplicationException applicationException)) { + return false; + } + + return applicationException.getType() == UNKNOWN_METHOD; + } + + private static RuntimeException propagate(Throwable throwable) + { + if (throwable instanceof InterruptedException) { + Thread.currentThread().interrupt(); + } + throwIfUnchecked(throwable); + throw new RuntimeException(throwable); + } + + @FunctionalInterface + private interface AlternativeCall + { + T execute() + throws TException; + } + + @FunctionalInterface + private interface UnaryCall + { + void call(A arg) + throws TException; + } } diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreAuthenticationModule.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreAuthenticationModule.java index b220b01de744..89c8df7f72a1 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreAuthenticationModule.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreAuthenticationModule.java @@ -23,7 +23,6 @@ import io.trino.plugin.hive.authentication.HadoopAuthentication; import io.trino.plugin.hive.authentication.HiveMetastoreAuthentication; import io.trino.plugin.hive.authentication.MetastoreKerberosConfig; -import io.trino.plugin.hive.metastore.thrift.ThriftMetastoreAuthenticationConfig.ThriftMetastoreAuthenticationType; import static com.google.inject.Scopes.SINGLETON; import static io.airlift.configuration.ConfigBinder.configBinder; @@ -40,14 +39,10 @@ protected void setup(Binder binder) private Module getAuthenticationModule() { - ThriftMetastoreAuthenticationType type = buildConfigObject(ThriftMetastoreAuthenticationConfig.class).getAuthenticationType(); - switch (type) { - case NONE: - return new NoHiveMetastoreAuthenticationModule(); - case KERBEROS: - return new KerberosHiveMetastoreAuthenticationModule(); - } - throw new AssertionError("Unknown authentication type: " + type); + return switch (buildConfigObject(ThriftMetastoreAuthenticationConfig.class).getAuthenticationType()) { + case NONE -> new NoHiveMetastoreAuthenticationModule(); + case KERBEROS -> new KerberosHiveMetastoreAuthenticationModule(); + }; } public static class NoHiveMetastoreAuthenticationModule diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreClient.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreClient.java index 4ad306d54e77..c9e1807b4c1c 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreClient.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreClient.java @@ -51,10 +51,10 @@ Database getDatabase(String databaseName) List getAllTables(String databaseName) throws TException; - List getTableNamesByFilter(String databaseName, String filter) + List getAllViews(String databaseName) throws TException; - List getTableNamesByType(String databaseName, String tableType) + List getTablesWithParameter(String databaseName, String parameterKey, String parameterValue) throws TException; void createDatabase(Database database) @@ -78,9 +78,6 @@ void alterTableWithEnvironmentContext(String databaseName, String tableName, Tab Table getTable(String databaseName, String tableName) throws TException; - Table getTableWithCapabilities(String databaseName, String tableName) - throws TException; - List getFields(String databaseName, String tableName) throws TException; diff --git a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreUtil.java b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreUtil.java index 8cf7e8706cc5..ac3ef86446ba 100644 --- a/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreUtil.java +++ b/plugin/trino-hive/src/main/java/io/trino/plugin/hive/metastore/thrift/ThriftMetastoreUtil.java @@ -290,7 +290,7 @@ public static boolean isRoleEnabled(ConnectorIdentity identity, Function 0 && distinctValuesCount == 0) { distinctValuesCount = 1; } // the metastore may store an estimate, so the value stored may be higher than the total number of rows - if (distinctValuesCount > nonNullsCount) { - return nonNullsCount; - } - return distinctValuesCount; + return Math.min(distinctValuesCount, nonNullsCount); } public static Set fromRolePrincipalGrants(Collection grants) @@ -643,13 +640,10 @@ private static RoleGrant fromRolePrincipalGrant(RolePrincipalGrant grant) public static org.apache.hadoop.hive.metastore.api.PrincipalType fromTrinoPrincipalType(PrincipalType principalType) { - switch (principalType) { - case USER: - return org.apache.hadoop.hive.metastore.api.PrincipalType.USER; - case ROLE: - return org.apache.hadoop.hive.metastore.api.PrincipalType.ROLE; - } - throw new IllegalArgumentException("Unsupported principal type: " + principalType); + return switch (principalType) { + case USER -> org.apache.hadoop.hive.metastore.api.PrincipalType.USER; + case ROLE -> org.apache.hadoop.hive.metastore.api.PrincipalType.ROLE; + }; } public static PrincipalType fromMetastoreApiPrincipalType(org.apache.hadoop.hive.metastore.api.PrincipalType principalType) @@ -732,24 +726,17 @@ public static Set parsePrivilege(PrivilegeGrantInfo userGrant boolean grantOption = userGrant.isGrantOption(); String name = userGrant.getPrivilege().toUpperCase(ENGLISH); HivePrincipal grantor = new HivePrincipal(fromMetastoreApiPrincipalType(userGrant.getGrantorType()), userGrant.getGrantor()); - switch (name) { - case "ALL": - return Arrays.stream(HivePrivilegeInfo.HivePrivilege.values()) - .map(hivePrivilege -> new HivePrivilegeInfo(hivePrivilege, grantOption, grantor, grantee.orElse(grantor))) - .collect(toImmutableSet()); - case "SELECT": - return ImmutableSet.of(new HivePrivilegeInfo(SELECT, grantOption, grantor, grantee.orElse(grantor))); - case "INSERT": - return ImmutableSet.of(new HivePrivilegeInfo(INSERT, grantOption, grantor, grantee.orElse(grantor))); - case "UPDATE": - return ImmutableSet.of(new HivePrivilegeInfo(UPDATE, grantOption, grantor, grantee.orElse(grantor))); - case "DELETE": - return ImmutableSet.of(new HivePrivilegeInfo(DELETE, grantOption, grantor, grantee.orElse(grantor))); - case "OWNERSHIP": - return ImmutableSet.of(new HivePrivilegeInfo(OWNERSHIP, grantOption, grantor, grantee.orElse(grantor))); - default: - throw new IllegalArgumentException("Unsupported privilege name: " + name); - } + return switch (name) { + case "ALL" -> Arrays.stream(HivePrivilegeInfo.HivePrivilege.values()) + .map(hivePrivilege -> new HivePrivilegeInfo(hivePrivilege, grantOption, grantor, grantee.orElse(grantor))) + .collect(toImmutableSet()); + case "SELECT" -> ImmutableSet.of(new HivePrivilegeInfo(SELECT, grantOption, grantor, grantee.orElse(grantor))); + case "INSERT" -> ImmutableSet.of(new HivePrivilegeInfo(INSERT, grantOption, grantor, grantee.orElse(grantor))); + case "UPDATE" -> ImmutableSet.of(new HivePrivilegeInfo(UPDATE, grantOption, grantor, grantee.orElse(grantor))); + case "DELETE" -> ImmutableSet.of(new HivePrivilegeInfo(DELETE, grantOption, grantor, grantee.orElse(grantor))); + case "OWNERSHIP" -> ImmutableSet.of(new HivePrivilegeInfo(OWNERSHIP, grantOption, grantor, grantee.orElse(grantor))); + default -> throw new IllegalArgumentException("Unsupported privilege name: " + name); + }; } public static HiveBasicStatistics getHiveBasicStatistics(Map parameters) @@ -808,6 +795,7 @@ public static ColumnStatisticsObj createMetastoreColumnStatistics(String columnN case SHORT: case INT: case LONG: + case TIMESTAMP: return createLongStatistics(columnName, columnType, statistics); case FLOAT: case DOUBLE: @@ -818,8 +806,6 @@ public static ColumnStatisticsObj createMetastoreColumnStatistics(String columnN return createStringStatistics(columnName, columnType, statistics, rowCount); case DATE: return createDateStatistics(columnName, columnType, statistics); - case TIMESTAMP: - return createLongStatistics(columnName, columnType, statistics); case BINARY: return createBinaryStatistics(columnName, columnType, statistics, rowCount); case DECIMAL: @@ -971,7 +957,7 @@ public static Set getSupportedColumnStatistics(Type type) throw new IllegalArgumentException("Unsupported type: " + type); } - public static boolean isNumericType(Type type) + private static boolean isNumericType(Type type) { return type.equals(BIGINT) || type.equals(INTEGER) || type.equals(SMALLINT) || type.equals(TINYINT) || type.equals(DOUBLE) || type.equals(REAL) || diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/cache/TestCachingHiveMetastore.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/cache/TestCachingHiveMetastore.java index 901e60861444..bcdf07ac98c6 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/cache/TestCachingHiveMetastore.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/cache/TestCachingHiveMetastore.java @@ -174,14 +174,14 @@ public void testCachingWithOnlyPartitionsCacheEnabled() assertThatCachingWithDisabledPartitionCache() .whenExecuting(testedMetastore -> { Optional
table = testedMetastore.getTable(TEST_DATABASE, TEST_TABLE); - testedMetastore.getPartition(table.get(), TEST_PARTITION_VALUES1); + testedMetastore.getPartition(table.orElseThrow(), TEST_PARTITION_VALUES1); }) .omitsCacheForNumberOfOperations(1); assertThatCachingWithDisabledPartitionCache() .whenExecuting(testedMetastore -> { Optional
table = testedMetastore.getTable(TEST_DATABASE, TEST_TABLE); - testedMetastore.getPartitionsByNames(table.get(), TEST_PARTITION_VALUES1); + testedMetastore.getPartitionsByNames(table.orElseThrow(), TEST_PARTITION_VALUES1); }) .omitsCacheForNumberOfOperations(1); } @@ -279,14 +279,14 @@ public void testGetPartitionNames() { ImmutableList expectedPartitions = ImmutableList.of(TEST_PARTITION1, TEST_PARTITION2); assertEquals(mockClient.getAccessCount(), 0); - assertEquals(metastore.getPartitionNamesByFilter(TEST_DATABASE, TEST_TABLE, PARTITION_COLUMN_NAMES, TupleDomain.all()).get(), expectedPartitions); + assertEquals(metastore.getPartitionNamesByFilter(TEST_DATABASE, TEST_TABLE, PARTITION_COLUMN_NAMES, TupleDomain.all()).orElseThrow(), expectedPartitions); assertEquals(mockClient.getAccessCount(), 1); - assertEquals(metastore.getPartitionNamesByFilter(TEST_DATABASE, TEST_TABLE, PARTITION_COLUMN_NAMES, TupleDomain.all()).get(), expectedPartitions); + assertEquals(metastore.getPartitionNamesByFilter(TEST_DATABASE, TEST_TABLE, PARTITION_COLUMN_NAMES, TupleDomain.all()).orElseThrow(), expectedPartitions); assertEquals(mockClient.getAccessCount(), 1); metastore.flushCache(); - assertEquals(metastore.getPartitionNamesByFilter(TEST_DATABASE, TEST_TABLE, PARTITION_COLUMN_NAMES, TupleDomain.all()).get(), expectedPartitions); + assertEquals(metastore.getPartitionNamesByFilter(TEST_DATABASE, TEST_TABLE, PARTITION_COLUMN_NAMES, TupleDomain.all()).orElseThrow(), expectedPartitions); assertEquals(mockClient.getAccessCount(), 2); } @@ -296,8 +296,8 @@ public void testGetPartitionNames() *

* At the moment of writing, CachingHiveMetastore uses HivePartitionName for keys in partition cache. * HivePartitionName has a peculiar, semi- value-based equality. HivePartitionName may or may not be missing - * a name and it matters for bulk load, but it doesn't matter for single-partition load. - * Because of equality semantics, the cache keys may gets mixed during bulk load. + * a name, and it matters for bulk load, but it doesn't matter for single-partition load. + * Because of equality semantics, the cache keys may get mixed during bulk load. */ @Test public void testGetPartitionThenGetPartitions() @@ -367,16 +367,16 @@ public void testGetPartitionNamesByParts() ImmutableList expectedPartitions = ImmutableList.of(TEST_PARTITION1, TEST_PARTITION2); assertEquals(mockClient.getAccessCount(), 0); - assertEquals(metastore.getPartitionNamesByFilter(TEST_DATABASE, TEST_TABLE, PARTITION_COLUMN_NAMES, TupleDomain.all()).get(), expectedPartitions); + assertEquals(metastore.getPartitionNamesByFilter(TEST_DATABASE, TEST_TABLE, PARTITION_COLUMN_NAMES, TupleDomain.all()).orElseThrow(), expectedPartitions); assertEquals(mockClient.getAccessCount(), 1); - assertEquals(metastore.getPartitionNamesByFilter(TEST_DATABASE, TEST_TABLE, PARTITION_COLUMN_NAMES, TupleDomain.all()).get(), expectedPartitions); + assertEquals(metastore.getPartitionNamesByFilter(TEST_DATABASE, TEST_TABLE, PARTITION_COLUMN_NAMES, TupleDomain.all()).orElseThrow(), expectedPartitions); assertEquals(mockClient.getAccessCount(), 1); assertEquals(metastore.getPartitionFilterStats().getRequestCount(), 2); assertEquals(metastore.getPartitionFilterStats().getHitRate(), 0.5); metastore.flushCache(); - assertEquals(metastore.getPartitionNamesByFilter(TEST_DATABASE, TEST_TABLE, PARTITION_COLUMN_NAMES, TupleDomain.all()).get(), expectedPartitions); + assertEquals(metastore.getPartitionNamesByFilter(TEST_DATABASE, TEST_TABLE, PARTITION_COLUMN_NAMES, TupleDomain.all()).orElseThrow(), expectedPartitions); assertEquals(mockClient.getAccessCount(), 2); assertEquals(metastore.getPartitionFilterStats().getRequestCount(), 3); assertEquals(metastore.getPartitionFilterStats().getHitRate(), 1.0 / 3); @@ -423,14 +423,14 @@ public void testInvalidGetPartitionNamesByParts() public void testGetPartitionsByNames() { assertEquals(mockClient.getAccessCount(), 0); - Table table = metastore.getTable(TEST_DATABASE, TEST_TABLE).get(); + Table table = metastore.getTable(TEST_DATABASE, TEST_TABLE).orElseThrow(); assertEquals(mockClient.getAccessCount(), 1); // Select half of the available partitions and load them into the cache assertEquals(metastore.getPartitionsByNames(table, ImmutableList.of(TEST_PARTITION1)).size(), 1); assertEquals(mockClient.getAccessCount(), 2); - // Now select all of the partitions + // Now select all the partitions assertEquals(metastore.getPartitionsByNames(table, ImmutableList.of(TEST_PARTITION1, TEST_PARTITION2)).size(), 2); // There should be one more access to fetch the remaining partition assertEquals(mockClient.getAccessCount(), 3); @@ -486,7 +486,7 @@ public void testGetTableStatistics() { assertEquals(mockClient.getAccessCount(), 0); - Table table = metastore.getTable(TEST_DATABASE, TEST_TABLE).get(); + Table table = metastore.getTable(TEST_DATABASE, TEST_TABLE).orElseThrow(); assertEquals(mockClient.getAccessCount(), 1); assertEquals(metastore.getTableStatistics(table), TEST_STATS); @@ -504,10 +504,10 @@ public void testGetPartitionStatistics() { assertEquals(mockClient.getAccessCount(), 0); - Table table = metastore.getTable(TEST_DATABASE, TEST_TABLE).get(); + Table table = metastore.getTable(TEST_DATABASE, TEST_TABLE).orElseThrow(); assertEquals(mockClient.getAccessCount(), 1); - Partition partition = metastore.getPartition(table, TEST_PARTITION_VALUES1).get(); + Partition partition = metastore.getPartition(table, TEST_PARTITION_VALUES1).orElseThrow(); assertEquals(mockClient.getAccessCount(), 2); assertEquals(metastore.getPartitionStatistics(table, ImmutableList.of(partition)), ImmutableMap.of(TEST_PARTITION1, TEST_STATS)); @@ -530,7 +530,7 @@ public void testUpdatePartitionStatistics() HiveMetastoreClosure hiveMetastoreClosure = new HiveMetastoreClosure(metastore); - Table table = hiveMetastoreClosure.getTable(TEST_DATABASE, TEST_TABLE).get(); + Table table = hiveMetastoreClosure.getTable(TEST_DATABASE, TEST_TABLE).orElseThrow(); assertEquals(mockClient.getAccessCount(), 1); hiveMetastoreClosure.updatePartitionStatistics(table.getDatabaseName(), table.getTableName(), TEST_PARTITION1, identity()); @@ -540,7 +540,7 @@ public void testUpdatePartitionStatistics() @Test public void testInvalidGetPartitionsByNames() { - Table table = metastore.getTable(TEST_DATABASE, TEST_TABLE).get(); + Table table = metastore.getTable(TEST_DATABASE, TEST_TABLE).orElseThrow(); Map> partitionsByNames = metastore.getPartitionsByNames(table, ImmutableList.of(BAD_PARTITION)); assertEquals(partitionsByNames.size(), 1); Optional onlyElement = Iterables.getOnlyElement(partitionsByNames.values()); @@ -695,7 +695,7 @@ public Map> getPartitionsByNames(Table table, List TEST_PARTITION_VALUES1 = ImmutableList.of("testpartition1"); - public static final List TEST_PARTITION_VALUES2 = ImmutableList.of("testpartition2"); + private static final List TEST_PARTITION_VALUES2 = ImmutableList.of("testpartition2"); public static final List TEST_ROLES = ImmutableList.of("testrole"); - public static final List TEST_ROLE_GRANTS = ImmutableList.of( + private static final List TEST_ROLE_GRANTS = ImmutableList.of( new RolePrincipalGrant("role1", "user", USER, false, 0, "grantor1", USER), new RolePrincipalGrant("role2", "role1", ROLE, true, 0, "grantor2", ROLE)); public static final List PARTITION_COLUMN_NAMES = ImmutableList.of(TEST_COLUMN); @@ -118,6 +118,18 @@ public List getAllTables(String dbName) return ImmutableList.of(TEST_TABLE); } + @Override + public List getAllViews(String databaseName) + { + throw new UnsupportedOperationException(); + } + + @Override + public List getTablesWithParameter(String databaseName, String parameterKey, String parameterValue) + { + throw new UnsupportedOperationException(); + } + @Override public Database getDatabase(String name) throws TException @@ -158,12 +170,6 @@ public Table getTable(String dbName, String tableName) TableType.MANAGED_TABLE.name()); } - @Override - public Table getTableWithCapabilities(String databaseName, String tableName) - { - throw new UnsupportedOperationException(); - } - @Override public List getFields(String databaseName, String tableName) { @@ -232,18 +238,6 @@ public void deletePartitionColumnStatistics(String databaseName, String tableNam throw new UnsupportedOperationException(); } - @Override - public List getTableNamesByFilter(String databaseName, String filter) - { - throw new UnsupportedOperationException(); - } - - @Override - public List getTableNamesByType(String databaseName, String tableType) - { - throw new UnsupportedOperationException(); - } - @Override public List getPartitionNames(String dbName, String tableName) throws TException @@ -297,14 +291,26 @@ public List getPartitionsByNames(String dbName, String tableName, Lis if (!dbName.equals(TEST_DATABASE) || !tableName.equals(TEST_TABLE) || !ImmutableSet.of(TEST_PARTITION1, TEST_PARTITION2).containsAll(names)) { throw new NoSuchObjectException(); } - return Lists.transform(names, name -> { - try { - return new Partition(ImmutableList.copyOf(Warehouse.getPartValuesFromPartName(name)), TEST_DATABASE, TEST_TABLE, 0, 0, DEFAULT_STORAGE_DESCRIPTOR, ImmutableMap.of()); - } - catch (MetaException e) { - throw new RuntimeException(e); - } - }); + return names.stream() + .map(MockThriftMetastoreClient::getPartitionsByNamesUnchecked) + .collect(toImmutableList()); + } + + private static Partition getPartitionsByNamesUnchecked(String name) + { + try { + return new Partition( + ImmutableList.copyOf(Warehouse.getPartValuesFromPartName(name)), + TEST_DATABASE, + TEST_TABLE, + 0, + 0, + DEFAULT_STORAGE_DESCRIPTOR, + ImmutableMap.of()); + } + catch (MetaException e) { + throw new RuntimeException(e); + } } @Override @@ -422,7 +428,6 @@ public void revokeRole(String role, String granteeName, PrincipalType granteeTyp @Override public List listGrantedPrincipals(String role) - throws TException { throw new UnsupportedOperationException(); } @@ -481,7 +486,6 @@ public LockResponse checkLock(long lockId) @Override public void unlock(long lockId) - throws TException { throw new UnsupportedOperationException(); } @@ -500,28 +504,24 @@ public String getConfigValue(String name, String defaultValue) @Override public void updateTableWriteId(String dbName, String tableName, long transactionId, long writeId, OptionalLong rowCountChange) - throws TException { throw new UnsupportedOperationException(); } @Override public void alterPartitions(String dbName, String tableName, List partitions, long writeId) - throws TException { throw new UnsupportedOperationException(); } @Override public void addDynamicPartitions(String dbName, String tableName, List partitionNames, long transactionId, long writeId, AcidOperation operation) - throws TException { throw new UnsupportedOperationException(); } @Override public void alterTransactionalTable(Table table, long transactionId, long writeId, EnvironmentContext context) - throws TException { throw new UnsupportedOperationException(); } diff --git a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestStaticMetastoreLocator.java b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestStaticMetastoreLocator.java index 7375b72d2a75..9ff4b96cf9c0 100644 --- a/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestStaticMetastoreLocator.java +++ b/plugin/trino-hive/src/test/java/io/trino/plugin/hive/metastore/thrift/TestStaticMetastoreLocator.java @@ -220,7 +220,7 @@ public Table getTable(String dbName, String tableName) }; } - private void assertEqualHiveClient(ThriftMetastoreClient actual, ThriftMetastoreClient expected) + private static void assertEqualHiveClient(ThriftMetastoreClient actual, ThriftMetastoreClient expected) { if (actual instanceof FailureAwareThriftMetastoreClient) { actual = ((FailureAwareThriftMetastoreClient) actual).getDelegate(); diff --git a/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveMetastoreClientFactory.java b/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveMetastoreClientFactory.java index e0edffbc1012..c66395b2ed1f 100644 --- a/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveMetastoreClientFactory.java +++ b/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveMetastoreClientFactory.java @@ -16,17 +16,26 @@ import com.google.common.net.HostAndPort; import com.google.inject.Inject; import com.google.inject.name.Named; +import io.airlift.units.Duration; +import io.trino.plugin.hive.metastore.thrift.DefaultThriftMetastoreClientFactory; import io.trino.plugin.hive.metastore.thrift.NoHiveMetastoreAuthentication; -import io.trino.plugin.hive.metastore.thrift.ThriftHiveMetastoreClient; -import io.trino.plugin.hive.metastore.thrift.Transport; +import io.trino.plugin.hive.metastore.thrift.ThriftMetastoreClient; +import io.trino.plugin.hive.metastore.thrift.ThriftMetastoreClientFactory; import org.apache.thrift.TException; import java.net.URI; import java.util.Optional; +import static java.util.concurrent.TimeUnit.SECONDS; + public final class TestHiveMetastoreClientFactory { - private static final String LOCALHOST = "localhost"; + private final ThriftMetastoreClientFactory thriftMetastoreClientFactory = new DefaultThriftMetastoreClientFactory( + Optional.empty(), + Optional.empty(), + new Duration(10, SECONDS), + new NoHiveMetastoreAuthentication(), + "localhost"); @Inject @Named("databases.hive.metastore.host") @@ -36,17 +45,12 @@ public final class TestHiveMetastoreClientFactory @Named("databases.hive.metastore.port") private int metastorePort; - ThriftHiveMetastoreClient createMetastoreClient() + public ThriftMetastoreClient createMetastoreClient() throws TException { URI metastore = URI.create("thrift://" + metastoreHost + ":" + metastorePort); - return new ThriftHiveMetastoreClient( - Transport.create( - HostAndPort.fromParts(metastore.getHost(), metastore.getPort()), - Optional.empty(), - Optional.empty(), - 10000, - new NoHiveMetastoreAuthentication(), - Optional.empty()), LOCALHOST); + return thriftMetastoreClientFactory.create( + HostAndPort.fromParts(metastore.getHost(), metastore.getPort()), + Optional.empty()); } } diff --git a/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveTransactionalTable.java b/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveTransactionalTable.java index 128af14bf6d8..03603a450e0d 100644 --- a/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveTransactionalTable.java +++ b/testing/trino-product-tests/src/main/java/io/trino/tests/product/hive/TestHiveTransactionalTable.java @@ -18,7 +18,7 @@ import com.google.inject.Inject; import io.airlift.log.Logger; import io.airlift.units.Duration; -import io.trino.plugin.hive.metastore.thrift.ThriftHiveMetastoreClient; +import io.trino.plugin.hive.metastore.thrift.ThriftMetastoreClient; import io.trino.tempto.assertions.QueryAssert.Row; import io.trino.tempto.hadoop.hdfs.HdfsClient; import io.trino.tempto.query.QueryExecutor; @@ -1824,7 +1824,7 @@ public void testFilesForAbortedTransactionsIgnored() "STORED AS ORC " + "TBLPROPERTIES ('transactional'='true')"); - ThriftHiveMetastoreClient client = testHiveMetastoreClientFactory.createMetastoreClient(); + ThriftMetastoreClient client = testHiveMetastoreClientFactory.createMetastoreClient(); try { String selectFromOnePartitionsSql = "SELECT col FROM " + tableName + " ORDER BY COL";