#921: Fixed: [YCQL] improve error messages related to index DDL

Summary:
Updated a few error codes and a set of error messages to rename Table/Index into Object.
Added appropriate tests.

Related doc:   <doc-link>

Test Plan:
ybd  --cxx-test ql-create-index-test
ybd  --cxx-test ql-drop-stmt-test --gtest_filter TestQLDropStmt.TestQLDropIndex

Reviewers: robert, neha, neil

Reviewed By: neil

Subscribers: neha, bogdan, bharat, kannan, eng

Differential Revision: https://phabricator.dev.yugabyte.com/D6264

java/yb-cql/src/test/java/org/yb/cql/TestBasicStatements.java

@@ -105,7 +105,7 @@ public void testCQLTimesOut() throws Exception {
       AssertionWrappers.fail("Not expecting a client side timeout.");
     } catch (RuntimeException re) {
       LOG.info("Caught execption ", re);
-      AssertionWrappers.assertTrue(re.getMessage().contains("Timed out"));
+      AssertionWrappers.assertTrue(re.getMessage().contains("passed its deadline"));
     }
 
     // destroy the cluster, without trying to clean up the tables etc.

java/yb-cql/src/test/java/org/yb/cql/TestIndex.java

@@ -669,7 +669,7 @@ private void assertInvalidUniqueIndexDML(String query, String indexName) {
       fail("InvalidQueryException not thrown for " + query);
     } catch (InvalidQueryException e) {
       assertTrue(e.getMessage().startsWith(
-          String.format("Query error: Execution Error. Duplicate value disallowed by unique " +
+          String.format("Execution Error. Duplicate value disallowed by unique " +
                         "index %s", indexName)));
     }
   }

java/yb-cql/src/test/java/org/yb/cql/TestTransaction.java

@@ -469,7 +469,7 @@ public void testSystemTransactionsTable() throws Exception {
             Integer.valueOf(3), Integer.valueOf(3), "v3");
 
     thrown.expect(com.datastax.driver.core.exceptions.InvalidQueryException.class);
-    thrown.expectMessage("Table Not Found");
+    thrown.expectMessage("Object Not Found");
     Iterator<Row> rows = session.execute("SELECT * FROM system.transactions").iterator();
   }
 

src/yb/client/client-internal.cc

@@ -443,7 +443,7 @@ Status YBClient::Data::CreateTable(YBClient* client,
   *table_id = resp.table_id();
   RETURN_NOT_OK(s);
   if (resp.has_error()) {
-    if (resp.error().code() == MasterErrorPB::TABLE_ALREADY_PRESENT && attempts > 1) {
+    if (resp.error().code() == MasterErrorPB::OBJECT_ALREADY_PRESENT && attempts > 1) {
       // If the table already exists and the number of attempts is >
       // 1, then it means we may have succeeded in creating the
       // table, but client didn't receive the successful
@@ -570,7 +570,7 @@ Status YBClient::Data::DeleteTable(YBClient* client,
   RETURN_NOT_OK(s);
 
   if (resp.has_error()) {
-    if (resp.error().code() == MasterErrorPB::TABLE_NOT_FOUND && attempts > 1) {
+    if (resp.error().code() == MasterErrorPB::OBJECT_NOT_FOUND && attempts > 1) {
       // A prior attempt to delete the table has succeeded, but
       // appeared as a failure to the client due to, e.g., an I/O or
       // network issue.
@@ -617,7 +617,7 @@ Status YBClient::Data::IsDeleteTableInProgress(YBClient* client,
   // compiler complains.
   RETURN_NOT_OK(s);
   if (resp.has_error()) {
-    if (resp.error().code() == MasterErrorPB::TABLE_NOT_FOUND) {
+    if (resp.error().code() == MasterErrorPB::OBJECT_NOT_FOUND) {
       *delete_in_progress = false;
       return Status::OK();
     }

src/yb/client/client-test.cc

@@ -494,7 +494,7 @@ TEST_F(ClientTest, TestBadTable) {
   shared_ptr<YBTable> t;
   Status s = client_->OpenTable(YBTableName(kKeyspaceName, "xxx-does-not-exist"), &t);
   ASSERT_TRUE(s.IsNotFound());
-  ASSERT_STR_CONTAINS(s.ToString(false), "Not found: The table does not exist");
+  ASSERT_STR_CONTAINS(s.ToString(false), "Not found: The object does not exist");
 }
 
 // Test that, if the master is down, we experience a network error talking
@@ -1290,7 +1290,7 @@ TEST_F(ClientTest, TestDeleteTable) {
   // Try to open the deleted table
   Status s = client_table_.Open(kTableName, client_.get());
   ASSERT_TRUE(s.IsNotFound());
-  ASSERT_STR_CONTAINS(s.ToString(), "The table does not exist");
+  ASSERT_STR_CONTAINS(s.ToString(), "The object does not exist");
 
   // Create a new table with the same name. This is to ensure that the client
   // doesn't cache anything inappropriately by table name (see KUDU-1055).
@@ -1312,7 +1312,7 @@ TEST_F(ClientTest, TestGetTableSchema) {
   Status s = client_->GetTableSchema(YBTableName(kKeyspaceName, "MissingTableName"), &schema,
                                      &partition_schema);
   ASSERT_TRUE(s.IsNotFound());
-  ASSERT_STR_CONTAINS(s.ToString(), "The table does not exist");
+  ASSERT_STR_CONTAINS(s.ToString(), "The object does not exist");
 }
 
 TEST_F(ClientTest, TestStaleLocations) {

src/yb/master/catalog_manager.h

@@ -1032,8 +1032,12 @@ class CatalogManager : public tserver::TabletPeerLookupIf {
   // Return all the available (user-defined) types.
   void GetAllUDTypes(std::vector<scoped_refptr<UDTypeInfo> >* types);
 
+  NamespaceName GetNamespaceNameUnlocked(const NamespaceId& id) const;
   NamespaceName GetNamespaceName(const NamespaceId& id) const;
 
+  NamespaceName GetNamespaceNameUnlocked(const scoped_refptr<TableInfo>& table) const;
+  NamespaceName GetNamespaceName(const scoped_refptr<TableInfo>& table) const;
+
   void GetAllRoles(std::vector<scoped_refptr<RoleInfo>>* roles);
 
   // Find all the roles for which 'role' is a member of the list 'member_of'.

src/yb/master/master-test.cc

@@ -798,7 +798,7 @@ TEST_F(MasterTest, TestNamespaces) {
     const Status s = CreateNamespace(other_ns_name, &resp);
     ASSERT_TRUE(s.IsAlreadyPresent()) << s.ToString();
     ASSERT_STR_CONTAINS(s.ToString(),
-        Substitute("Keyspace $0 already exists", other_ns_name));
+        Substitute("Keyspace '$0' already exists", other_ns_name));
   }
   {
     ASSERT_NO_FATALS(DoListAllNamespaces(&namespaces));
@@ -872,7 +872,7 @@ TEST_F(MasterTest, TestNamespaces) {
     const Status s = CreateNamespace(default_namespace_name, &resp);
     ASSERT_TRUE(s.IsAlreadyPresent()) << s.ToString();
     ASSERT_STR_CONTAINS(s.ToString(),
-        Substitute("Keyspace $0 already exists", default_namespace_name));
+        Substitute("Keyspace '$0' already exists", default_namespace_name));
   }
   {
     ASSERT_NO_FATALS(DoListAllNamespaces(&namespaces));
@@ -1345,10 +1345,10 @@ TEST_F(MasterTest, TestFullTableName) {
     ASSERT_OK(proxy_->AlterTable(req, &resp, ResetAndGetController()));
     SCOPED_TRACE(resp.DebugString());
     ASSERT_TRUE(resp.has_error());
-    ASSERT_EQ(resp.error().code(), MasterErrorPB::TABLE_ALREADY_PRESENT);
+    ASSERT_EQ(resp.error().code(), MasterErrorPB::OBJECT_ALREADY_PRESENT);
     ASSERT_EQ(resp.error().status().code(), AppStatusPB::ALREADY_PRESENT);
     ASSERT_STR_CONTAINS(resp.error().status().ShortDebugString(),
-        "Table already exists");
+        " already exists");
   }
   // Check that nothing's changed (still have 3 tables).
   ASSERT_NO_FATALS(DoListAllTables(&tables));
@@ -1380,10 +1380,10 @@ TEST_F(MasterTest, TestFullTableName) {
     ASSERT_OK(proxy_->DeleteTable(req, &resp, ResetAndGetController()));
     SCOPED_TRACE(resp.DebugString());
     ASSERT_TRUE(resp.has_error());
-    ASSERT_EQ(resp.error().code(), MasterErrorPB::TABLE_NOT_FOUND);
+    ASSERT_EQ(resp.error().code(), MasterErrorPB::OBJECT_NOT_FOUND);
     ASSERT_EQ(resp.error().status().code(), AppStatusPB::NOT_FOUND);
     ASSERT_STR_CONTAINS(resp.error().status().ShortDebugString(),
-        "The table does not exist");
+        "The object does not exist");
   }
 
   // Delete the table.

src/yb/master/master.proto

@@ -57,11 +57,11 @@ message MasterErrorPB {
     // The schema provided for a request was not well-formed.
     INVALID_SCHEMA = 2;
 
-    // The requested table does not exist
-    TABLE_NOT_FOUND = 3;
+    // The requested table or index does not exist
+    OBJECT_NOT_FOUND = 3;
 
-    // The name requested for the table is already in use
-    TABLE_ALREADY_PRESENT = 4;
+    // The name requested for the table or index is already in use
+    OBJECT_ALREADY_PRESENT = 4;
 
     // The number of tablets requested for a new table is over the per TS limit.
     TOO_MANY_TABLETS = 5;

src/yb/util/status.cc

@@ -147,16 +147,22 @@ std::string Status::CodeAsString() const {
   return cstr != nullptr ? cstr : "Incorrect status code " + std::to_string(code);
 }
 
-std::string Status::ToUserMessage(bool include_file_and_line) const {
+std::string Status::ToUserMessage(bool include_code) const {
   if (error_code() == 0) {
     // Return empty string for success.
     return "";
   }
-  return ToString(include_file_and_line);
+  // Never include internal file name for the user.
+  return ToString(/* include_file_and_line */ false, include_code);
 }
 
-std::string Status::ToString(bool include_file_and_line) const {
-  std::string result(CodeAsString());
+std::string Status::ToString(bool include_file_and_line, bool include_code) const {
+  std::string result;
+
+  if (include_code) {
+    result += CodeAsString();
+  }
+
   if (state_ == nullptr) {
     return result;
   }
@@ -176,9 +182,19 @@ std::string Status::ToString(bool include_file_and_line) const {
     result.append(std::to_string(state_->line_number));
     result.append(")");
   }
-  result.append(": ");
+
+  if (!result.empty()) {
+    result.append(": ");
+  }
+
   Slice msg = message();
   result.append(reinterpret_cast<const char*>(msg.data()), msg.size());
+
+  // If no message (rare case) - show code (if it's not shown yet).
+  if (result.empty()) {
+    result += CodeAsString();
+  }
+
   int64_t error = error_code();
   if (error != -1) {
     char buf[64];

src/yb/util/status.h

@@ -193,11 +193,11 @@ class Status {
 
   // Returns a text message of this status to be reported to users.
   // Returns empty string for success.
-  std::string ToUserMessage(bool include_file_and_line = false) const;
+  std::string ToUserMessage(bool include_code = false) const;
 
   // Return a string representation of this status suitable for printing.
   // Returns the string "OK" for success.
-  std::string ToString(bool include_file_and_line = true) const;
+  std::string ToString(bool include_file_and_line = true, bool include_code = true) const;
 
   // Return a string representation of the status code, without the message
   // text or posix code information.

src/yb/util/ybc-internal.cc

@@ -45,7 +45,7 @@ YBCStatus ToYBCStatus(const Status& status) {
   if (status.ok()) {
     return nullptr;
   }
-  std::string status_str = status.ToUserMessage();
+  std::string status_str = status.ToUserMessage(/* include_code */ true);
   size_t status_msg_buf_size = status_str.size() + 1;
   YBCStatus ybc_status = reinterpret_cast<YBCStatus>(
       malloc(sizeof(YBCStatusStruct) + status_msg_buf_size));

src/yb/yql/cql/ql/exec/executor.cc

@@ -422,16 +422,16 @@ Status Executor::ExecPTNode(const PTCreateTable *tnode) {
   if (PREDICT_FALSE(!s.ok())) {
     ErrorCode error_code = ErrorCode::SERVER_ERROR;
     if (s.IsAlreadyPresent()) {
-      error_code = ErrorCode::DUPLICATE_TABLE;
+      error_code = ErrorCode::DUPLICATE_OBJECT;
     } else if (s.IsNotFound()) {
       error_code = tnode->opcode() == TreeNodeOpcode::kPTCreateIndex
-                   ? ErrorCode::TABLE_NOT_FOUND
+                   ? ErrorCode::OBJECT_NOT_FOUND
                    : ErrorCode::KEYSPACE_NOT_FOUND;
     } else if (s.IsInvalidArgument()) {
       error_code = ErrorCode::INVALID_TABLE_DEFINITION;
     }
 
-    if (tnode->create_if_not_exists() && error_code == ErrorCode::DUPLICATE_TABLE) {
+    if (tnode->create_if_not_exists() && error_code == ErrorCode::DUPLICATE_OBJECT) {
       return Status::OK();
     }
 
@@ -509,7 +509,7 @@ Status Executor::ExecPTNode(const PTDropStmt *tnode) {
       // Drop the table.
       const YBTableName table_name = tnode->yb_table_name();
       s = ql_env_->DeleteTable(table_name);
-      error_not_found = ErrorCode::TABLE_NOT_FOUND;
+      error_not_found = ErrorCode::OBJECT_NOT_FOUND;
       result_ = std::make_shared<SchemaChangeResult>(
           "DROPPED", "TABLE", table_name.namespace_name(), table_name.table_name());
       ql_env_->RemoveCachedTableDesc(table_name);
@@ -521,7 +521,7 @@ Status Executor::ExecPTNode(const PTDropStmt *tnode) {
       const YBTableName table_name = tnode->yb_table_name();
       YBTableName indexed_table_name;
       s = ql_env_->DeleteIndexTable(table_name, &indexed_table_name);
-      error_not_found = ErrorCode::TABLE_NOT_FOUND;
+      error_not_found = ErrorCode::OBJECT_NOT_FOUND;
       result_ = std::make_shared<SchemaChangeResult>(
           "UPDATED", "TABLE", indexed_table_name.namespace_name(), indexed_table_name.table_name());
       ql_env_->RemoveCachedTableDesc(indexed_table_name);
@@ -646,7 +646,7 @@ Status Executor::ExecPTNode(const PTSelectStmt *tnode, TnodeContext* tnode_conte
     // If this is a system table but the table does not exist, it is okay. Just return OK with void
     // result.
     return tnode->is_system() ? Status::OK()
-                              : exec_context_->Error(tnode, ErrorCode::TABLE_NOT_FOUND);
+                              : exec_context_->Error(tnode, ErrorCode::OBJECT_NOT_FOUND);
   }
 
   const StatementParameters& params = exec_context_->params();
@@ -655,7 +655,7 @@ Status Executor::ExecPTNode(const PTSelectStmt *tnode, TnodeContext* tnode_conte
   // for query without index, or the index id in the leaf node (where child_select is null also).
   const bool continue_select = !tnode->child_select() && !params.table_id().empty();
   if (continue_select && params.table_id() != table->id()) {
-    return exec_context_->Error(tnode, "Table no longer exists.", ErrorCode::TABLE_NOT_FOUND);
+    return exec_context_->Error(tnode, "Object no longer exists.", ErrorCode::OBJECT_NOT_FOUND);
   }
 
   // If there is an index to select from, execute it.
@@ -1898,7 +1898,7 @@ Status Executor::ProcessStatementStatus(const ParseTree& parse_tree, const Statu
         errcode == ErrorCode::WRONG_METADATA_VERSION   ||
         errcode == ErrorCode::INVALID_TABLE_DEFINITION ||
         errcode == ErrorCode::INVALID_ARGUMENTS        ||
-        errcode == ErrorCode::TABLE_NOT_FOUND          ||
+        errcode == ErrorCode::OBJECT_NOT_FOUND         ||
         errcode == ErrorCode::TYPE_NOT_FOUND) {
       parse_tree.ClearAnalyzedTableCache(ql_env_);
       parse_tree.ClearAnalyzedUDTypeCache(ql_env_);

src/yb/yql/cql/ql/ptree/pt_dml.cc

@@ -117,7 +117,7 @@ Status PTDmlStmt::LookupTable(SemContext *sem_context) {
   if (!table_ || (table_->IsIndex() && !FLAGS_allow_index_table_read_write) ||
       // Only looking for CQL tables.
       (table_->table_type() != client::YBTableType::YQL_TABLE_TYPE)) {
-    return sem_context->Error(table_loc(), ErrorCode::TABLE_NOT_FOUND);
+    return sem_context->Error(table_loc(), ErrorCode::OBJECT_NOT_FOUND);
   }
   LoadSchema(sem_context, table_, &column_map_);
   return Status::OK();
@@ -340,7 +340,7 @@ Status PTDmlStmt::AnalyzeIndexesForWrites(SemContext *sem_context) {
       std::shared_ptr<client::YBTable> index_table = sem_context->GetTableDesc(index_id);
       if (index_table == nullptr) {
         return sem_context->Error(this, Substitute("Index table $0 not found", index_id).c_str(),
-                                  ErrorCode::TABLE_NOT_FOUND);
+                                  ErrorCode::OBJECT_NOT_FOUND);
       }
       pk_only_indexes_.insert(index_table);
     } else {

src/yb/yql/cql/ql/ptree/pt_select.cc

@@ -301,7 +301,7 @@ Status PTSelectStmt::LookupIndex(SemContext *sem_context) {
   if (!table_ || !table_->IsIndex() ||
       // Only looking for CQL Indexes.
       (table_->table_type() != client::YBTableType::YQL_TABLE_TYPE)) {
-    return sem_context->Error(table_loc(), ErrorCode::TABLE_NOT_FOUND);
+    return sem_context->Error(table_loc(), ErrorCode::OBJECT_NOT_FOUND);
   }
   LoadSchema(sem_context, table_, &column_map_);
   return Status::OK();

src/yb/yql/cql/ql/ptree/sem_context.cc

@@ -117,7 +117,7 @@ Status SemContext::LookupTable(const YBTableName& name,
   if (*table == nullptr || ((*table)->IsIndex() && !FLAGS_allow_index_table_read_write) ||
       // Only looking for CQL tables.
       (*table)->table_type() != client::YBTableType::YQL_TABLE_TYPE) {
-    return Error(loc, ErrorCode::TABLE_NOT_FOUND);
+    return Error(loc, ErrorCode::OBJECT_NOT_FOUND);
   }
 
   return LoadSchema(*table, col_descs, column_definitions);
@@ -133,7 +133,7 @@ Status SemContext::LookupIndex(const TableId& index_id,
   if (*index_table == nullptr || !(*index_table)->IsIndex() ||
       // Only looking for CQL Indexes.
       (*index_table)->table_type() != client::YBTableType::YQL_TABLE_TYPE) {
-    return Error(loc, ErrorCode::TABLE_NOT_FOUND);
+    return Error(loc, ErrorCode::OBJECT_NOT_FOUND);
   }
 
   return LoadSchema(*index_table, col_descs, column_definitions);
@@ -157,7 +157,7 @@ Status SemContext::MapSymbol(const MCString& name, PTAlterColumnDefinition *entr
 
 Status SemContext::MapSymbol(const MCString& name, PTCreateTable *entry) {
   if (symtab_[name].create_table_ != nullptr) {
-    return Error(entry, ErrorCode::DUPLICATE_TABLE);
+    return Error(entry, ErrorCode::DUPLICATE_OBJECT);
   }
   symtab_[name].create_table_ = entry;
   return Status::OK();

src/yb/yql/cql/ql/sem/analyzer.cc

@@ -44,7 +44,7 @@ CHECKED_STATUS Analyzer::Analyze(ParseTree::UniPtr parse_tree) {
     if (!ptree->reparsed()) {
       const ErrorCode errcode = GetErrorCode(s);
       if (errcode != ErrorCode::KEYSPACE_NOT_FOUND &&
-          errcode != ErrorCode::TABLE_NOT_FOUND &&
+          errcode != ErrorCode::OBJECT_NOT_FOUND &&
           errcode != ErrorCode::TYPE_NOT_FOUND &&
           sem_context_->cache_used()) {
         ptree->ClearAnalyzedTableCache(ql_env_);

src/yb/yql/cql/ql/test/CMakeLists.txt

@@ -22,6 +22,7 @@ add_dependencies(ql_test ql_api)
 set(YB_TEST_LINK_LIBS ql_test ${YB_MIN_TEST_LIBS})
 ADD_YB_TEST(ql-parser-test)
 ADD_YB_TEST(ql-create-table-test)
+ADD_YB_TEST(ql-create-index-test)
 ADD_YB_TEST(ql-drop-stmt-test)
 ADD_YB_TEST(ql-query-test)
 ADD_YB_TEST(ql-insert-table-test)