IMPALA-2070: include the database comment when showing databases

As part of change, refactor catalog and frontend functions to return
TDatabase/Db objects instead of just the string names of databases -
this required a lot of method/variable renamings.

Add test for creating database with comment. Modify existing tests
that assumed only a single column in SHOW DATABASES results.

Change-Id: I400e99b0aa60df24e7f051040074e2ab184163bf
Reviewed-on: http://gerrit.cloudera.org:8080/620
Reviewed-by: Tim Armstrong <tarmstrong@cloudera.com>
Tested-by: Internal Jenkins

be/src/catalog/catalog-server.cc

@@ -338,20 +338,20 @@ void CatalogServer::BuildTopicUpdates(const vector<TCatalogObject>& catalog_obje
 void CatalogServer::CatalogUrlCallback(const Webserver::ArgumentMap& args,
     Document* document) {
   TGetDbsResult get_dbs_result;
-  Status status = catalog_->GetDbNames(NULL, &get_dbs_result);
+  Status status = catalog_->GetDbs(NULL, &get_dbs_result);
   if (!status.ok()) {
     Value error(status.GetDetail().c_str(), document->GetAllocator());
       document->AddMember("error", error, document->GetAllocator());
     return;
   }
   Value databases(kArrayType);
-  BOOST_FOREACH(const string& db, get_dbs_result.dbs) {
+  BOOST_FOREACH(const TDatabase& db, get_dbs_result.dbs) {
     Value database(kObjectType);
-    Value str(db.c_str(), document->GetAllocator());
+    Value str(db.db_name.c_str(), document->GetAllocator());
     database.AddMember("name", str, document->GetAllocator());
 
     TGetTablesResult get_table_results;
-    Status status = catalog_->GetTableNames(db, NULL, &get_table_results);
+    Status status = catalog_->GetTableNames(db.db_name, NULL, &get_table_results);
     if (!status.ok()) {
       Value error(status.GetDetail().c_str(), document->GetAllocator());
       database.AddMember("error", error, document->GetAllocator());
@@ -361,7 +361,8 @@ void CatalogServer::CatalogUrlCallback(const Webserver::ArgumentMap& args,
     Value table_array(kArrayType);
     BOOST_FOREACH(const string& table, get_table_results.tables) {
       Value table_obj(kObjectType);
-      Value fq_name(Substitute("$0.$1", db, table).c_str(), document->GetAllocator());
+      Value fq_name(Substitute("$0.$1", db.db_name, table).c_str(),
+          document->GetAllocator());
       table_obj.AddMember("fqtn", fq_name, document->GetAllocator());
       Value table_name(table.c_str(), document->GetAllocator());
       table_obj.AddMember("name", table_name, document->GetAllocator());

be/src/catalog/catalog.cc

@@ -43,7 +43,7 @@ Catalog::Catalog() {
     {"execDdl", "([B)[B", &exec_ddl_id_},
     {"resetMetadata", "([B)[B", &reset_metadata_id_},
     {"getTableNames", "([B)[B", &get_table_names_id_},
-    {"getDbNames", "([B)[B", &get_db_names_id_},
+    {"getDbs", "([B)[B", &get_dbs_id_},
     {"getFunctions", "([B)[B", &get_functions_id_},
     {"checkUserSentryAdmin", "([B)V", &sentry_admin_check_id_},
     {"getCatalogObject", "([B)[B", &get_catalog_object_id_},
@@ -113,10 +113,10 @@ Status Catalog::UpdateCatalog(const TUpdateCatalogRequest& req,
   return JniUtil::CallJniMethod(catalog_, update_metastore_id_, req, resp);
 }
 
-Status Catalog::GetDbNames(const string* pattern, TGetDbsResult* db_names) {
+Status Catalog::GetDbs(const string* pattern, TGetDbsResult* dbs) {
   TGetDbsParams params;
   if (pattern != NULL) params.__set_pattern(*pattern);
-  return JniUtil::CallJniMethod(catalog_, get_db_names_id_, params, db_names);
+  return JniUtil::CallJniMethod(catalog_, get_dbs_id_, params, dbs);
 }
 
 Status Catalog::GetTableNames(const string& db, const string* pattern,

be/src/catalog/catalog.h

@@ -70,10 +70,10 @@ class Catalog {
   /// If pattern is NULL, match all databases otherwise match only those databases that
   /// match the pattern string. Patterns are "p1|p2|p3" where | denotes choice,
   /// and each pN may contain wildcards denoted by '*' which match all strings.
-  /// TODO: GetDbNames() and GetTableNames() can probably be scraped in favor of
+  /// TODO: GetDbs() and GetTableNames() can probably be scrapped in favor of
   /// GetAllCatalogObjects(). Consider removing them and moving everything to use
   /// that.
-  Status GetDbNames(const std::string* pattern, TGetDbsResult* table_names);
+  Status GetDbs(const std::string* pattern, TGetDbsResult* dbs);
 
   /// Returns all matching table names, per Hive's "SHOW TABLES <pattern>". Each
   /// table name returned is unqualified.
@@ -108,7 +108,7 @@ class Catalog {
   jmethodID get_catalog_object_id_;  // JniCatalog.getCatalogObject()
   jmethodID get_catalog_objects_id_;  // JniCatalog.getCatalogObjects()
   jmethodID get_catalog_version_id_;  // JniCatalog.getCatalogVersion()
-  jmethodID get_db_names_id_; // JniCatalog.getDbNames()
+  jmethodID get_dbs_id_; // JniCatalog.getDbs()
   jmethodID get_table_names_id_; // JniCatalog.getTableNames()
   jmethodID get_functions_id_; // JniCatalog.getFunctions()
   jmethodID prioritize_load_id_; // JniCatalog.prioritizeLoad()

be/src/service/frontend.cc

@@ -68,7 +68,7 @@ Frontend::Frontend() {
     {"describeDb", "([B)[B", &describe_db_id_},
     {"describeTable", "([B)[B", &describe_table_id_},
     {"showCreateTable", "([B)Ljava/lang/String;", &show_create_table_id_},
-    {"getDbNames", "([B)[B", &get_db_names_id_},
+    {"getDbs", "([B)[B", &get_dbs_id_},
     {"getDataSrcMetadata", "([B)[B", &get_data_src_metadata_id_},
     {"getStats", "([B)[B", &get_stats_id_},
     {"getFunctions", "([B)[B", &get_functions_id_},
@@ -144,12 +144,12 @@ Status Frontend::GetTableNames(const string& db, const string* pattern,
   return JniUtil::CallJniMethod(fe_, get_table_names_id_, params, table_names);
 }
 
-Status Frontend::GetDbNames(const string* pattern, const TSessionState* session,
-    TGetDbsResult* db_names) {
+Status Frontend::GetDbs(const string* pattern, const TSessionState* session,
+    TGetDbsResult* dbs) {
   TGetDbsParams params;
   if (pattern != NULL) params.__set_pattern(*pattern);
   if (session != NULL) params.__set_session(*session);
-  return JniUtil::CallJniMethod(fe_, get_db_names_id_, params, db_names);
+  return JniUtil::CallJniMethod(fe_, get_dbs_id_, params, dbs);
 }
 
 Status Frontend::GetDataSrcMetadata(const string* pattern,

be/src/service/frontend.h

@@ -73,8 +73,8 @@ class Frontend {
   /// be set to the user's current session. If this is an Impala internal request,
   /// the session should be set to NULL which will skip privilege checks returning all
   /// results.
-  Status GetDbNames(const std::string* pattern, const TSessionState* session,
-      TGetDbsResult* table_names);
+  Status GetDbs(const std::string* pattern, const TSessionState* session,
+      TGetDbsResult* dbs);
 
   /// Return all data sources matching the optional argument 'pattern'.
   /// If pattern is NULL, match all data source names otherwise match only those that
@@ -181,7 +181,7 @@ class Frontend {
   jmethodID describe_db_id_; // JniFrontend.describeDb
   jmethodID describe_table_id_; // JniFrontend.describeTable
   jmethodID show_create_table_id_; // JniFrontend.showCreateTable
-  jmethodID get_db_names_id_; // JniFrontend.getDbNames
+  jmethodID get_dbs_id_; // JniFrontend.getDbs
   jmethodID get_data_src_metadata_id_; // JniFrontend.getDataSrcMetadata
   jmethodID get_stats_id_; // JniFrontend.getTableStats
   jmethodID get_functions_id_; // JniFrontend.getFunctions

be/src/service/impala-server-callbacks.cc

@@ -380,7 +380,7 @@ void ImpalaServer::SessionsUrlCallback(const Webserver::ArgumentMap& args,
 void ImpalaServer::CatalogUrlCallback(const Webserver::ArgumentMap& args,
     Document* document) {
   TGetDbsResult get_dbs_result;
-  Status status = exec_env_->frontend()->GetDbNames(NULL, NULL, &get_dbs_result);
+  Status status = exec_env_->frontend()->GetDbs(NULL, NULL, &get_dbs_result);
   if (!status.ok()) {
     Value error(status.GetDetail().c_str(), document->GetAllocator());
     document->AddMember("error", error, document->GetAllocator());
@@ -388,14 +388,14 @@ void ImpalaServer::CatalogUrlCallback(const Webserver::ArgumentMap& args,
   }
 
   Value databases(kArrayType);
-  BOOST_FOREACH(const string& db, get_dbs_result.dbs) {
+  BOOST_FOREACH(const TDatabase& db, get_dbs_result.dbs) {
     Value database(kObjectType);
-    Value str(db.c_str(), document->GetAllocator());
+    Value str(db.db_name.c_str(), document->GetAllocator());
     database.AddMember("name", str, document->GetAllocator());
 
     TGetTablesResult get_table_results;
     Status status =
-        exec_env_->frontend()->GetTableNames(db, NULL, NULL, &get_table_results);
+        exec_env_->frontend()->GetTableNames(db.db_name, NULL, NULL, &get_table_results);
     if (!status.ok()) {
       Value error(status.GetDetail().c_str(), document->GetAllocator());
       database.AddMember("error", error, document->GetAllocator());
@@ -405,7 +405,8 @@ void ImpalaServer::CatalogUrlCallback(const Webserver::ArgumentMap& args,
     Value table_array(kArrayType);
     BOOST_FOREACH(const string& table, get_table_results.tables) {
       Value table_obj(kObjectType);
-      Value fq_name(Substitute("$0.$1", db, table).c_str(), document->GetAllocator());
+      Value fq_name(Substitute("$0.$1", db.db_name, table).c_str(),
+          document->GetAllocator());
       table_obj.AddMember("fqtn", fq_name, document->GetAllocator());
       Value table_name(table.c_str(), document->GetAllocator());
       table_obj.AddMember("name", table_name, document->GetAllocator());

be/src/service/impala-server.cc

@@ -935,13 +935,14 @@ Status ImpalaServer::UnregisterQuery(const TUniqueId& query_id, bool check_infli
 }
 
 Status ImpalaServer::UpdateCatalogMetrics() {
-  TGetDbsResult db_names;
-  RETURN_IF_ERROR(exec_env_->frontend()->GetDbNames(NULL, NULL, &db_names));
-  ImpaladMetrics::CATALOG_NUM_DBS->set_value(db_names.dbs.size());
+  TGetDbsResult dbs;
+  RETURN_IF_ERROR(exec_env_->frontend()->GetDbs(NULL, NULL, &dbs));
+  ImpaladMetrics::CATALOG_NUM_DBS->set_value(dbs.dbs.size());
   ImpaladMetrics::CATALOG_NUM_TABLES->set_value(0L);
-  BOOST_FOREACH(const string& db, db_names.dbs) {
+  BOOST_FOREACH(const TDatabase& db, dbs.dbs) {
     TGetTablesResult table_names;
-    RETURN_IF_ERROR(exec_env_->frontend()->GetTableNames(db, NULL, NULL, &table_names));
+    RETURN_IF_ERROR(exec_env_->frontend()->GetTableNames(db.db_name, NULL, NULL,
+        &table_names));
     ImpaladMetrics::CATALOG_NUM_TABLES->Increment(table_names.tables.size());
   }
 

be/src/service/query-exec-state.cc

@@ -227,12 +227,19 @@ Status ImpalaServer::QueryExecState::ExecLocalCatalogOp(
     }
     case TCatalogOpType::SHOW_DBS: {
       const TShowDbsParams* params = &catalog_op.show_dbs_params;
-      TGetDbsResult db_names;
+      TGetDbsResult dbs;
       const string* db_pattern =
           params->__isset.show_pattern ? (&params->show_pattern) : NULL;
       RETURN_IF_ERROR(
-          frontend_->GetDbNames(db_pattern, &query_ctx_.session, &db_names));
-      SetResultSet(db_names.dbs);
+          frontend_->GetDbs(db_pattern, &query_ctx_.session, &dbs));
+      vector<string> names, comments;
+      names.reserve(dbs.dbs.size());
+      comments.reserve(dbs.dbs.size());
+      BOOST_FOREACH(const TDatabase& db, dbs.dbs) {
+        names.push_back(db.db_name);
+        comments.push_back(db.metastore_db.description);
+      }
+      SetResultSet(names, comments);
       return Status::OK();
     }
     case TCatalogOpType::SHOW_DATA_SRCS: {

be/src/service/query-exec-state.h

@@ -65,7 +65,7 @@ class ImpalaServer::QueryExecState {
   Status Exec(TExecRequest* exec_request);
 
   /// Execute a HiveServer2 metadata operation
-  /// TODO: This is likely a superset of GetTableNames/GetDbNames. Coalesce these different
+  /// TODO: This is likely a superset of GetTableNames/GetDbs. Coalesce these different
   /// code paths.
   Status Exec(const TMetadataOpRequest& exec_request);
 

common/thrift/Frontend.thrift

@@ -84,8 +84,7 @@ struct TGetTablesResult {
   1: list<string> tables
 }
 
-// Arguments to getDbNames, which returns a list of dbs that match an optional
-// pattern
+// Arguments to getDbs, which returns a list of dbs that match an optional pattern
 struct TGetDbsParams {
   // If not set, match every database
   1: optional string pattern
@@ -96,9 +95,9 @@ struct TGetDbsParams {
   2: optional ImpalaInternalService.TSessionState session
 }
 
-// getDbNames returns a list of database names
+// getDbs returns a list of databases
 struct TGetDbsResult {
-  1: list<string> dbs
+  1: list<CatalogObjects.TDatabase> dbs
 }
 
 // Arguments to getDataSrcsNames, which returns a list of data sources that match an

fe/src/main/java/com/cloudera/impala/catalog/BuiltinsDb.java

@@ -15,8 +15,11 @@
 package com.cloudera.impala.catalog;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.Map;
 
+import org.apache.hadoop.hive.metastore.api.Database;
+
 import com.cloudera.impala.analysis.ArithmeticExpr;
 import com.cloudera.impala.analysis.BinaryPredicate;
 import com.cloudera.impala.analysis.CaseExpr;
@@ -31,7 +34,7 @@ import com.google.common.collect.Lists;
 
 public class BuiltinsDb extends Db {
   public BuiltinsDb(String name, Catalog catalog) {
-    super(name, catalog, null);
+    super(name, catalog, createMetastoreDb(name));
     setIsSystemDb(true);
     initBuiltins();
   }
@@ -55,6 +58,13 @@ public class BuiltinsDb extends Db {
     ScalarBuiltins.initBuiltins(this);
   }
 
+  private static final String BUILTINS_DB_COMMENT = "System database for Impala builtin functions";
+
+  private static Database createMetastoreDb(String name) {
+    return new org.apache.hadoop.hive.metastore.api.Database(name,
+        BUILTINS_DB_COMMENT, "", Collections.<String,String>emptyMap());
+  }
+
   private static final Map<Type, String> SAMPLE_INIT_SYMBOL =
       ImmutableMap.<Type, String>builder()
         .put(Type.BOOLEAN,

fe/src/main/java/com/cloudera/impala/catalog/Catalog.java

@@ -15,6 +15,7 @@
 package com.cloudera.impala.catalog;
 
 import java.util.Collections;
+import java.util.Comparator;
 import java.util.List;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.atomic.AtomicReference;
@@ -128,13 +129,13 @@ public abstract class Catalog {
   }
 
   /**
-   * Returns a list of databases that match dbPattern. See filterStringsByPattern
-   * for details of the pattern match semantics.
+   * Returns databases that match dbPattern. See filterStringsByPattern for details of
+   * the pattern match semantics.
    *
    * dbPattern may be null (and thus matches everything).
    */
-  public List<String> getDbNames(String dbPattern) {
-    return filterStringsByPattern(dbCache_.get().keySet(), dbPattern);
+  public List<Db> getDbs(String dbPattern) {
+    return filterCatalogObjectsByPattern(dbCache_.get().values(), dbPattern);
   }
 
   /**
@@ -239,12 +240,7 @@ public abstract class Catalog {
    * pattern may be null (and thus matches everything).
    */
   public List<DataSource> getDataSources(String pattern) {
-    List<String> names = filterStringsByPattern(dataSources_.keySet(), pattern);
-    List<DataSource> dataSources = Lists.newArrayListWithCapacity(names.size());
-    for (String name: names) {
-      dataSources.add(dataSources_.get(name));
-    }
-    return dataSources;
+    return filterCatalogObjectsByPattern(dataSources_.getValues(), pattern);
   }
 
   /**
@@ -341,11 +337,12 @@ public abstract class Catalog {
    */
   private List<String> filterStringsByPattern(Iterable<String> candidates,
       String matchPattern) {
-    List<String> filtered = Lists.newArrayList();
+    List<String> filtered;
     if (matchPattern == null) {
       filtered = Lists.newArrayList(candidates);
     } else {
       PatternMatcher matcher = PatternMatcher.createHivePatternMatcher(matchPattern);
+      filtered = Lists.newArrayList();
       for (String candidate: candidates) {
         if (matcher.matches(candidate)) filtered.add(candidate);
       }
@@ -354,6 +351,41 @@ public abstract class Catalog {
     return filtered;
   }
 
+  private static class CatalogObjectOrder implements Comparator<CatalogObject> {
+    @Override
+    public int compare(CatalogObject o1, CatalogObject o2) {
+      return String.CASE_INSENSITIVE_ORDER.compare(o1.getName(), o2.getName());
+    }
+  }
+
+  private static final CatalogObjectOrder CATALOG_OBJECT_ORDER = new CatalogObjectOrder();
+
+  /**
+   * Implement Hive's pattern-matching semantics for SHOW statements. The only
+   * metacharacters are '*' which matches any string of characters, and '|'
+   * which denotes choice.  Doing the work here saves loading tables or
+   * databases from the metastore (which Hive would do if we passed the call
+   * through to the metastore client).
+   *
+   * If matchPattern is null, all strings are considered to match. If it is the
+   * empty string, no strings match.
+   */
+  private <T extends CatalogObject> List<T> filterCatalogObjectsByPattern(
+      Iterable<? extends T> candidates, String matchPattern) {
+    List<T> filtered;
+    if (matchPattern == null) {
+      filtered = Lists.newArrayList(candidates);
+    } else {
+      PatternMatcher matcher = PatternMatcher.createHivePatternMatcher(matchPattern);
+      filtered = Lists.newArrayList();
+      for (T candidate: candidates) {
+        if (matcher.matches(candidate.getName())) filtered.add(candidate);
+      }
+    }
+    Collections.sort(filtered, CATALOG_OBJECT_ORDER);
+    return filtered;
+  }
+
   /**
    * Returns the HdfsPartition object for the given dbName/tableName and partition spec.
    * This will trigger a metadata load if the table metadata is not yet cached.

fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java

@@ -30,6 +30,7 @@ import org.apache.hadoop.fs.RemoteIterator;
 import org.apache.hadoop.hdfs.DistributedFileSystem;
 import org.apache.hadoop.hdfs.protocol.CachePoolEntry;
 import org.apache.hadoop.hdfs.protocol.CachePoolInfo;
+import org.apache.hadoop.hive.metastore.api.Database;
 import org.apache.hadoop.hive.metastore.api.UnknownDBException;
 import org.apache.log4j.Logger;
 import org.apache.thrift.TException;
@@ -219,13 +220,7 @@ public class CatalogServiceCatalog extends Catalog {
     // of all items in the catalog.
     catalogLock_.readLock().lock();
     try {
-      for (String dbName: getDbNames(null)) {
-        Db db = getDb(dbName);
-        if (db == null) {
-          LOG.error("Database: " + dbName + " was expected to be in the catalog " +
-              "cache. Skipping database and all child objects for this update.");
-          continue;
-        }
+      for (Db db: getDbs(null)) {
         TCatalogObject catalogDb = new TCatalogObject(TCatalogObjectType.DATABASE,
             db.getCatalogVersion());
         catalogDb.setDb(db.toThrift());
@@ -249,12 +244,12 @@ public class CatalogServiceCatalog extends Catalog {
               catalogTbl.setTable(tbl.toThrift());
             } catch (Exception e) {
               LOG.debug(String.format("Error calling toThrift() on table %s.%s: %s",
-                  dbName, tblName, e.getMessage()), e);
+                  db.getName(), tblName, e.getMessage()), e);
               continue;
             }
             catalogTbl.setCatalog_version(tbl.getCatalogVersion());
           } else {
-            catalogTbl.setTable(new TTable(dbName, tblName));
+            catalogTbl.setTable(new TTable(db.getName(), tblName));
           }
           resp.addToObjects(catalogTbl);
         }

fe/src/main/java/com/cloudera/impala/service/Frontend.java

@@ -263,7 +263,8 @@ public class Frontend {
       ddl.op_type = TCatalogOpType.SHOW_DBS;
       ddl.setShow_dbs_params(analysis.getShowDbsStmt().toThrift());
       metadata.setColumns(Arrays.asList(
-          new TColumn("name", Type.STRING.toThrift())));
+          new TColumn("name", Type.STRING.toThrift()),
+          new TColumn("comment", Type.STRING.toThrift())));
     } else if (analysis.isShowDataSrcsStmt()) {
       ddl.op_type = TCatalogOpType.SHOW_DATA_SRCS;
       ddl.setShow_data_srcs_params(analysis.getShowDataSrcsStmt().toThrift());
@@ -626,27 +627,34 @@ public class Frontend {
   }
 
   /**
-   * Returns all database names that match the pattern and
+   * Returns all databases that match the pattern and
    * are accessible to the given user. If pattern is null, matches all dbs.
    */
-  public List<String> getDbNames(String dbPattern, User user) {
-    List<String> dbNames = impaladCatalog_.getDbNames(dbPattern);
+  public List<Db> getDbs(String dbPattern, User user) {
+    List<Db> dbs = impaladCatalog_.getDbs(dbPattern);
     // If authorization is enabled, filter out the databases the user does not
     // have permissions on.
     if (authzConfig_.isEnabled()) {
-      Iterator<String> iter = dbNames.iterator();
+      Iterator<Db> iter = dbs.iterator();
       while (iter.hasNext()) {
-        String dbName = iter.next();
-        // Default DB should always be shown.
-        if (dbName.toLowerCase().equals(Catalog.DEFAULT_DB.toLowerCase())) continue;
-        PrivilegeRequest request = new PrivilegeRequestBuilder()
-            .any().onAnyTable(dbName).toRequest();
-        if (!authzChecker_.get().hasAccess(user, request)) {
-          iter.remove();
-        }
+        Db db = iter.next();
+        if (!isAccessibleToUser(db, user)) iter.remove();
       }
     }
-    return dbNames;
+    return dbs;
+  }
+
+  /**
+   * Check whether database is accessible to given user.
+   */
+  private boolean isAccessibleToUser(Db db, User user) {
+    if (db.getName().toLowerCase().equals(Catalog.DEFAULT_DB.toLowerCase())) {
+      // Default DB should always be shown.
+      return true;
+    }
+    PrivilegeRequest request = new PrivilegeRequestBuilder()
+        .any().onAnyTable(db.getName()).toRequest();
+    return authzChecker_.get().hasAccess(user, request);
   }
 
   /**

fe/src/main/java/com/cloudera/impala/service/JniCatalog.java

@@ -28,11 +28,13 @@ import com.cloudera.impala.authorization.SentryConfig;
 import com.cloudera.impala.authorization.User;
 import com.cloudera.impala.catalog.CatalogException;
 import com.cloudera.impala.catalog.CatalogServiceCatalog;
+import com.cloudera.impala.catalog.Db;
 import com.cloudera.impala.catalog.Function;
 import com.cloudera.impala.common.ImpalaException;
 import com.cloudera.impala.common.InternalException;
 import com.cloudera.impala.common.JniUtil;
 import com.cloudera.impala.thrift.TCatalogObject;
+import com.cloudera.impala.thrift.TDatabase;
 import com.cloudera.impala.thrift.TDdlExecRequest;
 import com.cloudera.impala.thrift.TFunction;
 import com.cloudera.impala.thrift.TGetAllCatalogObjectsResponse;
@@ -51,6 +53,7 @@ import com.cloudera.impala.thrift.TUpdateCatalogRequest;
 import com.cloudera.impala.util.GlogAppender;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
+import com.google.common.collect.Lists;
 
 /**
  * JNI-callable interface for the CatalogService. The main point is to serialize
@@ -143,16 +146,19 @@ public class JniCatalog {
   }
 
   /**
-   * Returns a list of table names matching an optional pattern.
-   * The argument is a serialized TGetTablesParams object.
-   * The return type is a serialized TGetTablesResult object.
+   * Returns a list of databases matching an optional pattern.
+   * The argument is a serialized TGetDbParams object.
+   * The return type is a serialized TGetDbResult object.
    */
-  public byte[] getDbNames(byte[] thriftGetTablesParams) throws ImpalaException,
+  public byte[] getDbs(byte[] thriftGetTablesParams) throws ImpalaException,
       TException {
     TGetDbsParams params = new TGetDbsParams();
     JniUtil.deserializeThrift(protocolFactory_, params, thriftGetTablesParams);
+    List<Db> dbs = catalog_.getDbs(null);
     TGetDbsResult result = new TGetDbsResult();
-    result.setDbs(catalog_.getDbNames(null));
+    List<TDatabase> tDbs = Lists.newArrayListWithCapacity(dbs.size());
+    for (Db db: dbs) tDbs.add(db.toThrift());
+    result.setDbs(tDbs);
     TSerializer serializer = new TSerializer(protocolFactory_);
     return serializer.serialize(result);
   }

fe/src/main/java/com/cloudera/impala/service/JniFrontend.java

@@ -51,6 +51,7 @@ import com.cloudera.impala.authorization.AuthorizationConfig;
 import com.cloudera.impala.authorization.ImpalaInternalAdminUser;
 import com.cloudera.impala.authorization.User;
 import com.cloudera.impala.catalog.DataSource;
+import com.cloudera.impala.catalog.Db;
 import com.cloudera.impala.catalog.Function;
 import com.cloudera.impala.catalog.Role;
 import com.cloudera.impala.common.FileSystemUtil;
@@ -58,6 +59,7 @@ import com.cloudera.impala.common.ImpalaException;
 import com.cloudera.impala.common.InternalException;
 import com.cloudera.impala.common.JniUtil;
 import com.cloudera.impala.thrift.TCatalogObject;
+import com.cloudera.impala.thrift.TDatabase;
 import com.cloudera.impala.thrift.TDescribeDbParams;
 import com.cloudera.impala.thrift.TDescribeResult;
 import com.cloudera.impala.thrift.TDescribeTableParams;
@@ -258,23 +260,23 @@ public class JniFrontend {
   }
 
   /**
-   * Returns a list of table names matching an optional pattern.
-   * The argument is a serialized TGetTablesParams object.
-   * The return type is a serialised TGetTablesResult object.
-   * @see Frontend#getTableNames
+   * Returns a list of databases matching an optional pattern.
+   * The argument is a serialized TGetDbParams object.
+   * The return type is a serialised TGetDbResult object.
+   * @see Frontend#getDbParams
    */
-  public byte[] getDbNames(byte[] thriftGetTablesParams) throws ImpalaException {
+  public byte[] getDbs(byte[] thriftGetTablesParams) throws ImpalaException {
     TGetDbsParams params = new TGetDbsParams();
     JniUtil.deserializeThrift(protocolFactory_, params, thriftGetTablesParams);
     // If the session was not set it indicates this is an internal Impala call.
     User user = params.isSetSession() ?
         new User(TSessionStateUtil.getEffectiveUser(params.getSession())) :
         ImpalaInternalAdminUser.getInstance();
-    List<String> dbs = frontend_.getDbNames(params.pattern, user);
-
+    List<Db> dbs = frontend_.getDbs(params.pattern, user);
     TGetDbsResult result = new TGetDbsResult();
-    result.setDbs(dbs);
-
+    List<TDatabase> tDbs = Lists.newArrayListWithCapacity(dbs.size());
+    for (Db db: dbs) tDbs.add(db.toThrift());
+    result.setDbs(tDbs);
     TSerializer serializer = new TSerializer(protocolFactory_);
     try {
       return serializer.serialize(result);

fe/src/main/java/com/cloudera/impala/service/MetadataOp.java

@@ -259,12 +259,8 @@ public class MetadataOp {
     PatternMatcher fnPattern = PatternMatcher.createJdbcPatternMatcher(functionName);
 
     ImpaladCatalog catalog = fe.getCatalog();
-    for (String dbName: fe.getDbNames(null, user)) {
-      if (!schemaPattern.matches(dbName)) continue;
-
-      Db db = catalog.getDb(dbName);
-      if (db == null) continue;
-
+    for (Db db: fe.getDbs(null, user)) {
+      if (!schemaPattern.matches(db.getName())) continue;
       if (functionName != null) {
         // Get function metadata
         List<Function> fns = db.getFunctions(null, fnPattern);
@@ -278,7 +274,7 @@ public class MetadataOp {
           tableList.add(tabName);
           Table table = null;
           try {
-            table = catalog.getTable(dbName, tabName);
+            table = catalog.getTable(db.getName(), tabName);
           } catch (TableLoadingException e) {
             // Ignore exception (this table will be skipped).
           }
@@ -288,13 +284,13 @@ public class MetadataOp {
           // If the table is not yet loaded, the columns will be unknown. Add it
           // to the set of missing tables.
           if (!table.isLoaded()) {
-            result.missingTbls.add(new TableName(dbName, tabName));
+            result.missingTbls.add(new TableName(db.getName(), tabName));
           } else {
             columns.addAll(fe.getColumns(table, columnPattern, user));
           }
           tablesColumnsList.add(columns);
         }
-        result.dbs.add(dbName);
+        result.dbs.add(db.getName());
         result.tableNames.add(tableList);
         result.columns.add(tablesColumnsList);
       }

fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java

@@ -31,7 +31,6 @@ import org.junit.Assert;
 import org.junit.Test;
 
 import com.cloudera.impala.catalog.ArrayType;
-import com.cloudera.impala.catalog.Catalog;
 import com.cloudera.impala.catalog.CatalogException;
 import com.cloudera.impala.analysis.CreateTableStmt;
 import com.cloudera.impala.catalog.DataSource;
@@ -2336,7 +2335,7 @@ public class AnalyzeDDLTest extends AnalyzerTest {
 
   @Test
   public void TestDescribeDb() throws AnalysisException {
-    addTestDb("test_analyse_desc_db");
+    addTestDb("test_analyse_desc_db", null);
     AnalyzesOk("describe database test_analyse_desc_db");
     AnalyzesOk("describe database extended test_analyse_desc_db");
     AnalyzesOk("describe database formatted test_analyse_desc_db");
@@ -2374,7 +2373,7 @@ public class AnalyzeDDLTest extends AnalyzerTest {
         "Could not resolve path: 'functional_parquet.allcomplextypes.nonexistent'");
 
     // Handling of ambiguous paths.
-    addTestDb("ambig");
+    addTestDb("ambig", null);
     addTestTable("create table ambig.ambig (ambig struct<ambig:array<int>>)");
     // Single element path can only be resolved as <table>.
     DescribeTableStmt describe = (DescribeTableStmt)AnalyzesOk("describe ambig",

fe/src/test/java/com/cloudera/impala/analysis/AnalyzeStmtsTest.java

@@ -403,7 +403,7 @@ public class AnalyzeStmtsTest extends AnalyzerTest {
           String.format("Could not resolve column/field reference: '%s'", field));
     }
 
-    addTestDb("d");
+    addTestDb("d", null);
 
     // Test array of scalars. Only explicit paths make sense.
     addTestTable("create table d.t1 (c array<int>)");
@@ -634,7 +634,7 @@ public class AnalyzeStmtsTest extends AnalyzerTest {
 
   @Test
   public void TestSlotRefPathAmbiguity() {
-    addTestDb("a");
+    addTestDb("a", null);
     addTestTable("create table a.a (a struct<a:struct<a:int>>)");
 
     // Slot path is not ambiguous.
@@ -678,7 +678,7 @@ public class AnalyzeStmtsTest extends AnalyzerTest {
 
   @Test
   public void TestStarPathAmbiguity() {
-    addTestDb("a");
+    addTestDb("a", null);
     addTestTable("create table a.a (a struct<a:struct<a:int>>)");
 
     // Star path is not ambiguous.
@@ -721,7 +721,7 @@ public class AnalyzeStmtsTest extends AnalyzerTest {
 
   @Test
   public void TestTableRefPathAmbiguity() {
-    addTestDb("a");
+    addTestDb("a", null);
     addTestTable("create table a.a (a array<struct<a:array<int>>>)");
 
     // Table paths are not ambiguous.

fe/src/test/java/com/cloudera/impala/analysis/AnalyzerTest.java

@@ -19,13 +19,13 @@ import static org.junit.Assert.fail;
 
 import java.io.StringReader;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
-import org.junit.Assert;
-
 import org.junit.After;
+import org.junit.Assert;
 import org.junit.Test;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -138,10 +138,11 @@ public class AnalyzerTest {
    * Returns the new dummy database.
    * The database is registered in testDbs_ and removed in the @After method.
    */
-  protected Db addTestDb(String dbName) {
+  protected Db addTestDb(String dbName, String comment) {
     Db db = catalog_.getDb(dbName);
     Preconditions.checkState(db == null, "Test db must not already exist.");
-    db = new Db(dbName, catalog_, null);
+    db = new Db(dbName, catalog_, new org.apache.hadoop.hive.metastore.api.Database(
+        dbName, comment, "", Collections.<String, String>emptyMap()));
     catalog_.addDb(db);
     testDbs_.add(db);
     return db;

fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java

@@ -42,6 +42,7 @@ import com.cloudera.impala.authorization.AuthorizeableTable;
 import com.cloudera.impala.authorization.User;
 import com.cloudera.impala.catalog.AuthorizationException;
 import com.cloudera.impala.catalog.Catalog;
+import com.cloudera.impala.catalog.Db;
 import com.cloudera.impala.catalog.ImpaladCatalog;
 import com.cloudera.impala.catalog.ScalarFunction;
 import com.cloudera.impala.catalog.Type;
@@ -1441,11 +1442,17 @@ public class AuthorizationTest {
     List<String> expectedDbs = Lists.newArrayList("default", "functional",
         "functional_parquet", "functional_seq_snap", "tpcds", "tpch");
 
-    List<String> dbs = fe_.getDbNames("*", USER);
-    Assert.assertEquals(expectedDbs, dbs);
+    List<Db> dbs = fe_.getDbs("*", USER);
+    assertEquals(expectedDbs, extractDbNames(dbs));
 
-    dbs = fe_.getDbNames(null, USER);
-    Assert.assertEquals(expectedDbs, dbs);
+    dbs = fe_.getDbs(null, USER);
+    assertEquals(expectedDbs, extractDbNames(dbs));
+  }
+
+  private List<String> extractDbNames(List<Db> dbs) {
+    List<String> names = Lists.newArrayListWithCapacity(dbs.size());
+    for (Db db: dbs) names.add(db.getName());
+    return names;
   }
 
   @Test

fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java

@@ -44,8 +44,7 @@ public class BlockIdGenerator {
 
       // Load all tables in the catalog
       Catalog catalog = CatalogServiceTestCatalog.create();
-      for (String dbName: catalog.getDbNames(null)) {
-        Db database = catalog.getDb(dbName);
+      for (Db database: catalog.getDbs(null)) {
         for (String tableName: database.getAllTableNames()) {
           Table table = database.getTable(tableName);
           // Only do this for hdfs tables

fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java

@@ -43,9 +43,9 @@ public class ImpaladTestCatalog extends ImpaladCatalog {
     CatalogServiceCatalog catalogServerCatalog =
         CatalogServiceTestCatalog.createWithAuth(authzConfig.getSentryConfig());
     // Bootstrap the catalog by adding all dbs, tables, and functions.
-    for (String dbName: catalogServerCatalog.getDbNames(null)) {
+    for (Db db: catalogServerCatalog.getDbs(null)) {
       // Adding DB should include all tables/fns in that database.
-      addDb(catalogServerCatalog.getDb(dbName));
+      addDb(db);
     }
     authPolicy_ = ((CatalogServiceTestCatalog) catalogServerCatalog).getAuthPolicy();
     srcCatalog_ = catalogServerCatalog;

testdata/workloads/functional-query/queries/QueryTest/alter-table.test

@@ -758,9 +758,9 @@ create database if not exists test_alter_property_length_db
 ---- QUERY
 show databases like 'test_alter_property_length_db'
 ---- RESULTS
-'test_alter_property_length_db'
+'test_alter_property_length_db',''
 ---- TYPES
-STRING
+STRING,STRING
 ====
 ---- QUERY
 drop table if exists test_alter_property_length_db.property_length
@@ -801,5 +801,5 @@ drop database if exists test_alter_property_length_db
 show databases like 'test_alter_property_length_db'
 ---- RESULTS
 ---- TYPES
-STRING
+STRING,STRING
 ====

testdata/workloads/functional-query/queries/QueryTest/create.test

@@ -152,9 +152,9 @@ drop table ddl_test_db.test_like_file_create
 # It should show up now
 show databases like 'ddl_test_db'
 ---- RESULTS
-'ddl_test_db'
+'ddl_test_db',''
 ---- TYPES
-STRING
+STRING,STRING
 ====
 ---- QUERY
 # Make sure creating a database with the same name doesn't throw an error when
@@ -817,7 +817,7 @@ drop database ddl_test_db
 show databases like 'ddl_test_db'
 ---- RESULTS
 ---- TYPES
-STRING
+STRING,STRING
 ====
 ---- QUERY
 # Test DROP DATABASE ... [CASCADE | RESTRICT]
@@ -826,9 +826,9 @@ create database if not exists test_drop_cascade_db
 ---- QUERY
 show databases like 'test_drop_cascade_db'
 ---- RESULTS
-'test_drop_cascade_db'
+'test_drop_cascade_db',''
 ---- TYPES
-STRING
+STRING,STRING
 ====
 ---- QUERY
 create table if not exists test_drop_cascade_db.t1 (i int);
@@ -884,9 +884,9 @@ create database if not exists test_drop_restrict_db
 ---- QUERY
 show databases like 'test_drop_restrict_db'
 ---- RESULTS
-'test_drop_restrict_db'
+'test_drop_restrict_db',''
 ---- TYPES
-STRING
+STRING,STRING
 ====
 ---- QUERY
 drop database test_drop_restrict_db restrict
@@ -903,9 +903,9 @@ create database if not exists test_property_length_db
 ---- QUERY
 show databases like 'test_property_length_db'
 ---- RESULTS
-'test_property_length_db'
+'test_property_length_db',''
 ---- TYPES
-STRING
+STRING,STRING
 ====
 ---- QUERY
 drop table if exists test_property_length_db.short_properties

testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test

@@ -360,10 +360,10 @@ root
 # This privilege actually active
 show databases
 ---- RESULTS
-'default'
-'functional'
+'default','Default Hive database'
+'functional',''
 ---- TYPES
-STRING
+STRING,STRING
 ====
 ---- USER
 root
@@ -431,10 +431,10 @@ root
 # The privilege is still active
 show databases
 ---- RESULTS
-'default'
-'functional'
+'default','Default Hive database'
+'functional',''
 ---- TYPES
-STRING
+STRING,STRING
 ====
 ---- QUERY
 # Privilege still exists, but grant option is set to false

testdata/workloads/functional-query/queries/QueryTest/grant_revoke_no_insert.test

@@ -313,10 +313,10 @@ root
 # This privilege actually active
 show databases
 ---- RESULTS
-'default'
-'functional'
+'default','Default Hive database'
+'functional',''
 ---- TYPES
-STRING
+STRING,STRING
 ====
 ---- USER
 root
@@ -384,10 +384,10 @@ root
 # The privilege is still active
 show databases
 ---- RESULTS
-'default'
-'functional'
+'default','Default Hive database'
+'functional',''
 ---- TYPES
-STRING
+STRING,STRING
 ====
 ---- QUERY
 # Privilege still exists, but grant option is set to false

testdata/workloads/functional-query/queries/QueryTest/show.test

@@ -189,23 +189,30 @@ STRING
 # Show databases
 show databases like 'tpcds'
 ---- RESULTS
-'tpcds'
+'tpcds',''
 ---- TYPES
-STRING
+STRING,STRING
 ====
 ---- QUERY
 show databases like 'functional'
 ---- RESULTS
-'functional'
+'functional',''
 ---- TYPES
-STRING
+STRING,STRING
 ====
 ---- QUERY
 show databases like 'functional'
 ---- RESULTS
-'functional'
+'functional',''
 ---- TYPES
-STRING
+STRING,STRING
+====
+---- QUERY
+show databases like 'def*'
+---- RESULTS
+'default','Default Hive database'
+---- TYPES
+STRING,STRING
 ====
 ---- QUERY
 show files in insert_string_partitioned

tests/common/impala_test_suite.py

@@ -135,6 +135,12 @@ class ImpalaTestSuite(BaseTestSuite):
       hdfs_client =  get_hdfs_client(host, port)
     return hdfs_client
 
+  @classmethod
+  def all_db_names(self):
+    results = self.client.execute("show databases").data
+    # Extract first column - database name
+    return [row.split("\t")[0] for row in results]
+
   @classmethod
   def cleanup_db(self, db_name, sync_ddl=1):
     self.client.execute("use default")

tests/metadata/test_ddl.py

@@ -32,7 +32,7 @@ from tests.util.filesystem_utils import WAREHOUSE, IS_DEFAULT_FS
 class TestDdlStatements(ImpalaTestSuite):
   TEST_DBS = ['ddl_test_db', 'ddl_purge_db', 'alter_table_test_db', 'alter_table_test_db2',
               'function_ddl_test', 'udf_test', 'data_src_test', 'truncate_table_test_db',
-              'test_db', 'alter_purge_db']
+              'test_db', 'alter_purge_db', 'db_with_comment']
 
   @classmethod
   def get_workload(self):
@@ -231,7 +231,7 @@ class TestDdlStatements(ImpalaTestSuite):
     # not fail
     self.client.execute("create database if not exists test_db")
     # The database should appear in the catalog (IMPALA-2441)
-    assert 'test_db' in self.client.execute("show databases").data
+    assert 'test_db' in self.all_db_names()
     # Ensure a table can be created in this database from Impala and that it is
     # accessable in both Impala and Hive
     self.client.execute("create table if not exists test_db.test_tbl_in_impala(a int)")
@@ -260,7 +260,7 @@ class TestDdlStatements(ImpalaTestSuite):
     # Drop the database immediately after creation (within a statestore heartbeat) and
     # verify the catalog gets updated properly.
     self.client.execute('drop database ddl_test_db')
-    assert 'ddl_test_db' not in self.client.execute("show databases").data
+    assert 'ddl_test_db' not in self.all_db_names()
 
   # TODO: don't use hdfs_client
   @SkipIfS3.insert # S3: missing coverage: alter table
@@ -509,17 +509,33 @@ class TestDdlStatements(ImpalaTestSuite):
     assert properties['p2'] == 'val3'
     assert properties[''] == ''
 
+  @pytest.mark.execute_serially
+  def test_create_db_comment(self, vector):
+    DB_NAME = 'db_with_comment'
+    COMMENT = 'A test comment'
+    self._create_db(DB_NAME, sync=True, comment=COMMENT)
+    result = self.client.execute("show databases like '{0}'".format(DB_NAME))
+    assert len(result.data) == 1
+    cols = result.data[0].split('\t')
+    assert len(cols) == 2
+    assert cols[0] == DB_NAME
+    assert cols[1] == COMMENT
+
   @classmethod
   def _use_multiple_impalad(cls, vector):
     return vector.get_value('exec_option')['sync_ddl'] == 1
 
-  def _create_db(self, db_name, sync=False):
+  def _create_db(self, db_name, sync=False, comment=None):
     """Creates a database using synchronized DDL to ensure all nodes have the test
     database available for use before executing the .test file(s).
     """
     impala_client = self.create_impala_client()
     sync and impala_client.set_configuration({'sync_ddl': 1})
-    ddl = "create database {0} location '{1}/{0}.db'".format(db_name, WAREHOUSE)
+    if comment is None:
+      ddl = "create database {0} location '{1}/{0}.db'".format(db_name, WAREHOUSE)
+    else:
+      ddl = "create database {0} comment '{1}' location '{2}/{0}.db'".format(
+        db_name, comment, WAREHOUSE)
     impala_client.execute(ddl)
     impala_client.close()
 

tests/metadata/test_metadata_query_statements.py

@@ -158,8 +158,7 @@ class TestMetadataQueryStatements(ImpalaTestSuite):
     call(["hive", "-e", "DROP DATABASE IF EXISTS %s CASCADE" % db_name])
     self.client.execute("invalidate metadata")
 
-    result = self.client.execute("show databases")
-    assert db_name not in result.data
+    assert db_name not in self.all_db_names()
 
     call(["hive", "-e", "CREATE DATABASE %s" % db_name])
 
@@ -171,21 +170,18 @@ class TestMetadataQueryStatements(ImpalaTestSuite):
       assert "TableNotFoundException: Table not found: %s.%s"\
           % (db_name, tbl_name) in str(e)
 
-    result = self.client.execute("show databases")
-    assert db_name not in result.data
+    assert db_name not in self.all_db_names()
 
     # Create a table external to Impala.
     call(["hive", "-e", "CREATE TABLE %s.%s (i int)" % (db_name, tbl_name)])
 
     # Impala does not know about this database or table.
-    result = self.client.execute("show databases")
-    assert db_name not in result.data
+    assert db_name not in self.all_db_names()
 
     # Run 'invalidate metadata <table name>'. It should add the database and table
     # in to Impala's catalog.
     self.client.execute("invalidate metadata %s.%s"  % (db_name, tbl_name))
-    result = self.client.execute("show databases")
-    assert db_name in result.data
+    assert db_name in self.all_db_names()
 
     result = self.client.execute("show tables in %s" % db_name)
     assert tbl_name in result.data
@@ -272,9 +268,7 @@ class TestMetadataQueryStatements(ImpalaTestSuite):
     assert len(result.data) == 0
 
     # Requires a refresh to see the dropped database
-    result = self.client.execute("show databases");
-    assert db_name in result.data
+    assert db_name in self.all_db_names()
 
     self.client.execute("invalidate metadata")
-    result = self.client.execute("show databases");
-    assert db_name not in result.data
+    assert db_name not in self.all_db_names()

tests/util/compute_table_stats.py

@@ -27,7 +27,7 @@ def compute_stats(impala_client, db_names=None, table_names=None,
   """
   print "Enumerating databases and tables for compute stats."
 
-  all_dbs = set(name.lower() for name in impala_client.execute("show databases").data)
+  all_dbs = set(name.split('\t')[0].lower() for name in impala_client.execute("show databases").data)
   selected_dbs = all_dbs if db_names is None else set(db_names)
   for db in all_dbs.intersection(selected_dbs):
     all_tables =\