diff --git a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/HiveMetastore.java b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/HiveMetastore.java index cef27b463c594..a2c22dbe12769 100644 --- a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/HiveMetastore.java +++ b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/HiveMetastore.java @@ -45,6 +45,8 @@ public interface HiveMetastore List getAllTables(String databaseName); + List getTablesWithParameter(String databaseName, String parameterKey, String parameterValue); + List getAllViews(String databaseName); void createDatabase(Database database); diff --git a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/RecordingHiveMetastore.java b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/RecordingHiveMetastore.java index 016cb2a250c4e..94e6b2470dd86 100644 --- a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/RecordingHiveMetastore.java +++ b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/RecordingHiveMetastore.java @@ -73,6 +73,7 @@ public class RecordingHiveMetastore private final Cache tableStatisticsCache; private final Cache, Map> partitionStatisticsCache; private final Cache> allTablesCache; + private final Cache> tablesWithParameterCache; private final Cache> allViewsCache; private final Cache> partitionCache; private final Cache>> partitionNamesCache; @@ -96,6 +97,7 @@ public RecordingHiveMetastore(@ForRecordingHiveMetastore HiveMetastore delegate, tableStatisticsCache = createCache(hiveConfig); partitionStatisticsCache = createCache(hiveConfig); allTablesCache = createCache(hiveConfig); + tablesWithParameterCache = createCache(hiveConfig); allViewsCache = createCache(hiveConfig); partitionCache = createCache(hiveConfig); partitionNamesCache = createCache(hiveConfig); @@ -123,6 +125,7 @@ void loadRecording() tableStatisticsCache.putAll(toMap(recording.getTableStatistics())); partitionStatisticsCache.putAll(toMap(recording.getPartitionStatistics())); allTablesCache.putAll(toMap(recording.getAllTables())); + tablesWithParameterCache.putAll(toMap(recording.getTablesWithParameter())); allViewsCache.putAll(toMap(recording.getAllViews())); partitionCache.putAll(toMap(recording.getPartitions())); partitionNamesCache.putAll(toMap(recording.getPartitionNames())); @@ -161,6 +164,7 @@ public void writeRecording() toPairs(tableStatisticsCache), toPairs(partitionStatisticsCache), toPairs(allTablesCache), + toPairs(tablesWithParameterCache), toPairs(allViewsCache), toPairs(partitionCache), toPairs(partitionNamesCache), @@ -253,6 +257,13 @@ public List getAllTables(String databaseName) return loadValue(allTablesCache, databaseName, () -> delegate.getAllTables(databaseName)); } + @Override + public List getTablesWithParameter(String databaseName, String parameterKey, String parameterValue) + { + TablesWithParameterCacheKey key = new TablesWithParameterCacheKey(databaseName, parameterKey, parameterValue); + return loadValue(tablesWithParameterCache, key, () -> delegate.getTablesWithParameter(databaseName, parameterKey, parameterValue)); + } + @Override public List getAllViews(String databaseName) { @@ -502,6 +513,7 @@ public static class Recording private final List> tableStatistics; private final List, Map>> partitionStatistics; private final List>> allTables; + private final List>> tablesWithParameter; private final List>> allViews; private final List>> partitions; private final List>>> partitionNames; @@ -520,6 +532,7 @@ public Recording( @JsonProperty("tableStatistics") List> tableStatistics, @JsonProperty("partitionStatistics") List, Map>> partitionStatistics, @JsonProperty("allTables") List>> allTables, + @JsonProperty("tablesWithParameter") List>> tablesWithParameter, @JsonProperty("allViews") List>> allViews, @JsonProperty("partitions") List>> partitions, @JsonProperty("partitionNames") List>>> partitionNames, @@ -536,6 +549,7 @@ public Recording( this.tableStatistics = tableStatistics; this.partitionStatistics = partitionStatistics; this.allTables = allTables; + this.tablesWithParameter = tablesWithParameter; this.allViews = allViews; this.partitions = partitions; this.partitionNames = partitionNames; @@ -569,6 +583,12 @@ public List>> getTables() return tables; } + @JsonProperty + public List>> getTablesWithParameter() + { + return tablesWithParameter; + } + @JsonProperty public List>> getSupportedColumnStatistics() { diff --git a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/TablesWithParameterCacheKey.java b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/TablesWithParameterCacheKey.java new file mode 100644 index 0000000000000..2cce508711b14 --- /dev/null +++ b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/TablesWithParameterCacheKey.java @@ -0,0 +1,80 @@ +/* + * 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.prestosql.plugin.hive.metastore; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonProperty; + +import javax.annotation.concurrent.Immutable; + +import java.util.Objects; + +@Immutable +public class TablesWithParameterCacheKey +{ + private final String databaseName; + private final String parameterKey; + private final String parameterValue; + + @JsonCreator + public TablesWithParameterCacheKey( + @JsonProperty("databaseName") String databaseName, + @JsonProperty("parameterKey") String parameterKey, + @JsonProperty("parameterValue") String parameterValue) + { + this.databaseName = databaseName; + this.parameterKey = parameterKey; + this.parameterValue = parameterValue; + } + + @JsonProperty + public String getDatabaseName() + { + return databaseName; + } + + @JsonProperty + public String getParameterKey() + { + return parameterKey; + } + + @JsonProperty + public String getParameterValue() + { + return parameterValue; + } + + @Override + public boolean equals(Object o) + { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + + TablesWithParameterCacheKey other = (TablesWithParameterCacheKey) o; + return Objects.equals(databaseName, other.databaseName) && + Objects.equals(parameterKey, other.parameterKey) && + Objects.equals(parameterValue, other.parameterValue); + } + + @Override + public int hashCode() + { + return Objects.hash(databaseName, parameterKey, parameterValue); + } +} diff --git a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/cache/CachingHiveMetastore.java b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/cache/CachingHiveMetastore.java index 62a9e6dda3550..eeb003cb9acb8 100644 --- a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/cache/CachingHiveMetastore.java +++ b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/cache/CachingHiveMetastore.java @@ -35,6 +35,7 @@ import io.prestosql.plugin.hive.metastore.PartitionWithStatistics; import io.prestosql.plugin.hive.metastore.PrincipalPrivileges; import io.prestosql.plugin.hive.metastore.Table; +import io.prestosql.plugin.hive.metastore.TablesWithParameterCacheKey; import io.prestosql.plugin.hive.metastore.UserTableKey; import io.prestosql.spi.PrestoException; import io.prestosql.spi.security.RoleGrant; @@ -86,6 +87,7 @@ public class CachingHiveMetastore private final LoadingCache> databaseNamesCache; private final LoadingCache> tableCache; private final LoadingCache> tableNamesCache; + private final LoadingCache> tablesWithParameterCache; private final LoadingCache tableStatisticsCache; private final LoadingCache partitionStatisticsCache; private final LoadingCache> viewNamesCache; @@ -141,6 +143,9 @@ private CachingHiveMetastore(HiveMetastore delegate, Executor executor, Optional tableNamesCache = newCacheBuilder(expiresAfterWriteMillis, refreshMills, maximumSize) .build(asyncReloading(CacheLoader.from(this::loadAllTables), executor)); + tablesWithParameterCache = newCacheBuilder(expiresAfterWriteMillis, refreshMills, maximumSize) + .build(asyncReloading(CacheLoader.from(this::loadTablesMatchingParameter), executor)); + tableStatisticsCache = newCacheBuilder(expiresAfterWriteMillis, refreshMills, maximumSize) .build(asyncReloading(new CacheLoader() { @@ -373,6 +378,18 @@ private List loadAllTables(String databaseName) return delegate.getAllTables(databaseName); } + @Override + public List getTablesWithParameter(String databaseName, String parameterKey, String parameterValue) + { + TablesWithParameterCacheKey key = new TablesWithParameterCacheKey(databaseName, parameterKey, parameterValue); + return get(tablesWithParameterCache, key); + } + + private List loadTablesMatchingParameter(TablesWithParameterCacheKey key) + { + return delegate.getTablesWithParameter(key.getDatabaseName(), key.getParameterKey(), key.getParameterValue()); + } + @Override public List getAllViews(String databaseName) { diff --git a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/file/FileHiveMetastore.java b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/file/FileHiveMetastore.java index a61f410f1cf06..c0bdcdbb8ba54 100644 --- a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/file/FileHiveMetastore.java +++ b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/file/FileHiveMetastore.java @@ -90,6 +90,7 @@ import static io.prestosql.plugin.hive.HiveErrorCode.HIVE_PARTITION_DROPPED_DURING_QUERY; import static io.prestosql.plugin.hive.HiveMetadata.TABLE_COMMENT; import static io.prestosql.plugin.hive.HivePartitionManager.extractPartitionValues; +import static io.prestosql.plugin.hive.HiveUtil.PRESTO_VIEW_FLAG; import static io.prestosql.plugin.hive.HiveUtil.toPartitionValues; import static io.prestosql.plugin.hive.metastore.HivePrivilegeInfo.HivePrivilege.OWNERSHIP; import static io.prestosql.plugin.hive.metastore.MetastoreUtil.makePartName; @@ -398,19 +399,26 @@ public synchronized List getAllTables(String databaseName) } @Override - public synchronized List getAllViews(String databaseName) + public synchronized List getTablesWithParameter(String databaseName, String parameterKey, String parameterValue) { + requireNonNull(parameterKey, "parameterKey is null"); + requireNonNull(parameterValue, "parameterValue is null"); + List tables = getAllTables(databaseName); - List views = tables.stream() + return tables.stream() .map(tableName -> getTable(databaseName, tableName)) .filter(Optional::isPresent) .map(Optional::get) - .filter(table -> table.getTableType().equals(VIRTUAL_VIEW.name())) + .filter(table -> parameterValue.equals(table.getParameters().get(parameterKey))) .map(Table::getTableName) .collect(toImmutableList()); + } - return views; + @Override + public synchronized List getAllViews(String databaseName) + { + return getTablesWithParameter(databaseName, PRESTO_VIEW_FLAG, "true"); } @Override diff --git a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/glue/GlueHiveMetastore.java b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/glue/GlueHiveMetastore.java index 65fcddd13c185..8ba3ca28009c5 100644 --- a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/glue/GlueHiveMetastore.java +++ b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/glue/GlueHiveMetastore.java @@ -365,6 +365,13 @@ public List getAllTables(String databaseName) } } + @Override + public synchronized List getTablesWithParameter(String databaseName, String parameterKey, String parameterValue) + { + // TODO + throw new UnsupportedOperationException("getTablesWithParameter for GlueHiveMetastore is not implemented"); + } + @Override public List getAllViews(String databaseName) { diff --git a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/BridgingHiveMetastore.java b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/BridgingHiveMetastore.java index 065d268f9c7ef..6fa1b76aa0d62 100644 --- a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/BridgingHiveMetastore.java +++ b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/BridgingHiveMetastore.java @@ -125,6 +125,12 @@ public List getAllTables(String databaseName) return delegate.getAllTables(databaseName); } + @Override + public List getTablesWithParameter(String databaseName, String parameterKey, String parameterValue) + { + return delegate.getTablesWithParameter(databaseName, parameterKey, parameterValue); + } + @Override public List getAllViews(String databaseName) { diff --git a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftHiveMetastore.java b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftHiveMetastore.java index af7f02a4c1d94..761cc6763c050 100644 --- a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftHiveMetastore.java +++ b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftHiveMetastore.java @@ -73,6 +73,7 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Function; +import java.util.regex.Pattern; import java.util.stream.Collectors; import static com.google.common.base.Preconditions.checkArgument; @@ -122,6 +123,9 @@ public class ThriftHiveMetastore private final AtomicInteger chosenGetTableAlternative = new AtomicInteger(Integer.MAX_VALUE); private final AtomicInteger chosenTableParamAlternative = new AtomicInteger(Integer.MAX_VALUE); + 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]*$"); + @Inject public ThriftHiveMetastore(MetastoreLocator metastoreLocator, ThriftHiveMetastoreConfig thriftConfig) { @@ -208,6 +212,27 @@ public List getAllTables(String databaseName) } } + @Override + public List getTablesWithParameter(String databaseName, String parameterKey, String parameterValue) + { + try { + return retry() + .stopOn(UnknownDBException.class) + .stopOnIllegalExceptions() + .run("getTablesWithParameter", stats.getGetTablesWithParameter().wrap( + () -> doGetTablesWithParameter(databaseName, parameterKey, parameterValue))); + } + catch (UnknownDBException e) { + return ImmutableList.of(); + } + catch (TException e) { + throw new PrestoException(HIVE_METASTORE_ERROR, e); + } + catch (Exception e) { + throw propagate(e); + } + } + @Override public Optional getTable(String databaseName, String tableName) { @@ -718,7 +743,7 @@ public List getAllViews(String databaseName) .stopOn(UnknownDBException.class) .stopOnIllegalExceptions() .run("getAllViews", stats.getGetAllViews().wrap( - () -> getPrestoViews(databaseName))); + () -> doGetTablesWithParameter(databaseName, PRESTO_VIEW_FLAG, "true"))); } catch (UnknownDBException e) { return ImmutableList.of(); @@ -731,16 +756,25 @@ public List getAllViews(String databaseName) } } - private List getPrestoViews(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 + PRESTO_VIEW_FLAG + " = \"true\""; - String filterWithLike = HIVE_FILTER_FIELD_PARAMS + PRESTO_VIEW_FLAG + " LIKE \"true\""; + String filterWithEquals = HIVE_FILTER_FIELD_PARAMS + parameterKey + " = \"" + parameterValue + "\""; + String filterWithLike = HIVE_FILTER_FIELD_PARAMS + parameterKey + " LIKE \"" + parameterValue + "\""; return alternativeCall( chosenTableParamAlternative, diff --git a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftMetastore.java b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftMetastore.java index c77f893205d73..a6c865e86c4b4 100644 --- a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftMetastore.java +++ b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftMetastore.java @@ -54,6 +54,8 @@ public interface ThriftMetastore List getAllTables(String databaseName); + List getTablesWithParameter(String databaseName, String parameterKey, String parameterValue); + List getAllViews(String databaseName); Optional getDatabase(String databaseName); diff --git a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftMetastoreStats.java b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftMetastoreStats.java index 0f45943d144db..a2044049d99f7 100644 --- a/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftMetastoreStats.java +++ b/presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/ThriftMetastoreStats.java @@ -21,6 +21,7 @@ public class ThriftMetastoreStats private final ThriftMetastoreApiStats getAllDatabases = new ThriftMetastoreApiStats(); private final ThriftMetastoreApiStats getDatabase = new ThriftMetastoreApiStats(); private final ThriftMetastoreApiStats getAllTables = new ThriftMetastoreApiStats(); + private final ThriftMetastoreApiStats getTablesWithParameter = new ThriftMetastoreApiStats(); private final ThriftMetastoreApiStats getAllViews = new ThriftMetastoreApiStats(); private final ThriftMetastoreApiStats getTable = new ThriftMetastoreApiStats(); private final ThriftMetastoreApiStats getFields = new ThriftMetastoreApiStats(); @@ -70,6 +71,13 @@ public ThriftMetastoreApiStats getGetAllTables() return getAllTables; } + @Managed + @Nested + public ThriftMetastoreApiStats getGetTablesWithParameter() + { + return getTablesWithParameter; + } + @Managed @Nested public ThriftMetastoreApiStats getGetAllViews() diff --git a/presto-hive/src/test/java/io/prestosql/plugin/hive/metastore/TestRecordingHiveMetastore.java b/presto-hive/src/test/java/io/prestosql/plugin/hive/metastore/TestRecordingHiveMetastore.java index 11eea97ac3dab..e32141313ddb0 100644 --- a/presto-hive/src/test/java/io/prestosql/plugin/hive/metastore/TestRecordingHiveMetastore.java +++ b/presto-hive/src/test/java/io/prestosql/plugin/hive/metastore/TestRecordingHiveMetastore.java @@ -131,6 +131,7 @@ private void validateMetadata(HiveMetastore hiveMetastore) assertEquals(hiveMetastore.getTableStatistics("database", "table"), PARTITION_STATISTICS); assertEquals(hiveMetastore.getPartitionStatistics("database", "table", ImmutableSet.of("value")), ImmutableMap.of("value", PARTITION_STATISTICS)); assertEquals(hiveMetastore.getAllTables("database"), ImmutableList.of("table")); + assertEquals(hiveMetastore.getTablesWithParameter("database", "param", "value3"), ImmutableList.of("table")); assertEquals(hiveMetastore.getAllViews("database"), ImmutableList.of()); assertEquals(hiveMetastore.getPartition("database", "table", ImmutableList.of("value")), Optional.of(PARTITION)); assertEquals(hiveMetastore.getPartitionNames("database", "table"), Optional.of(ImmutableList.of("value"))); @@ -210,6 +211,15 @@ public List getAllTables(String databaseName) return ImmutableList.of(); } + @Override + public List getTablesWithParameter(String databaseName, String parameterKey, String parameterValue) + { + if (databaseName.equals("database") && parameterKey.equals("param") && parameterValue.equals("value3")) { + return ImmutableList.of("table"); + } + return ImmutableList.of(); + } + @Override public List getAllViews(String databaseName) { diff --git a/presto-hive/src/test/java/io/prestosql/plugin/hive/metastore/UnimplementedHiveMetastore.java b/presto-hive/src/test/java/io/prestosql/plugin/hive/metastore/UnimplementedHiveMetastore.java index 17365b0aa38bf..8960e5f7d2964 100644 --- a/presto-hive/src/test/java/io/prestosql/plugin/hive/metastore/UnimplementedHiveMetastore.java +++ b/presto-hive/src/test/java/io/prestosql/plugin/hive/metastore/UnimplementedHiveMetastore.java @@ -82,6 +82,12 @@ public List getAllTables(String databaseName) throw new UnsupportedOperationException(); } + @Override + public List getTablesWithParameter(String databaseName, String parameterKey, String parameterValue) + { + throw new UnsupportedOperationException(); + } + @Override public List getAllViews(String databaseName) { diff --git a/presto-hive/src/test/java/io/prestosql/plugin/hive/metastore/thrift/InMemoryThriftMetastore.java b/presto-hive/src/test/java/io/prestosql/plugin/hive/metastore/thrift/InMemoryThriftMetastore.java index fcc3c3841b1af..7cc0720c1a95e 100644 --- a/presto-hive/src/test/java/io/prestosql/plugin/hive/metastore/thrift/InMemoryThriftMetastore.java +++ b/presto-hive/src/test/java/io/prestosql/plugin/hive/metastore/thrift/InMemoryThriftMetastore.java @@ -54,6 +54,7 @@ import static com.google.common.base.MoreObjects.toStringHelper; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.google.common.io.MoreFiles.deleteRecursively; import static com.google.common.io.RecursiveDeleteOption.ALLOW_INSECURE; import static io.prestosql.plugin.hive.HiveBasicStatistics.createEmptyStatistics; @@ -283,6 +284,19 @@ public synchronized List getAllTables(String databaseName) return tables.build(); } + @Override + public synchronized List getTablesWithParameter(String databaseName, String parameterKey, String parameterValue) + { + requireNonNull(parameterKey, "parameterKey is null"); + requireNonNull(parameterValue, "parameterValue is null"); + + return relations.entrySet().stream() + .filter(entry -> entry.getKey().getSchemaName().equals(databaseName) + && parameterValue.equals(entry.getValue().getParameters().get(parameterKey))) + .map(entry -> entry.getKey().getTableName()) + .collect(toImmutableList()); + } + @Override public synchronized List getAllViews(String databaseName) { diff --git a/presto-iceberg/README.md b/presto-iceberg/README.md index 0a1e8ae28ed14..53dcd6c35bcf3 100644 --- a/presto-iceberg/README.md +++ b/presto-iceberg/README.md @@ -73,8 +73,6 @@ as a time travel feature which lets you query your table's snapshot at a given t * Update the README to reflect the current status, and convert it to proper connector documentation before announcing the connector as ready for use. -* Fix table listing to skip non-Iceberg tables. This will need a new metastore method to list tables - filtered on a property name, similar to how view listing works in `ThriftHiveMetastore`. * Predicate pushdown is currently broken, which means delete is also broken. The code from the original `getTableLayouts()` implementation needs to be updated for `applyFilter()`. * All of the `HdfsContext` calls that use `/tmp` need to be fixed. diff --git a/presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergConnectorFactory.java b/presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergConnectorFactory.java index 0e8c53bb57bf4..abb11d7d9a158 100644 --- a/presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergConnectorFactory.java +++ b/presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergConnectorFactory.java @@ -24,6 +24,7 @@ import io.prestosql.plugin.hive.HiveCatalogName; import io.prestosql.plugin.hive.NodeVersion; import io.prestosql.plugin.hive.authentication.HiveAuthenticationModule; +import io.prestosql.plugin.hive.metastore.HiveMetastore; import io.prestosql.plugin.hive.metastore.HiveMetastoreModule; import io.prestosql.plugin.hive.s3.HiveS3Module; import io.prestosql.spi.NodeManager; @@ -48,10 +49,18 @@ import java.util.Optional; import static com.google.common.base.Throwables.throwIfUnchecked; +import static java.util.Objects.requireNonNull; public class IcebergConnectorFactory implements ConnectorFactory { + private final Optional metastore; + + public IcebergConnectorFactory(Optional metastore) + { + this.metastore = requireNonNull(metastore, "metastore is null"); + } + @Override public String getName() { @@ -76,7 +85,7 @@ public Connector create(String catalogName, Map config, Connecto new IcebergModule(), new HiveS3Module(), new HiveAuthenticationModule(), - new HiveMetastoreModule(Optional.empty()), + new HiveMetastoreModule(metastore), new MBeanServerModule(), binder -> { binder.bind(NodeVersion.class).toInstance(new NodeVersion(context.getNodeManager().getCurrentNode().getVersion())); diff --git a/presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergMetadata.java b/presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergMetadata.java index 0d9c1b33d4455..7c60dab1beaca 100644 --- a/presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergMetadata.java +++ b/presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergMetadata.java @@ -100,6 +100,8 @@ import static java.util.function.Function.identity; import static java.util.stream.Collectors.toList; import static java.util.stream.Collectors.toMap; +import static org.apache.iceberg.BaseMetastoreTableOperations.ICEBERG_TABLE_TYPE_VALUE; +import static org.apache.iceberg.BaseMetastoreTableOperations.TABLE_TYPE_PROP; import static org.apache.iceberg.TableMetadata.newTableMetadata; import static org.apache.iceberg.Transactions.createTableTransaction; @@ -172,11 +174,10 @@ public ConnectorTableMetadata getTableMetadata(ConnectorSession session, Connect @Override public List listTables(ConnectorSession session, Optional schemaName) { - // TODO: this should skip non-Iceberg tables return schemaName.map(Collections::singletonList) .orElseGet(metastore::getAllDatabases) .stream() - .flatMap(schema -> metastore.getAllTables(schema).stream() + .flatMap(schema -> metastore.getTablesWithParameter(schema, TABLE_TYPE_PROP, ICEBERG_TABLE_TYPE_VALUE).stream() .map(table -> new SchemaTableName(schema, table)) .collect(toList()) .stream()) diff --git a/presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergPlugin.java b/presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergPlugin.java index b4ea606b5fb19..3a4bda9cd208f 100644 --- a/presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergPlugin.java +++ b/presto-iceberg/src/main/java/io/prestosql/plugin/iceberg/IcebergPlugin.java @@ -14,15 +14,32 @@ package io.prestosql.plugin.iceberg; import com.google.common.collect.ImmutableList; +import io.prestosql.plugin.hive.metastore.HiveMetastore; import io.prestosql.spi.Plugin; import io.prestosql.spi.connector.ConnectorFactory; +import java.util.Optional; + +import static java.util.Objects.requireNonNull; + public class IcebergPlugin implements Plugin { + private final Optional metastore; + + public IcebergPlugin() + { + this(Optional.empty()); + } + + public IcebergPlugin(Optional metastore) + { + this.metastore = requireNonNull(metastore, "metastore is null"); + } + @Override public Iterable getConnectorFactories() { - return ImmutableList.of(new IcebergConnectorFactory()); + return ImmutableList.of(new IcebergConnectorFactory(metastore)); } } diff --git a/presto-iceberg/src/test/java/io/prestosql/plugin/iceberg/TestIcebergMetadataListing.java b/presto-iceberg/src/test/java/io/prestosql/plugin/iceberg/TestIcebergMetadataListing.java new file mode 100644 index 0000000000000..970268ced5e78 --- /dev/null +++ b/presto-iceberg/src/test/java/io/prestosql/plugin/iceberg/TestIcebergMetadataListing.java @@ -0,0 +1,125 @@ +/* + * 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.prestosql.plugin.iceberg; + +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import io.prestosql.Session; +import io.prestosql.plugin.hive.HdfsConfiguration; +import io.prestosql.plugin.hive.HdfsConfigurationInitializer; +import io.prestosql.plugin.hive.HdfsEnvironment; +import io.prestosql.plugin.hive.HiveConfig; +import io.prestosql.plugin.hive.HiveHdfsConfiguration; +import io.prestosql.plugin.hive.HivePlugin; +import io.prestosql.plugin.hive.authentication.NoHdfsAuthentication; +import io.prestosql.plugin.hive.metastore.HiveMetastore; +import io.prestosql.plugin.hive.metastore.file.FileHiveMetastore; +import io.prestosql.spi.security.Identity; +import io.prestosql.spi.security.SelectedRole; +import io.prestosql.tests.AbstractTestQueryFramework; +import io.prestosql.tests.DistributedQueryRunner; +import org.testng.annotations.AfterClass; +import org.testng.annotations.BeforeClass; +import org.testng.annotations.Test; + +import java.io.File; +import java.util.Optional; + +import static io.prestosql.spi.security.SelectedRole.Type.ROLE; +import static io.prestosql.testing.TestingSession.testSessionBuilder; +import static org.testng.Assert.assertEquals; + +public class TestIcebergMetadataListing + extends AbstractTestQueryFramework +{ + private static HiveMetastore metastore; + + public TestIcebergMetadataListing() + { + super(TestIcebergMetadataListing::createQueryRunner); + } + + private static DistributedQueryRunner createQueryRunner() + throws Exception + { + Session session = testSessionBuilder() + .setIdentity(new Identity( + "hive", + Optional.empty(), + ImmutableMap.of("hive", new SelectedRole(ROLE, Optional.of("admin"))))) + .build(); + DistributedQueryRunner queryRunner = DistributedQueryRunner.builder(session).build(); + + File baseDir = queryRunner.getCoordinator().getBaseDataDir().resolve("iceberg_data").toFile(); + + HiveConfig hiveConfig = new HiveConfig(); + HdfsConfiguration hdfsConfiguration = new HiveHdfsConfiguration(new HdfsConfigurationInitializer(hiveConfig), ImmutableSet.of()); + HdfsEnvironment hdfsEnvironment = new HdfsEnvironment(hdfsConfiguration, hiveConfig, new NoHdfsAuthentication()); + + metastore = new FileHiveMetastore(hdfsEnvironment, baseDir.toURI().toString(), "test"); + + queryRunner.installPlugin(new IcebergPlugin(Optional.of(metastore))); + queryRunner.createCatalog("iceberg", "iceberg"); + queryRunner.installPlugin(new HivePlugin("hive", Optional.of(metastore))); + queryRunner.createCatalog("hive", "hive", ImmutableMap.of("hive.security", "sql-standard")); + + return queryRunner; + } + + @BeforeClass + public void setUp() + { + assertQuerySucceeds("CREATE SCHEMA hive.test_schema"); + assertQuerySucceeds("CREATE TABLE iceberg.test_schema.iceberg_table1 (_string VARCHAR, _integer INTEGER)"); + assertQuerySucceeds("CREATE TABLE iceberg.test_schema.iceberg_table2 (_double DOUBLE) WITH (partitioning = ARRAY['_double'])"); + assertQuerySucceeds("CREATE TABLE hive.test_schema.hive_table (_double DOUBLE)"); + assertEquals(ImmutableSet.copyOf(metastore.getAllTables("test_schema")), ImmutableSet.of("iceberg_table1", "iceberg_table2", "hive_table")); + } + + @AfterClass(alwaysRun = true) + public void tearDown() + { + assertQuerySucceeds("DROP TABLE IF EXISTS hive.test_schema.hive_table"); + assertQuerySucceeds("DROP TABLE IF EXISTS iceberg.test_schema.iceberg_table2"); + assertQuerySucceeds("DROP TABLE IF EXISTS iceberg.test_schema.iceberg_table1"); + assertQuerySucceeds("DROP SCHEMA IF EXISTS hive.test_schema"); + } + + @Test + public void testTableListing() + { + assertQuery("SHOW TABLES FROM iceberg.test_schema", "VALUES 'iceberg_table1', 'iceberg_table2'"); + } + + @Test + public void testTableColumnListing() + { + // Verify information_schema.columns does not include columns from non-Iceberg tables + assertQuery("SELECT table_name, column_name FROM iceberg.information_schema.columns WHERE table_schema = 'test_schema'", + "VALUES ('iceberg_table1', '_string'), ('iceberg_table1', '_integer'), ('iceberg_table2', '_double')"); + } + + @Test + public void testTableDescribing() + { + assertQuery("DESCRIBE iceberg.test_schema.iceberg_table1", "VALUES ('_string', 'varchar', '', ''), ('_integer', 'integer', '', '')"); + } + + @Test + public void testTableValidation() + { + assertQuerySucceeds("SELECT * FROM iceberg.test_schema.iceberg_table1"); + assertQueryFails("SELECT * FROM iceberg.test_schema.hive_table", "Not an Iceberg table: test_schema.hive_table"); + } +}