You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by st...@apache.org on 2020/05/21 08:25:03 UTC
[impala] branch master updated: IMPALA-9669: Fix wrong
types/comments of loaded tables/views for GET_TABLES in LocalCatalog
This is an automated email from the ASF dual-hosted git repository.
stigahuang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
The following commit(s) were added to refs/heads/master by this push:
new 2e07d0c IMPALA-9669: Fix wrong types/comments of loaded tables/views for GET_TABLES in LocalCatalog
2e07d0c is described below
commit 2e07d0c07febf1d1ee9324708d543b792fb45b00
Author: stiga-huang <hu...@gmail.com>
AuthorDate: Mon Apr 20 15:11:33 2020 +0800
IMPALA-9669: Fix wrong types/comments of loaded tables/views for GET_TABLES in LocalCatalog
Coordinator can be in two modes: legacy mode or LocalCatalog mode.
Before IMPALA-8606, GET_TABLES required all tables to be loaded in
LocalCatalog-mode coordinator’s cache, which is a performance regression
compared to legacy mode coordinators. IMPALA-8606 changes the behavior
to only load the table names and create LocalIncompleteTable for each
table, which boosts the performance but results in all views being
returned with the default table type (TABLE). Besides this, all returned
comments are empty even if the table/view is loaded. This is a
regression since in legacy coordinators, loaded tables/views are shown
with correct table types and comments.
This is fixed by loading table types and comments along with table names
from catalogd. The cached list of table names of a DB is changed to be a
map containing the brief table metadata (name, type, comment). In case
of stale types or comments in the list, when loading the msTable of a
table, coordinator checks the type and comment and invalidates the table
list if any of them are stale.
Tests
- Add tests in test_hs2.test_get_tables and manually test it in
LocalCatalog mode.
- Run CORE tests.
Change-Id: I2180c603f061838347936f718cd4a0257d82e633
Reviewed-on: http://gerrit.cloudera.org:8080/15887
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
common/thrift/CatalogService.thrift | 18 ++++-
fe/src/main/java/org/apache/impala/catalog/Db.java | 18 ++++-
.../impala/catalog/local/CatalogdMetaProvider.java | 87 ++++++++++++++++++----
.../impala/catalog/local/DirectMetaProvider.java | 19 ++++-
.../org/apache/impala/catalog/local/LocalDb.java | 7 +-
.../impala/catalog/local/LocalIncompleteTable.java | 21 ++++--
.../apache/impala/catalog/local/MetaProvider.java | 4 +-
.../java/org/apache/impala/service/MetadataOp.java | 45 ++++++++---
.../impala/catalog/PartialCatalogInfoTest.java | 6 +-
.../catalog/local/CatalogdMetaProviderTest.java | 35 ++++++---
tests/hs2/test_hs2.py | 59 ++++++++++-----
tests/query_test/test_observability.py | 8 +-
12 files changed, 249 insertions(+), 78 deletions(-)
diff --git a/common/thrift/CatalogService.thrift b/common/thrift/CatalogService.thrift
index 662325a..0ab972d 100644
--- a/common/thrift/CatalogService.thrift
+++ b/common/thrift/CatalogService.thrift
@@ -389,13 +389,25 @@ struct TPartialTableInfo {
9: optional CatalogObjects.TValidWriteIdList valid_write_ids;
}
+struct TBriefTableMeta {
+ // Name of the table
+ 1: required string name
+
+ // HMS table type of the table: EXTERNAL_TABLE, MANAGED_TABLE, VIRTUAL_VIEW, etc.
+ // Unset if the table is unloaded.
+ 2: optional string msType
+
+ // Comment(remark) of the table. Unset if the table is unloaded.
+ 3: optional string comment
+}
+
// Selector for partial information about a Database.
struct TDbInfoSelector {
// The response should include the HMS Database object.
1: bool want_hms_database
- // The response should include the list of table names in the DB.
- 2: bool want_table_names
+ // The response should include TBriefTableMeta of tables in the DB.
+ 2: bool want_brief_meta_of_tables
// The response should include the list of function names in the DB.
3: bool want_function_names
@@ -404,7 +416,7 @@ struct TDbInfoSelector {
// Returned information about a Database, as selected by TDbInfoSelector.
struct TPartialDbInfo {
1: optional hive_metastore.Database hms_database
- 2: optional list<string> table_names
+ 2: optional list<TBriefTableMeta> brief_meta_of_tables
3: optional list<string> function_names
}
diff --git a/fe/src/main/java/org/apache/impala/catalog/Db.java b/fe/src/main/java/org/apache/impala/catalog/Db.java
index 8a465eb..b987008 100644
--- a/fe/src/main/java/org/apache/impala/catalog/Db.java
+++ b/fe/src/main/java/org/apache/impala/catalog/Db.java
@@ -32,6 +32,7 @@ import org.apache.impala.analysis.KuduPartitionParam;
import org.apache.impala.catalog.events.InFlightEvents;
import org.apache.impala.common.ImpalaException;
import org.apache.impala.common.ImpalaRuntimeException;
+import org.apache.impala.thrift.TBriefTableMeta;
import org.apache.impala.thrift.TCatalogObject;
import org.apache.impala.thrift.TCatalogObjectType;
import org.apache.impala.thrift.TDatabase;
@@ -53,6 +54,8 @@ import com.google.common.base.Preconditions;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
+import static org.apache.impala.service.MetadataOp.TABLE_COMMENT_KEY;
+
/**
* Internal representation of db-related metadata. Owned by Catalog instance.
* Not thread safe.
@@ -480,8 +483,19 @@ public class Db extends CatalogObjectImpl implements FeDb {
// instead to avoid this copy.
resp.db_info.hms_database = getMetaStoreDb().deepCopy();
}
- if (selector.want_table_names) {
- resp.db_info.table_names = getAllTableNames();
+ if (selector.want_brief_meta_of_tables) {
+ List<TBriefTableMeta> briefTableMetaList = Lists.newArrayListWithCapacity(
+ tableCache_.keySet().size());
+ for (Table tbl : tableCache_.getValues()) {
+ TBriefTableMeta meta = new TBriefTableMeta(tbl.getName());
+ org.apache.hadoop.hive.metastore.api.Table msTbl = tbl.getMetaStoreTable();
+ if (msTbl != null) {
+ meta.setMsType(msTbl.getTableType());
+ meta.setComment(msTbl.getParameters().get(TABLE_COMMENT_KEY));
+ }
+ briefTableMetaList.add(meta);
+ }
+ resp.db_info.brief_meta_of_tables = briefTableMetaList;
}
if (selector.want_function_names) {
resp.db_info.function_names = ImmutableList.copyOf(functions_.keySet());
diff --git a/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java b/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
index c66ccba..03aba1e 100644
--- a/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
+++ b/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
@@ -34,6 +34,9 @@ import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.atomic.AtomicReference;
+import com.google.common.collect.ImmutableCollection;
+import com.google.common.collect.ImmutableMap;
+import org.apache.commons.lang3.StringUtils;
import org.apache.hadoop.hive.metastore.api.ColumnStatisticsObj;
import org.apache.hadoop.hive.metastore.api.Database;
import org.apache.hadoop.hive.metastore.api.MetaException;
@@ -60,8 +63,10 @@ import org.apache.impala.common.InternalException;
import org.apache.impala.common.Pair;
import org.apache.impala.service.FeSupport;
import org.apache.impala.service.FrontendProfile;
+import org.apache.impala.service.MetadataOp;
import org.apache.impala.thrift.CatalogLookupStatus;
import org.apache.impala.thrift.TBackendGflags;
+import org.apache.impala.thrift.TBriefTableMeta;
import org.apache.impala.thrift.TCatalogInfoSelector;
import org.apache.impala.thrift.TCatalogObject;
import org.apache.impala.thrift.TCatalogObjectType;
@@ -109,6 +114,8 @@ import com.google.common.util.concurrent.Uninterruptibles;
import com.google.errorprone.annotations.Immutable;
import com.google.errorprone.annotations.concurrent.GuardedBy;
+import static org.apache.impala.service.MetadataOp.TABLE_COMMENT_KEY;
+
/**
* MetaProvider which fetches metadata in a granular fashion from the catalogd.
*
@@ -204,7 +211,7 @@ public class CatalogdMetaProvider implements MetaProvider {
private static final String CATALOG_FETCH_PREFIX = "CatalogFetch";
private static final String DB_LIST_STATS_CATEGORY = "DatabaseList";
private static final String DB_METADATA_STATS_CATEGORY = "Databases";
- private static final String TABLE_NAMES_STATS_CATEGORY = "TableNames";
+ private static final String TABLE_LIST_STATS_CATEGORY = "TableList";
private static final String TABLE_METADATA_CACHE_CATEGORY = "Tables";
private static final String PARTITION_LIST_STATS_CATEGORY = "PartitionLists";
private static final String PARTITIONS_STATS_CATEGORY = "Partitions";
@@ -446,7 +453,7 @@ public class CatalogdMetaProvider implements MetaProvider {
// around invalidation (IMPALA-7534). Namely, we have the following interleaving to
// worry about:
//
- // Thread 1: loadTableNames() misses and sends a request to fetch table names
+ // Thread 1: loadTableList() misses and sends a request to fetch table names
// Catalogd: sends a response with table list ['foo']
// Thread 2: creates a table 'bar'
// Catalogd: returns an invalidation for the table name list
@@ -638,22 +645,28 @@ public class CatalogdMetaProvider implements MetaProvider {
}
@Override
- public ImmutableList<String> loadTableNames(final String dbName)
+ public ImmutableCollection<TBriefTableMeta> loadTableList(final String dbName)
throws MetaException, UnknownDBException, TException {
- return loadWithCaching("table names for database " + dbName,
- TABLE_NAMES_STATS_CATEGORY,
- new DbCacheKey(dbName.toLowerCase(), DbCacheKey.DbInfoType.TABLE_NAMES),
- new Callable<ImmutableList<String>>() {
+ ImmutableMap<String, TBriefTableMeta> res = loadWithCaching(
+ "table list of database " + dbName, TABLE_LIST_STATS_CATEGORY,
+ new DbCacheKey(dbName.toLowerCase(), DbCacheKey.DbInfoType.TABLE_LIST),
+ new Callable<ImmutableMap<String, TBriefTableMeta>>() {
@Override
- public ImmutableList<String> call() throws Exception {
+ public ImmutableMap<String, TBriefTableMeta> call() throws Exception {
TGetPartialCatalogObjectRequest req = newReqForDb(dbName);
- req.db_info_selector.want_table_names = true;
+ req.db_info_selector.want_brief_meta_of_tables = true;
TGetPartialCatalogObjectResponse resp = sendRequest(req);
- checkResponse(resp.db_info != null && resp.db_info.table_names != null,
+ checkResponse(
+ resp.db_info != null && resp.db_info.brief_meta_of_tables != null,
req, "missing expected table names");
- return ImmutableList.copyOf(resp.db_info.table_names);
+ ImmutableMap.Builder<String, TBriefTableMeta> map = ImmutableMap.builder();
+ for (TBriefTableMeta meta : resp.db_info.brief_meta_of_tables) {
+ map.put(meta.getName(), meta);
+ }
+ return map.build();
}
});
+ return res.values();
}
private TGetPartialCatalogObjectRequest newReqForTable(String dbName,
@@ -708,9 +721,53 @@ public class CatalogdMetaProvider implements MetaProvider {
resp.table_info.valid_write_ids);
}
});
+ // The table list is populated based on tables in a given Db in catalogd. If a table
+ // is not loaded at the time in catalogd, we assume that the table type is "TABLE" and
+ // comment is empty (See MetadataOp.getTableType and MetadataOp.getTableComment()).
+ // However, after issuing this load request, if we find that our assumption was
+ // incorrect, we need to invalidate the table list immediately. The table list gets
+ // invalidated automatically when we receive the catalog topic-update but until then,
+ // all the getTables calls will show incorrect table type and comment for this table.
+ // TODO: consider removing this after IMPALA-9670.
+ invalidateStaleTableList(dbName.toLowerCase(), ref);
return Pair.create(ref.msTable_, (TableMetaRef)ref);
}
+ /**
+ * Invalidate table list of the given DB if it's loaded and it contains stale
+ * type/comment comparing to the newly loaded msTable.
+ */
+ private void invalidateStaleTableList(final String dbName,
+ final TableMetaRefImpl latestMeta) {
+ Preconditions.checkNotNull(latestMeta.msTable_, "loaded table should have a msTable");
+ DbCacheKey dbKey = new DbCacheKey(dbName, DbCacheKey.DbInfoType.TABLE_LIST);
+ Object dbValue = cache_.getIfPresent(dbKey);
+ if (dbValue instanceof ImmutableMap) {
+ TBriefTableMeta cachedMeta = ((ImmutableMap<String, TBriefTableMeta>) dbValue).get(
+ latestMeta.tableName_);
+ String latestComment = latestMeta.msTable_.getParameters().get(TABLE_COMMENT_KEY);
+ boolean hasStaleComment = !StringUtils.equals(latestComment, cachedMeta.comment);
+ // It's possible that the cached type is null (due to previously unloaded) and the
+ // latest msType we get is a table type (e.g. MANAGED_TABLE, EXTERNAL_TABLE).
+ // This is the most usual case and we don't consider the cached type is stale since
+ // they have the same result after applying MetadataOp.getImpalaTableType().
+ boolean hasStaleType = !StringUtils.equals(
+ MetadataOp.getImpalaTableType(cachedMeta.msType),
+ MetadataOp.getImpalaTableType(latestMeta.msTable_.getTableType()));
+ if (hasStaleType || hasStaleComment) {
+ List<String> invalidated = new ArrayList<>();
+ invalidateCacheForDb(dbName, ImmutableList.of(DbCacheKey.DbInfoType.TABLE_LIST),
+ invalidated);
+ if (!invalidated.isEmpty()) {
+ Preconditions.checkState(invalidated.size() == 1);
+ LOG.debug("Invalidated stale {} after loading table {}: hasStaleType={}, " +
+ "hasStaleComment={}",
+ invalidated.get(0), latestMeta.tableName_, hasStaleType, hasStaleComment);
+ }
+ }
+ }
+ }
+
@Override
public List<ColumnStatisticsObj> loadTableColumnStatistics(final TableMetaRef table,
List<String> colNames) throws TException {
@@ -1238,7 +1295,7 @@ public class CatalogdMetaProvider implements MetaProvider {
// DB, so we'll be coarse-grained here and invalidate the DB table list when
// any table change happens. It's relatively cheap to re-fetch this.
invalidateCacheForDb(obj.table.db_name,
- ImmutableList.of(DbCacheKey.DbInfoType.TABLE_NAMES),
+ ImmutableList.of(DbCacheKey.DbInfoType.TABLE_LIST),
invalidated);
break;
case FUNCTION:
@@ -1262,7 +1319,7 @@ public class CatalogdMetaProvider implements MetaProvider {
invalidated.add("list of database names");
}
invalidateCacheForDb(obj.db.db_name, ImmutableList.of(
- DbCacheKey.DbInfoType.TABLE_NAMES,
+ DbCacheKey.DbInfoType.TABLE_LIST,
DbCacheKey.DbInfoType.HMS_METADATA,
DbCacheKey.DbInfoType.FUNCTION_NAMES), invalidated);
break;
@@ -1596,8 +1653,8 @@ public class CatalogdMetaProvider implements MetaProvider {
static enum DbInfoType {
/** Cache the HMS Database object */
HMS_METADATA,
- /** Cache an ImmutableList<String> for table names within the DB */
- TABLE_NAMES,
+ /** Cache an ImmutableList<BriefTableMeta> for table names within the DB */
+ TABLE_LIST,
/** Cache an ImmutableList<String> for function names within the DB */
FUNCTION_NAMES
}
diff --git a/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java b/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
index 98527a7..ef4397d 100644
--- a/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
+++ b/fe/src/main/java/org/apache/impala/catalog/local/DirectMetaProvider.java
@@ -34,7 +34,7 @@ import org.apache.hadoop.hive.metastore.api.NoSuchObjectException;
import org.apache.hadoop.hive.metastore.api.Partition;
import org.apache.hadoop.hive.metastore.api.PrimaryKeysRequest;
import org.apache.hadoop.hive.metastore.api.Table;
-import org.apache.hadoop.hive.metastore.api.UnknownDBException;
+import org.apache.hadoop.hive.metastore.api.TableMeta;
import org.apache.impala.authorization.AuthorizationPolicy;
import org.apache.impala.catalog.FileMetadataLoader;
import org.apache.impala.catalog.Function;
@@ -46,6 +46,7 @@ import org.apache.impala.common.Pair;
import org.apache.impala.compat.MetastoreShim;
import org.apache.impala.service.BackendConfig;
import org.apache.impala.thrift.TBackendGflags;
+import org.apache.impala.thrift.TBriefTableMeta;
import org.apache.impala.thrift.TNetworkAddress;
import org.apache.impala.thrift.TValidWriteIdList;
import org.apache.impala.util.ListMap;
@@ -53,6 +54,7 @@ import org.apache.impala.util.MetaStoreUtil;
import org.apache.thrift.TException;
import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
@@ -112,10 +114,19 @@ class DirectMetaProvider implements MetaProvider {
}
@Override
- public ImmutableList<String> loadTableNames(String dbName)
- throws MetaException, UnknownDBException, TException {
+ public ImmutableCollection<TBriefTableMeta> loadTableList(String dbName)
+ throws TException {
try (MetaStoreClient c = msClientPool_.getClient()) {
- return ImmutableList.copyOf(c.getHiveClient().getAllTables(dbName));
+ List<TableMeta> tableMetaList = c.getHiveClient().getTableMeta(dbName,
+ /*tablePatterns=*/null, /*tableTypes=*/null);
+ ImmutableList.Builder<TBriefTableMeta> ret = ImmutableList.builder();
+ for (TableMeta meta : tableMetaList) {
+ TBriefTableMeta briefMeta = new TBriefTableMeta(meta.getTableName());
+ briefMeta.setMsType(meta.getTableType());
+ briefMeta.setComment(meta.getComments());
+ ret.add(briefMeta);
+ }
+ return ret.build();
}
}
diff --git a/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java b/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
index 4925f56..b972ca8 100644
--- a/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
+++ b/fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
@@ -35,6 +35,7 @@ import org.apache.impala.catalog.FeTable;
import org.apache.impala.catalog.Function;
import org.apache.impala.catalog.Function.CompareMode;
import org.apache.impala.catalog.TableLoadingException;
+import org.apache.impala.thrift.TBriefTableMeta;
import org.apache.impala.thrift.TDatabase;
import org.apache.impala.thrift.TFunctionCategory;
import org.apache.impala.util.FunctionUtils;
@@ -165,9 +166,9 @@ class LocalDb implements FeDb {
if (tables_ != null) return;
Map<String, FeTable> newMap = new HashMap<>();
try {
- List<String> names = catalog_.getMetaProvider().loadTableNames(name_);
- for (String tableName : names) {
- newMap.put(tableName.toLowerCase(), new LocalIncompleteTable(this, tableName));
+ MetaProvider metaProvider = catalog_.getMetaProvider();
+ for (TBriefTableMeta meta : metaProvider.loadTableList(name_)) {
+ newMap.put(meta.getName(), new LocalIncompleteTable(this, meta));
}
} catch (TException e) {
throw new LocalCatalogException(String.format(
diff --git a/fe/src/main/java/org/apache/impala/catalog/local/LocalIncompleteTable.java b/fe/src/main/java/org/apache/impala/catalog/local/LocalIncompleteTable.java
index 58d7f2a..282d6c8 100644
--- a/fe/src/main/java/org/apache/impala/catalog/local/LocalIncompleteTable.java
+++ b/fe/src/main/java/org/apache/impala/catalog/local/LocalIncompleteTable.java
@@ -18,21 +18,32 @@ package org.apache.impala.catalog.local;
import org.apache.impala.catalog.FeIncompleteTable;
import org.apache.impala.common.ImpalaException;
+import org.apache.impala.thrift.TBriefTableMeta;
import org.apache.impala.thrift.TTableDescriptor;
+import javax.annotation.Nullable;
import java.util.Set;
/**
- * FeTable implementation which represents an unloaded table in the LocalCatalog
- * implementation. This is used so operations that don't require the whole table meta,
- * like GetTables HiveServer2 operation, can get a quick response.
+ * FeTable implementation which represents a table with its brief metadata required by
+ * operations like GetTables HiveServer2 op that don't require the whole table meta.
*/
public class LocalIncompleteTable extends LocalTable implements FeIncompleteTable {
+ // These are null if the table is unloaded in catalogd.
+ @Nullable
+ private final String msTableType;
+ @Nullable
+ private final String tableComment;
- public LocalIncompleteTable(LocalDb db, String tblName) {
- super(db, tblName);
+ public LocalIncompleteTable(LocalDb db, TBriefTableMeta tableMeta) {
+ super(db, tableMeta.getName());
+ msTableType = tableMeta.getMsType();
+ tableComment = tableMeta.getComment();
}
+ public String getMsTableType() { return msTableType; }
+ public String getTableComment() { return tableComment; }
+
@Override
public ImpalaException getCause() { return null; }
diff --git a/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java b/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
index 6a60226..6d081c9 100644
--- a/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
+++ b/fe/src/main/java/org/apache/impala/catalog/local/MetaProvider.java
@@ -32,11 +32,13 @@ import org.apache.impala.catalog.Function;
import org.apache.impala.catalog.HdfsPartition.FileDescriptor;
import org.apache.impala.catalog.SqlConstraints;
import org.apache.impala.common.Pair;
+import org.apache.impala.thrift.TBriefTableMeta;
import org.apache.impala.thrift.TNetworkAddress;
import org.apache.impala.thrift.TValidWriteIdList;
import org.apache.impala.util.ListMap;
import org.apache.thrift.TException;
+import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableList;
import com.google.errorprone.annotations.Immutable;
@@ -63,7 +65,7 @@ public interface MetaProvider {
Database loadDb(String dbName) throws TException;
- ImmutableList<String> loadTableNames(String dbName)
+ ImmutableCollection<TBriefTableMeta> loadTableList(String dbName)
throws MetaException, UnknownDBException, TException;
Pair<Table, TableMetaRef> loadTable(String dbName, String tableName)
diff --git a/fe/src/main/java/org/apache/impala/service/MetadataOp.java b/fe/src/main/java/org/apache/impala/service/MetadataOp.java
index 78dcfb8..9b08e4a 100644
--- a/fe/src/main/java/org/apache/impala/service/MetadataOp.java
+++ b/fe/src/main/java/org/apache/impala/service/MetadataOp.java
@@ -25,6 +25,7 @@ import java.util.Set;
import org.apache.hadoop.hive.metastore.api.SQLForeignKey;
import org.apache.hadoop.hive.metastore.api.SQLPrimaryKey;
+import org.apache.hadoop.hive.metastore.api.Table;
import org.apache.hive.service.rpc.thrift.TGetCrossReferenceReq;
import org.apache.hive.service.rpc.thrift.TGetPrimaryKeysReq;
import org.apache.impala.analysis.StmtMetadataLoader;
@@ -44,6 +45,7 @@ import org.apache.impala.catalog.ScalarType;
import org.apache.impala.catalog.StructType;
import org.apache.impala.catalog.Type;
import org.apache.impala.catalog.local.InconsistentMetadataFetchException;
+import org.apache.impala.catalog.local.LocalIncompleteTable;
import org.apache.impala.common.ImpalaException;
import org.apache.impala.compat.MetastoreShim;
import org.apache.impala.thrift.TColumn;
@@ -60,6 +62,8 @@ import org.slf4j.LoggerFactory;
import com.google.common.base.Strings;
import com.google.common.collect.Lists;
+import javax.annotation.Nullable;
+
/**
* Metadata operation. It contains static methods to execute HiveServer2 metadata
* operations and return the results, result schema and an unique request id in
@@ -73,6 +77,7 @@ public class MetadataOp {
private static final TColumnValue EMPTY_COL_VAL = createTColumnValue("");
public static final String TABLE_TYPE_TABLE = "TABLE";
public static final String TABLE_TYPE_VIEW = "VIEW";
+ public static final String TABLE_COMMENT_KEY = "comment";
// Result set schema for each of the metadata operations.
private final static TResultSetMetadata GET_CATALOGS_MD = new TResultSetMetadata();
@@ -374,23 +379,16 @@ public class MetadataOp {
continue;
}
- String comment = null;
+ String comment = getTableComment(table);
+ String tableType = getTableType(table);
List<Column> columns = Lists.newArrayList();
List<SQLPrimaryKey> primaryKeys = Lists.newArrayList();
List<SQLForeignKey> foreignKeys = Lists.newArrayList();
// If the table is not yet loaded, the columns will be unknown. Add it
// to the set of missing tables.
- String tableType = TABLE_TYPE_TABLE;
if (!table.isLoaded() || table instanceof FeIncompleteTable) {
result.missingTbls.add(new TableName(db.getName(), tabName));
} else {
- if (table.getMetaStoreTable() != null) {
- comment = table.getMetaStoreTable().getParameters().get("comment");
- String tableTypeStr = table.getMetaStoreTable().getTableType() == null ?
- null : table.getMetaStoreTable().getTableType().toUpperCase();
- tableType = MetastoreShim.HMS_TO_IMPALA_TYPE
- .getOrDefault(tableTypeStr, TABLE_TYPE_TABLE);
- }
columns.addAll(fe.getColumns(table, columnPatternMatcher, user));
if (columnPatternMatcher != PatternMatcher.MATCHER_MATCH_NONE) {
// It is unnecessary to populate pk/fk information if the request does not
@@ -418,6 +416,35 @@ public class MetadataOp {
return result;
}
+ public static String getTableType(FeTable table) {
+ String msTableType;
+ if (table instanceof LocalIncompleteTable) {
+ // LocalIncompleteTable doesn't have a msTable object but it contains the HMS table
+ // type if the table is loaded in catalogd.
+ msTableType = ((LocalIncompleteTable) table).getMsTableType();
+ } else {
+ Table msTbl = table.getMetaStoreTable();
+ msTableType = (msTbl == null || msTbl.getTableType() == null) ?
+ null : msTbl.getTableType().toUpperCase();
+ }
+ return getImpalaTableType(msTableType);
+ }
+
+ public static String getImpalaTableType(@Nullable String msTableType) {
+ return MetastoreShim.HMS_TO_IMPALA_TYPE.getOrDefault(msTableType, TABLE_TYPE_TABLE);
+ }
+
+ @Nullable
+ public static String getTableComment(FeTable table) {
+ if (table instanceof LocalIncompleteTable) {
+ // LocalIncompleteTable doesn't have a msTable object but it contains the comment
+ // if the table is loaded in catalogd.
+ return ((LocalIncompleteTable) table).getTableComment();
+ }
+ Table msTbl = table.getMetaStoreTable();
+ return msTbl == null ? null : msTbl.getParameters().get(TABLE_COMMENT_KEY);
+ }
+
/**
* Executes the GetCatalogs HiveServer2 operation and returns TResultSet.
* Hive does not have a catalog concept. It always returns an empty result set.
diff --git a/fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoTest.java b/fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoTest.java
index ba677d1..810a370 100644
--- a/fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoTest.java
+++ b/fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoTest.java
@@ -39,6 +39,7 @@ import org.apache.impala.common.InternalException;
import org.apache.impala.service.BackendConfig;
import org.apache.impala.testutil.CatalogServiceTestCatalog;
import org.apache.impala.thrift.CatalogLookupStatus;
+import org.apache.impala.thrift.TBriefTableMeta;
import org.apache.impala.thrift.TCatalogInfoSelector;
import org.apache.impala.thrift.TCatalogObject;
import org.apache.impala.thrift.TCatalogObjectType;
@@ -131,11 +132,12 @@ public class PartialCatalogInfoTest {
req.object_desc.db = new TDatabase("functional");
req.db_info_selector = new TDbInfoSelector();
req.db_info_selector.want_hms_database = true;
- req.db_info_selector.want_table_names = true;
+ req.db_info_selector.want_brief_meta_of_tables = true;
TGetPartialCatalogObjectResponse resp = sendRequest(req);
assertTrue(resp.isSetObject_version_number());
assertEquals(resp.db_info.hms_database.getName(), "functional");
- assertTrue(resp.db_info.table_names.contains("alltypes"));
+ assertTrue(resp.db_info.brief_meta_of_tables.stream().map(TBriefTableMeta::getName)
+ .anyMatch("alltypes"::equals));
}
@Test
diff --git a/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java b/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java
index 17b84ae..c4dcf2d 100644
--- a/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java
+++ b/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java
@@ -41,11 +41,10 @@ import org.apache.impala.service.FeSupport;
import org.apache.impala.service.FrontendProfile;
import org.apache.impala.testutil.TestUtils;
import org.apache.impala.thrift.TBackendGflags;
+import org.apache.impala.thrift.TBriefTableMeta;
import org.apache.impala.thrift.TCatalogObject;
import org.apache.impala.thrift.TCatalogObjectType;
import org.apache.impala.thrift.TDatabase;
-import org.apache.impala.thrift.TFunction;
-import org.apache.impala.thrift.TFunctionName;
import org.apache.impala.thrift.TNetworkAddress;
import org.apache.impala.thrift.TRuntimeProfileNode;
import org.apache.impala.thrift.TTable;
@@ -57,6 +56,7 @@ import org.slf4j.LoggerFactory;
import com.google.common.base.Stopwatch;
import com.google.common.cache.CacheStats;
+import com.google.common.collect.ImmutableCollection;
import com.google.common.collect.ImmutableList;
public class CatalogdMetaProviderTest {
@@ -185,16 +185,18 @@ public class CatalogdMetaProviderTest {
CacheStats stats = diffStats();
assertEquals(1, stats.missCount());
- // ... and the table names for it.
- ImmutableList<String> tableNames = provider_.loadTableNames("functional");
+ // ... and the table list for it.
+ ImmutableCollection<TBriefTableMeta> tableList = provider_.loadTableList(
+ "functional");
stats = diffStats();
assertEquals(1, stats.missCount());
// Load them again, should hit cache.
Database dbHit = provider_.loadDb("functional");
assertEquals(db, dbHit);
- ImmutableList<String> tableNamesHit = provider_.loadTableNames("functional");
- assertEquals(tableNames, tableNamesHit);
+ ImmutableCollection<TBriefTableMeta> tableListHit = provider_.loadTableList(
+ "functional");
+ assertEquals(tableList, tableListHit);
stats = diffStats();
assertEquals(2, stats.hitCount());
@@ -208,8 +210,9 @@ public class CatalogdMetaProviderTest {
// Load another time, should miss cache.
Database dbMiss = provider_.loadDb("functional");
assertEquals(db, dbMiss);
- ImmutableList<String> tableNamesMiss = provider_.loadTableNames("functional");
- assertEquals(tableNames, tableNamesMiss);
+ ImmutableCollection<TBriefTableMeta> tableListMiss = provider_.loadTableList(
+ "functional");
+ assertEquals(tableList, tableListMiss);
stats = diffStats();
assertEquals(0, stats.hitCount());
assertEquals(2, stats.missCount());
@@ -222,6 +225,16 @@ public class CatalogdMetaProviderTest {
provider_.loadTable("functional", "alltypes");
CacheStats stats = diffStats();
assertEquals(1, stats.hitCount());
+ assertEquals(1, stats.missCount()); // missing the table list
+
+ // Load the table list then load the table again.
+ provider_.loadTableList("functional");
+ stats = diffStats();
+ assertEquals(0, stats.hitCount());
+ assertEquals(1, stats.missCount());
+ provider_.loadTable("functional", "alltypes");
+ stats = diffStats();
+ assertEquals(2, stats.hitCount()); // hit the table and the table list
assertEquals(0, stats.missCount());
// Invalidate it.
@@ -233,7 +246,7 @@ public class CatalogdMetaProviderTest {
provider_.loadTable("functional", "alltypes");
stats = diffStats();
assertEquals(0, stats.hitCount());
- assertEquals(1, stats.missCount());
+ assertEquals(2, stats.missCount()); // miss the table and the table list
}
@Test
@@ -356,7 +369,7 @@ public class CatalogdMetaProviderTest {
// should get a cache hit since dbName,tblName should be treated as case-insensitive
stats = diffStats();
assertEquals(1, stats.hitCount());
- assertEquals(0, stats.missCount());
+ assertEquals(1, stats.missCount()); // missing the table list
// Invalidate it.
TCatalogObject obj = new TCatalogObject(TCatalogObjectType.TABLE, 0);
@@ -367,7 +380,7 @@ public class CatalogdMetaProviderTest {
provider_.loadTable(dbName, tblName);
stats = diffStats();
assertEquals(0, stats.hitCount());
- assertEquals(1, stats.missCount());
+ assertEquals(2, stats.missCount()); // missing the table and the table list
}
private void testInvalidateDb(String dbName) throws Exception {
diff --git a/tests/hs2/test_hs2.py b/tests/hs2/test_hs2.py
index c80decd..38c100e 100644
--- a/tests/hs2/test_hs2.py
+++ b/tests/hs2/test_hs2.py
@@ -447,43 +447,64 @@ class TestHS2(HS2TestSuite):
"""Basic test for the GetTables() HS2 method. Needs to execute serially because
the test depends on controlling whether a table is loaded or not and other
concurrent tests loading or invalidating tables could interfere with it."""
- table = "__hs2_column_comments_test"
+ table = "__hs2_table_comment_test"
+ view = "__hs2_view_comment_test"
self.execute_query("use {0}".format(unique_database))
self.execute_query("drop table if exists {0}".format(table))
+ self.execute_query("drop view if exists {0}".format(view))
self.execute_query("""
create table {0} (a int comment 'column comment')
comment 'table comment'""".format(table))
+ self.execute_query("""
+ create view {0} comment 'view comment' as select * from {1}
+ """.format(view, table))
try:
req = TCLIService.TGetTablesReq()
req.sessionHandle = self.session_handle
req.schemaName = unique_database
- req.tableName = table
- # Execute the request twice, the first time with the table unloaded and the second
- # with it loaded.
+ # Execute the request twice, the first time with the table/view unloaded and the
+ # second with them loaded.
self.execute_query("invalidate metadata {0}".format(table))
+ self.execute_query("invalidate metadata {0}".format(view))
for i in range(2):
get_tables_resp = self.hs2_client.GetTables(req)
TestHS2.check_response(get_tables_resp)
fetch_results_resp = self._fetch_results(get_tables_resp.operationHandle, 100)
results = fetch_results_resp.results
- table_cat = results.columns[0].stringVal.values[0]
- table_schema = results.columns[1].stringVal.values[0]
- table_name = results.columns[2].stringVal.values[0]
- table_type = results.columns[3].stringVal.values[0]
- table_remarks = results.columns[4].stringVal.values[0]
- assert table_cat == ''
- assert table_schema == unique_database
- assert table_name == table
- assert table_type == "TABLE"
- if i == 0:
- assert table_remarks == ""
- else:
- if not cluster_properties.is_catalog_v2_cluster():
- assert table_remarks == "table comment"
- # Ensure the table is loaded for the second iteration.
+ # The returned results should only contain metadata for the table and the view.
+ # Note that results are in columnar format so we get #rows by the length of the
+ # first column.
+ assert len(results.columns[0].stringVal.values) == 2
+ for row_idx in range(2):
+ table_cat = results.columns[0].stringVal.values[row_idx]
+ table_schema = results.columns[1].stringVal.values[row_idx]
+ table_name = results.columns[2].stringVal.values[row_idx]
+ table_type = results.columns[3].stringVal.values[row_idx]
+ table_remarks = results.columns[4].stringVal.values[row_idx]
+ assert table_cat == ''
+ assert table_schema == unique_database
+ if i == 0:
+ # In the first iteration the table and view are unloaded, so their types are
+ # in the default value (TABLE) and their comments are empty.
+ assert table_name in (table, view)
+ assert table_type == "TABLE"
+ assert table_remarks == ""
+ else:
+ # In the second iteration the table and view are loaded. They should be shown
+ # with correct types and comments. Verify types and comments based on table
+ # names so we don't care the order in which the table/view are returned.
+ if table_name == table:
+ assert table_type == "TABLE"
+ assert table_remarks == "table comment"
+ else:
+ assert table_name == view
+ assert table_type == "VIEW"
+ assert table_remarks == "view comment"
+ # Ensure the table and view are loaded for the second iteration.
self.execute_query("describe {0}".format(table))
+ self.execute_query("describe {0}".format(view))
# Test that session secret is validated by this API.
invalid_req = TCLIService.TGetTablesReq()
diff --git a/tests/query_test/test_observability.py b/tests/query_test/test_observability.py
index 8f27c72..076f793 100644
--- a/tests/query_test/test_observability.py
+++ b/tests/query_test/test_observability.py
@@ -314,10 +314,10 @@ class TestObservability(ImpalaTestSuite):
r'CatalogFetch.RPCs.Requests',
r'CatalogFetch.RPCs.Time',
r'CatalogFetch.StorageLoad.Time',
- r'CatalogFetch.TableNames.Hits',
- r'CatalogFetch.TableNames.Misses',
- r'CatalogFetch.TableNames.Requests',
- r'CatalogFetch.TableNames.Time',
+ r'CatalogFetch.TableList.Hits',
+ r'CatalogFetch.TableList.Misses',
+ r'CatalogFetch.TableList.Requests',
+ r'CatalogFetch.TableList.Time',
r'CatalogFetch.Tables.Hits',
r'CatalogFetch.Tables.Misses',
r'CatalogFetch.Tables.Requests',