Skip to content

Commit

Permalink
#1944: [YCQL] Fixed TS crash if a built-in function argument is a col…
Browse files Browse the repository at this point in the history 
…lection.

Summary:
Fixed PTBCall::Analyze() - to prevent usage of parametric types with failed BCalls.
Plus minor clean-up in macros.

Test Plan:
ybd --cxx-test ql-insert-table-test
ybd --cxx-test ql-query-test
ybd --cxx-test ql_ql-select-expr-test

Reviewers: mihnea, dmitry, neil

Reviewed By: neil

Subscribers: alex, eng

Differential Revision: https://phabricator.dev.yugabyte.com/D6986
  • Loading branch information
@OlegLoginov
OlegLoginov committed Aug 20, 2019
1 parent 5b6c07c commit 2d20f37
Show file tree
Hide file tree
Showing 9 changed files with 183 additions and 197 deletions.
13 changes: 11 additions & 2 deletions src/yb/yql/cql/ql/ptree/pt_bcall.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ CHECKED_STATUS PTBcall::Analyze(SemContext *sem_context) {
int pindex = 0;
const MCList<PTExpr::SharedPtr>& exprs = args_->node_list();
vector<PTExpr::SharedPtr> params(exprs.size());
for (const auto& expr : exprs) {
for (const PTExpr::SharedPtr& expr : exprs) {
RETURN_NOT_OK(expr->Analyze(sem_context));
RETURN_NOT_OK(expr->CheckRhsExpr(sem_context));

Expand Down Expand Up @@ -145,6 +145,15 @@ CHECKED_STATUS PTBcall::Analyze(SemContext *sem_context) {
// Use the builtin-function opcode since this is a regular builtin call.
bfopcode_ = static_cast<int32_t>(bfopcode);

if (*name_ == "cql_cast" || *name_ == "tojson") {
// Argument must be of primitive type for these operators.
for (const PTExpr::SharedPtr &expr : exprs) {
if (expr->expr_op() == ExprOperator::kCollection) {
return sem_context->Error(expr, "Input argument must be of primitive type",
ErrorCode::INVALID_ARGUMENTS);
}
}
}
} else {
// Use the server opcode since this is a server operator. Ignore the BFOpcode.
is_server_operator_ = true;
Expand Down Expand Up @@ -380,7 +389,7 @@ CHECKED_STATUS PTBcall::CheckOperatorAfterArgAnalyze(SemContext *sem_context) {
}

if (type->Contains(FROZEN) || type->Contains(USER_DEFINED_TYPE)) {
// Only the server side implementation allows unwrapping complex types based on the schema.
// Only the server side implementation allows complex types unwrapping based on the schema.
name_->insert(0, "server_");
}
}
Expand Down
6 changes: 1 addition & 5 deletions src/yb/yql/cql/ql/test/ql-create-index-test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,7 @@ class Master;
namespace ql {

#define EXEC_DUPLICATE_OBJECT_CREATE_STMT(stmt) \
do { \
Status s = processor->Run(stmt); \
EXPECT_FALSE(s.ok()); \
EXPECT_FALSE(s.ToString().find("Duplicate Object. Object") == string::npos); \
} while (false)
EXEC_INVALID_STMT_WITH_ERROR(stmt, "Duplicate Object. Object")

class TestQLCreateIndex : public QLTestBase {
public:
Expand Down
79 changes: 34 additions & 45 deletions src/yb/yql/cql/ql/test/ql-create-table-test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,7 @@ class Master;
namespace ql {

#define EXEC_DUPLICATE_OBJECT_CREATE_STMT(stmt) \
do { \
Status s = processor->Run(stmt); \
EXPECT_FALSE(s.ok()); \
EXPECT_FALSE(s.ToString().find("Duplicate Object. Object") == string::npos); \
} while (false)

#define EXEC_INVALID_TABLE_CREATE_STMT(stmt, msg) \
do { \
Status s = processor->Run(stmt); \
ASSERT_FALSE(s.ok()); \
ASSERT_FALSE(s.ToString().find(msg) == string::npos); \
} while (false)
EXEC_INVALID_STMT_WITH_ERROR(stmt, "Duplicate Object. Object")

class TestQLCreateTable : public QLTestBase {
public:
Expand Down Expand Up @@ -319,21 +308,21 @@ TEST_F(TestQLCreateTable, TestQLCreateTableWithClusteringOrderBy) {
EXEC_VALID_STMT(CreateStmt(table4));
EXEC_VALID_STMT(CreateStmt(table5));

EXEC_INVALID_TABLE_CREATE_STMT(CreateStmt(table6),
EXEC_INVALID_STMT_WITH_ERROR(CreateStmt(table6),
"Invalid Table Property. Columns in the CLUSTERING ORDER directive must be in same order "
"as the clustering key columns order (first_name must appear before last_name)");
EXEC_INVALID_TABLE_CREATE_STMT(CreateStmt(table7),
EXEC_INVALID_STMT_WITH_ERROR(CreateStmt(table7),
"Invalid Table Property. Columns in the CLUSTERING ORDER directive must be in same order "
"as the clustering key columns order (first_name must appear before last_name)");
EXEC_INVALID_TABLE_CREATE_STMT(CreateStmt(table8),
EXEC_INVALID_STMT_WITH_ERROR(CreateStmt(table8),
"Invalid Table Property. Missing CLUSTERING ORDER for column first_name");
EXEC_INVALID_TABLE_CREATE_STMT(CreateStmt(table9),
EXEC_INVALID_STMT_WITH_ERROR(CreateStmt(table9),
"Invalid Table Property. Not a clustering key colum");
EXEC_INVALID_TABLE_CREATE_STMT(CreateStmt(table10),
EXEC_INVALID_STMT_WITH_ERROR(CreateStmt(table10),
"Invalid Table Property. Not a clustering key colum");
EXEC_INVALID_TABLE_CREATE_STMT(CreateStmt(table11),
EXEC_INVALID_STMT_WITH_ERROR(CreateStmt(table11),
"Invalid Table Property. Not a clustering key colum");
EXEC_INVALID_TABLE_CREATE_STMT(CreateStmt(table12),
EXEC_INVALID_STMT_WITH_ERROR(CreateStmt(table12),
"Invalid Table Property. Not a clustering key colum");
}

Expand Down Expand Up @@ -378,32 +367,32 @@ TEST_F(TestQLCreateTable, TestQLCreateTableWithPartitionScemeOf) {

EXEC_VALID_STMT(CreateStmt(table2));
EXEC_VALID_STMT(CreateStmt(table3));
EXEC_INVALID_TABLE_CREATE_STMT(CreateStmt(table4),
"The number of hash keys in the current table "
"differ from the number of hash keys in 'devices'");
EXEC_INVALID_TABLE_CREATE_STMT(CreateStmt(table5),
"The number of hash keys in the current table "
"differ from the number of hash keys in 'devices'");
EXEC_INVALID_TABLE_CREATE_STMT(CreateStmt(table6),
"Object Not Found");
EXEC_INVALID_TABLE_CREATE_STMT(CreateStmt(table7),
"The hash key 'description' in the current table has a different "
"datatype from the corresponding hash key in 'devices'");
EXEC_INVALID_TABLE_CREATE_STMT(CreateStmt(table8),
"The hash key 'dev_id' in the current table has a different "
"datatype from the corresponding hash key in 'devices'");
EXEC_INVALID_TABLE_CREATE_STMT(CreateStmt(table9),
"The hash key 'image_id' in the current table has a different "
"datatype from the corresponding hash key in 'devices'");
EXEC_INVALID_TABLE_CREATE_STMT(CreateStmt(table10),
"The hash key 'description' in the current table has a different "
"datatype from the corresponding hash key in 'devices'");
EXEC_INVALID_TABLE_CREATE_STMT(CreateStmt(table11),
"syntax error");
EXEC_INVALID_TABLE_CREATE_STMT(CreateStmt(table12),
"syntax error");
EXEC_INVALID_TABLE_CREATE_STMT(CreateStmt(table13),
"syntax error");
EXEC_INVALID_STMT_WITH_ERROR(CreateStmt(table4),
"The number of hash keys in the current table "
"differ from the number of hash keys in 'devices'");
EXEC_INVALID_STMT_WITH_ERROR(CreateStmt(table5),
"The number of hash keys in the current table "
"differ from the number of hash keys in 'devices'");
EXEC_INVALID_STMT_WITH_ERROR(CreateStmt(table6),
"Object Not Found");
EXEC_INVALID_STMT_WITH_ERROR(CreateStmt(table7),
"The hash key 'description' in the current table has a different "
"datatype from the corresponding hash key in 'devices'");
EXEC_INVALID_STMT_WITH_ERROR(CreateStmt(table8),
"The hash key 'dev_id' in the current table has a different "
"datatype from the corresponding hash key in 'devices'");
EXEC_INVALID_STMT_WITH_ERROR(CreateStmt(table9),
"The hash key 'image_id' in the current table has a different "
"datatype from the corresponding hash key in 'devices'");
EXEC_INVALID_STMT_WITH_ERROR(CreateStmt(table10),
"The hash key 'description' in the current table has a different "
"datatype from the corresponding hash key in 'devices'");
EXEC_INVALID_STMT_WITH_ERROR(CreateStmt(table11),
"syntax error");
EXEC_INVALID_STMT_WITH_ERROR(CreateStmt(table12),
"syntax error");
EXEC_INVALID_STMT_WITH_ERROR(CreateStmt(table13),
"syntax error");
}

// Check for presence of rows in system.metrics table.
Expand Down
69 changes: 31 additions & 38 deletions src/yb/yql/cql/ql/test/ql-drop-stmt-test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,6 @@
namespace yb {
namespace ql {

#define EXEC_INVALID_DROP_STMT(stmt, expected_error) \
do { \
Status s = processor->Run(stmt); \
ASSERT_FALSE(s.ok()); \
ASSERT_FALSE(s.ToString().find(expected_error) == string::npos); \
} while (false);

class TestQLDropStmt : public QLTestBase {
public:
TestQLDropStmt() : QLTestBase() {
Expand All @@ -48,7 +41,7 @@ TEST_F(TestQLDropStmt, TestQLDropTable) {
const string not_found_drop_error = "Object Not Found";

// No tables exist at this point. Verify that this statement fails.
EXEC_INVALID_DROP_STMT(drop_stmt, not_found_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_stmt, not_found_drop_error);

// Although the table doesn't exist, a DROP TABLE IF EXISTS should succeed.
EXEC_VALID_STMT(drop_cond_stmt);
Expand All @@ -60,7 +53,7 @@ TEST_F(TestQLDropStmt, TestQLDropTable) {
EXEC_VALID_STMT(drop_stmt);

// Verify that the table was indeed deleted.
EXEC_INVALID_DROP_STMT(drop_stmt, not_found_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_stmt, not_found_drop_error);

// Create the table again.
EXEC_VALID_STMT(create_stmt);
Expand All @@ -69,7 +62,7 @@ TEST_F(TestQLDropStmt, TestQLDropTable) {
EXEC_VALID_STMT(drop_cond_stmt);

// Verify that the table was indeed deleted.
EXEC_INVALID_DROP_STMT(drop_stmt, not_found_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_stmt, not_found_drop_error);
}

TEST_F(TestQLDropStmt, TestQLDropIndex) {
Expand All @@ -91,8 +84,8 @@ TEST_F(TestQLDropStmt, TestQLDropIndex) {
const string not_found_index_drop_error = "Object Not Found";

// No tables exist at this point. Verify that these statements fail.
EXEC_INVALID_DROP_STMT(drop_table_stmt, not_found_table_drop_error);
EXEC_INVALID_DROP_STMT(drop_index_stmt, not_found_index_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_table_stmt, not_found_table_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_index_stmt, not_found_index_drop_error);

// Although the table and the index doesn't exist, a DROP TABLE IF EXISTS should succeed.
EXEC_VALID_STMT(drop_table_cond_stmt);
Expand All @@ -107,8 +100,8 @@ TEST_F(TestQLDropStmt, TestQLDropIndex) {
EXEC_VALID_STMT(drop_table_stmt);

// Verify that the table and the index were indeed deleted.
EXEC_INVALID_DROP_STMT(drop_table_stmt, not_found_table_drop_error);
EXEC_INVALID_DROP_STMT(drop_index_stmt, not_found_index_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_table_stmt, not_found_table_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_index_stmt, not_found_index_drop_error);

// Create the table and index again.
EXEC_VALID_STMT(create_table_stmt);
Expand All @@ -119,8 +112,8 @@ TEST_F(TestQLDropStmt, TestQLDropIndex) {
EXEC_VALID_STMT(drop_table_cond_stmt);

// Verify that the table and the index were indeed deleted.
EXEC_INVALID_DROP_STMT(drop_table_stmt, not_found_table_drop_error);
EXEC_INVALID_DROP_STMT(drop_index_stmt, not_found_index_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_table_stmt, not_found_table_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_index_stmt, not_found_index_drop_error);

// Create the table and index again.
EXEC_VALID_STMT(create_table_stmt);
Expand All @@ -130,8 +123,8 @@ TEST_F(TestQLDropStmt, TestQLDropIndex) {
EXEC_VALID_STMT(drop_table_stmt);

// Verify that the table and the index were indeed deleted.
EXEC_INVALID_DROP_STMT(drop_table_stmt, not_found_table_drop_error);
EXEC_INVALID_DROP_STMT(drop_index_stmt, not_found_index_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_table_stmt, not_found_table_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_index_stmt, not_found_index_drop_error);
}

TEST_F(TestQLDropStmt, TestQLDropKeyspace) {
Expand Down Expand Up @@ -160,7 +153,7 @@ TEST_F(TestQLDropStmt, TestQLDropKeyspace) {
//----------------------------------------------------------------------------------------------

// No keyspaces exist at this point. Verify that this statement fails.
EXEC_INVALID_DROP_STMT(drop_keyspace, not_found_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_keyspace, not_found_drop_error);

// Although the keyspace doesn't exist, a DROP KEYSPACE IF EXISTS should succeed.
EXEC_VALID_STMT(drop_keyspace_cond);
Expand All @@ -172,7 +165,7 @@ TEST_F(TestQLDropStmt, TestQLDropKeyspace) {
EXEC_VALID_STMT(drop_keyspace);

// Verify that the keyspace was indeed deleted.
EXEC_INVALID_DROP_STMT(drop_keyspace, not_found_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_keyspace, not_found_drop_error);

// Create the keyspace again.
EXEC_VALID_STMT(create_keyspace);
Expand All @@ -181,7 +174,7 @@ TEST_F(TestQLDropStmt, TestQLDropKeyspace) {
EXEC_VALID_STMT(drop_keyspace_cond);

// Verify that the keyspace was indeed deleted.
EXEC_INVALID_DROP_STMT(drop_keyspace, not_found_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_keyspace, not_found_drop_error);

//----------------------------------------------------------------------------------------------
// Test DROP KEYSPACE for non-empty keyspace -- Containing a Table
Expand All @@ -195,7 +188,7 @@ TEST_F(TestQLDropStmt, TestQLDropKeyspace) {
EXEC_VALID_STMT(create_table);

// Cannot drop keyspace with type inside.
EXEC_INVALID_DROP_STMT(drop_keyspace, non_empty_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_keyspace, non_empty_drop_error);

// Create a type.
EXEC_VALID_STMT(drop_table);
Expand All @@ -215,7 +208,7 @@ TEST_F(TestQLDropStmt, TestQLDropKeyspace) {
EXEC_VALID_STMT(create_type);

// Cannot drop keyspace with type inside.
EXEC_INVALID_DROP_STMT(drop_keyspace, non_empty_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_keyspace, non_empty_drop_error);

// Create a type.
EXEC_VALID_STMT(drop_type);
Expand Down Expand Up @@ -246,21 +239,21 @@ TEST_F(TestQLDropStmt, TestQLDropType) {
//------------------------------------------------------------------------------------------------

// No types exist at this point. Verify that this statement fails.
EXEC_INVALID_DROP_STMT(drop_type, not_found_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_type, not_found_drop_error);

// Create the type and a table using it.
EXEC_VALID_STMT(create_type);
EXEC_VALID_STMT(create_table);

// Verify that we cannot drop the type (because the table is referencing it).
EXEC_INVALID_DROP_STMT(drop_type, type_in_use_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_type, type_in_use_error);

// Drop the table and verify we can now drop the type;
EXEC_VALID_STMT(drop_table);
EXEC_VALID_STMT(drop_type);

// Verify that the type was indeed deleted.
EXEC_INVALID_DROP_STMT(drop_type, not_found_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_type, not_found_drop_error);

//------------------------------------------------------------------------------------------------
// Test DROP TYPE with condition (IF NOT EXISTS)
Expand All @@ -274,14 +267,14 @@ TEST_F(TestQLDropStmt, TestQLDropType) {
EXEC_VALID_STMT(create_table);

// Verify that DROP TYPE IF EXISTS fails because table is referencing type.
EXEC_INVALID_DROP_STMT(drop_type, type_in_use_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_type, type_in_use_error);

// Drop the table and verify we can now drop the type;
EXEC_VALID_STMT(drop_table);
EXEC_VALID_STMT(drop_type_cond);

// Verify that the type was indeed deleted.
EXEC_INVALID_DROP_STMT(drop_type, not_found_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_type, not_found_drop_error);
}

TEST_F(TestQLDropStmt, TestQLDropStmtParser) {
Expand All @@ -299,12 +292,12 @@ TEST_F(TestQLDropStmt, TestQLDropStmtParser) {
const string expected_drop_error = CqlError(strlen("DROP "),
". DROP " + object + " statement not supported");
auto drop_stmt = "DROP " + object + " test";
EXEC_INVALID_DROP_STMT(drop_stmt, expected_drop_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_stmt, expected_drop_error);

const string expected_drop_if_exists_error =
CqlError(strlen("DROP "), ". DROP " + object + " IF EXISTS statement not supported");
auto drop_if_exists_stmt = "DROP " + object + " IF EXISTS test";
EXEC_INVALID_DROP_STMT(drop_if_exists_stmt, expected_drop_if_exists_error);
EXEC_INVALID_STMT_WITH_ERROR(drop_if_exists_stmt, expected_drop_if_exists_error);
}

vector<string> drop_types = {
Expand All @@ -323,13 +316,13 @@ TEST_F(TestQLDropStmt, TestQLDropStmtParser) {
};
for (const auto& drop_type : drop_types) {
auto stmt = "DROP " + drop_type + " test";
EXEC_INVALID_DROP_STMT(stmt, CqlError(strlen("DROP ")));
EXEC_INVALID_STMT_WITH_ERROR(stmt, CqlError(strlen("DROP ")));
}

vector<string> opt_drop_behaviors = {"CASCADE", "RESTRICT"};
for (const auto& opt_drop_behavior : opt_drop_behaviors) {
auto stmt = "DROP TABLE test ";
EXEC_INVALID_DROP_STMT(stmt + opt_drop_behavior, CqlError(strlen(stmt)));
EXEC_INVALID_STMT_WITH_ERROR(stmt + opt_drop_behavior, CqlError(strlen(stmt)));
}
}

Expand All @@ -342,15 +335,15 @@ TEST_F(TestQLDropStmt, TestQLDropStmtAnalyzer) {

string expected_drop_error = CqlError(strlen("DROP TABLE "),
". Only one object name is allowed in a drop statement");
EXEC_INVALID_DROP_STMT("DROP TABLE a, b", expected_drop_error);
EXEC_INVALID_DROP_STMT("DROP TABLE a, b, c", expected_drop_error);
EXEC_INVALID_DROP_STMT("DROP TABLE a, b, c, d", expected_drop_error);
EXEC_INVALID_STMT_WITH_ERROR("DROP TABLE a, b", expected_drop_error);
EXEC_INVALID_STMT_WITH_ERROR("DROP TABLE a, b, c", expected_drop_error);
EXEC_INVALID_STMT_WITH_ERROR("DROP TABLE a, b, c, d", expected_drop_error);

expected_drop_error = CqlError(strlen("DROP TABLE IF EXISTS "),
". Only one object name is allowed in a drop statement");
EXEC_INVALID_DROP_STMT("DROP TABLE IF EXISTS a, b", expected_drop_error);
EXEC_INVALID_DROP_STMT("DROP TABLE IF EXISTS a, b, c", expected_drop_error);
EXEC_INVALID_DROP_STMT("DROP TABLE IF EXISTS a, b, c, d", expected_drop_error);
EXEC_INVALID_STMT_WITH_ERROR("DROP TABLE IF EXISTS a, b", expected_drop_error);
EXEC_INVALID_STMT_WITH_ERROR("DROP TABLE IF EXISTS a, b, c", expected_drop_error);
EXEC_INVALID_STMT_WITH_ERROR("DROP TABLE IF EXISTS a, b, c, d", expected_drop_error);
}

} // namespace ql
Expand Down
Loading

0 comments on commit 2d20f37

Please sign in to comment.