Skip to content

Commit

Permalink
Report error when detecting invalid label or property (#513)
Browse files Browse the repository at this point in the history 
* Report error when detecting invalid label or property

* fix test case error

* fix test case error

* fix cpplint
  • Loading branch information
@ljcui
ljcui authored May 20, 2024
1 parent a8bfc83 commit cf84dce
Show file tree
Hide file tree
Showing 8 changed files with 138 additions and 31 deletions.
40 changes: 40 additions & 0 deletions src/cypher/execution_plan/execution_plan.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1291,6 +1291,46 @@ void ExecutionPlan::Build(const std::vector<parser::SglQuery> &stmt, parser::Cmd
pass_manager.ExecutePasses();
}

void ExecutionPlan::PreValidate(
cypher::RTContext *ctx,
const std::unordered_map<std::string, std::set<std::string>>& node,
const std::unordered_map<std::string, std::set<std::string>>& edge) {
if (node.empty() && edge.empty()) {
return;
}
if (ctx->graph_.empty()) {
return;
}
auto graph = ctx->galaxy_->OpenGraph(ctx->user_, ctx->graph_);
auto txn = graph.CreateReadTxn();
const auto& si = txn.GetSchemaInfo();
for (const auto& pair : node) {
auto s = si.v_schema_manager.GetSchema(pair.first);
if (!s) {
THROW_CODE(CypherException, "No such vertex label: {}", pair.first);
}
for (const auto& name : pair.second) {
size_t fid;
if (!s->TryGetFieldId(name, fid)) {
THROW_CODE(CypherException, "No such vertex property: {}.{}", pair.first, name);
}
}
}
for (const auto& pair : edge) {
auto s = si.e_schema_manager.GetSchema(pair.first);
if (!s) {
THROW_CODE(CypherException, "No such edge label: {}", pair.first);
}
for (const auto& name : pair.second) {
size_t fid;
if (!s->TryGetFieldId(name, fid)) {
THROW_CODE(CypherException, "No such edge property: {}.{}", pair.first, name);
}
}
}
txn.Abort();
}

void ExecutionPlan::Validate(cypher::RTContext *ctx) {
// todo(kehuang): Add validation manager here.
GraphNameChecker checker(_root, ctx);
Expand Down
3 changes: 3 additions & 0 deletions src/cypher/execution_plan/execution_plan.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,9 @@ class ExecutionPlan {
void Build(const std::vector<parser::SglQuery> &stmt, parser::CmdType cmd,
cypher::RTContext *ctx);

void PreValidate(cypher::RTContext *ctx,
const std::unordered_map<std::string, std::set<std::string>>& node,
const std::unordered_map<std::string, std::set<std::string>>& edge);
void Validate(cypher::RTContext *ctx);

void Reset();
Expand Down
1 change: 1 addition & 0 deletions src/cypher/execution_plan/scheduler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ void Scheduler::EvalCypher(RTContext *ctx, const std::string &script, ElapsedTim
LOG_DEBUG() << sql_query.ToString();
}
plan = std::make_shared<ExecutionPlan>();
plan->PreValidate(ctx, visitor.GetNodeProperty(), visitor.GetRelProperty());
plan->Build(visitor.GetQuery(), visitor.CommandType(), ctx);
plan->Validate(ctx);
if (plan->CommandType() != parser::CmdType::QUERY) {
Expand Down
26 changes: 25 additions & 1 deletion src/cypher/parser/cypher_base_visitor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@ class CypherBaseVisitor : public LcypherVisitor {
std::string _curr_procedure_name;
/* alias carry between query parts */
std::vector<std::pair<std::string, cypher::SymbolNode::Type>> _carry;
std::string _listcompr_placeholder = "";
std::string _listcompr_placeholder;
std::unordered_map<std::string, std::set<std::string>> _node_property;
std::unordered_map<std::string, std::set<std::string>> _rel_property;
enum _ClauseType : uint32_t {
NA = 0x0,
MATCH = 0x1,
Expand Down Expand Up @@ -145,6 +147,12 @@ class CypherBaseVisitor : public LcypherVisitor {

const std::vector<SglQuery> &GetQuery() const { return _query; }

const std::unordered_map<std::string, std::set<std::string>>&
GetNodeProperty() const {return _node_property;}

const std::unordered_map<std::string, std::set<std::string>>&
GetRelProperty() const {return _rel_property;}

CmdType CommandType() const { return _cmd_type; }

std::any visitOC_Cypher(LcypherParser::OC_CypherContext *ctx) override {
Expand Down Expand Up @@ -819,6 +827,13 @@ class CypherBaseVisitor : public LcypherVisitor {
if (ctx->oC_Properties() != nullptr) {
TUP_PROPERTIES tmp = std::any_cast<TUP_PROPERTIES>(visit(ctx->oC_Properties()));
properties = std::move(tmp);
for (const auto& pair : std::get<0>(properties).Map()) {
for (auto& label : node_labels) {
if (!label.empty()) {
_node_property[label].emplace(pair.first);
}
}
}
}
AddSymbol(variable, cypher::SymbolNode::NODE, cypher::SymbolNode::LOCAL);
return std::make_tuple(variable, node_labels, properties);
Expand Down Expand Up @@ -882,6 +897,13 @@ class CypherBaseVisitor : public LcypherVisitor {
if (ctx->oC_Properties() != nullptr) {
TUP_PROPERTIES tmp = std::any_cast<TUP_PROPERTIES>(visit(ctx->oC_Properties()));
properties = tmp;
for (const auto& pair : std::get<0>(properties).Map()) {
for (auto &rel_label : relationship_types) {
if (!rel_label.empty()) {
_rel_property[rel_label].emplace(pair.first);
}
}
}
}
AddSymbol(variable, cypher::SymbolNode::RELATIONSHIP, cypher::SymbolNode::LOCAL);
return std::make_tuple(variable, relationship_types, range_literal, properties);
Expand Down Expand Up @@ -916,6 +938,7 @@ class CypherBaseVisitor : public LcypherVisitor {
for (auto &ctx_name : ctx->oC_RelTypeName()) {
std::string name = std::any_cast<std::string>(visit(ctx_name));
relationship_types.emplace_back(name);
_rel_property.emplace(name, std::set<std::string>{});
}
return relationship_types;
}
Expand All @@ -930,6 +953,7 @@ class CypherBaseVisitor : public LcypherVisitor {
for (auto &ctx_label : ctx->oC_NodeLabel()) {
std::string label = std::any_cast<std::string>(visit(ctx_label));
labels.emplace_back(label);
_node_property.emplace(label, std::set<std::string>{});
}
return labels;
}
Expand Down
95 changes: 65 additions & 30 deletions test/test_cypher.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ int test_file_script(const std::string &file, cypher::RTContext *ctx) {
for (auto &p : s.parts) p.symbol_table.DumpTable();
}
cypher::ExecutionPlan execution_plan;
execution_plan.PreValidate(ctx, visitor.GetNodeProperty(), visitor.GetRelProperty());
execution_plan.Build(stmt, visitor.CommandType(), ctx);
execution_plan.Validate(ctx);
execution_plan.DumpGraph();
Expand All @@ -92,6 +93,7 @@ int test_interactive(cypher::RTContext *ctx) {
parser.addErrorListener(&CypherErrorListener::INSTANCE);
visitor.visit(parser.oC_Cypher());
cypher::ExecutionPlan execution_plan;
execution_plan.PreValidate(ctx, visitor.GetNodeProperty(), visitor.GetRelProperty());
execution_plan.Build(visitor.GetQuery(), visitor.CommandType(), ctx);
execution_plan.Validate(ctx);
execution_plan.Execute(ctx);
Expand All @@ -116,6 +118,7 @@ void eval_scripts_check(cypher::RTContext *ctx, const std::vector<std::string> &
parser.addErrorListener(&CypherErrorListener::INSTANCE);
CypherBaseVisitor visitor(ctx, parser.oC_Cypher());
cypher::ExecutionPlan execution_plan;
execution_plan.PreValidate(ctx, visitor.GetNodeProperty(), visitor.GetRelProperty());
execution_plan.Build(visitor.GetQuery(), visitor.CommandType(), ctx);
execution_plan.Validate(ctx);
execution_plan.DumpGraph();
Expand Down Expand Up @@ -181,6 +184,27 @@ int test_find(cypher::RTContext *ctx) {
return 0;
}

void test_invalid_schema(cypher::RTContext *ctx) {
std::string cypher;
cypher = "MATCH (n:Person_x {name:'Vanessa Redgrave'})-[:ACTED_IN]->(m) RETURN n,m.title";
UT_EXPECT_THROW_MSG(eval_script(ctx, cypher), "No such")
cypher = "MATCH (n:Person {name_x:'Vanessa Redgrave'})-[:ACTED_IN]->(m) RETURN n,m.title";
UT_EXPECT_THROW_MSG(eval_script(ctx, cypher), "No such")
cypher = "MATCH (n:Person {name:'Vanessa Redgrave'})-[:ACTED_IN_x]->(m) RETURN n,m.title";
UT_EXPECT_THROW_MSG(eval_script(ctx, cypher), "No such")
cypher = "MATCH (n)<-[relatedTo]-(vanessa:Person_x {name:'Vanessa'}) RETURN n,relatedTo";
UT_EXPECT_THROW_MSG(eval_script(ctx, cypher), "No such")
cypher = "MATCH (n)<-[relatedTo]-(vanessa:Person {name_x:'Vanessa'}) RETURN n,relatedTo";
UT_EXPECT_THROW_MSG(eval_script(ctx, cypher), "No such")
cypher = "CREATE (:City {name:'Shanghai'}), (:City_x {name:'Zhongshan'})";
UT_EXPECT_THROW_MSG(eval_script(ctx, cypher), "No such")
cypher = "CREATE (:City {name_x:'Shanghai'}), (:City {name:'Zhongshan'})";
UT_EXPECT_THROW_MSG(eval_script(ctx, cypher), "No such")
cypher = "MATCH (c {name:'Houston'}) WITH c "
"MATCH (p:Person {name:'Liam Neeson'}) CREATE (c)-[:HAS_CHILD_x]->(p)";
UT_EXPECT_THROW_MSG(eval_script(ctx, cypher), "No such")
}

int test_query(cypher::RTContext *ctx) {
static const std::vector<std::pair<std::string, int>> script_check = {
{"MATCH (n:Person {name:'Vanessa Redgrave'})-[:ACTED_IN]->(m) RETURN n,m.title", 1},
Expand Down Expand Up @@ -745,7 +769,8 @@ int test_expression(cypher::RTContext *ctx) {
{"MATCH (n {name:'Rachel Kempson'}) RETURN exists((n)-[]->(:Person)) /*true*/", 1},
{"MATCH (n {name:'Rachel Kempson'}) RETURN exists((n)-[]->(:City)) /*false*/", 1},
{"MATCH (n {name:'Rachel Kempson'}) RETURN exists((n)-[:MARRIED]->()) /*true*/", 1},
{"MATCH (n {name:'Rachel Kempson'}) RETURN exists((n)-[:NO_TYPE]->()) /*false*/", 1},
// TODO(botu.wzy)
// {"MATCH (n {name:'Rachel Kempson'}) RETURN exists((n)-[:NO_TYPE]->()) /*false*/", 1},
{"MATCH (n {name:'Rachel Kempson'}),(m {name:'Liam Neeson'}) RETURN exists((n)-[]->(m)) "
"/*false*/",
1},
Expand Down Expand Up @@ -1792,17 +1817,17 @@ int test_create_label(cypher::RTContext *ctx) {
"CALL db.createLabel('edge', 'WROTE_MUSIC_FOR', '[]')",
"CALL db.createLabel('edge', 'ACTED_IN', '[]', ['charactername', string, true])",
R"(
CREATE (rachel:Person:Actor {name: 'Rachel Kempson', birthyear: 1910})
CREATE (michael:Person:Actor {name: 'Michael Redgrave', birthyear: 1908})
CREATE (vanessa:Person:Actor {name: 'Vanessa Redgrave', birthyear: 1937})
CREATE (corin:Person:Actor {name: 'Corin Redgrave', birthyear: 1939})
CREATE (liam:Person:Actor {name: 'Liam Neeson', birthyear: 1952})
CREATE (natasha:Person:Actor {name: 'Natasha Richardson', birthyear: 1963})
CREATE (richard:Person:Actor {name: 'Richard Harris', birthyear: 1930})
CREATE (dennis:Person:Actor {name: 'Dennis Quaid', birthyear: 1954})
CREATE (lindsay:Person:Actor {name: 'Lindsay Lohan', birthyear: 1986})
CREATE (jemma:Person:Actor {name: 'Jemma Redgrave', birthyear: 1965})
CREATE (roy:Person:Actor {name: 'Roy Redgrave', birthyear: 1873})
CREATE (rachel:Person {name: 'Rachel Kempson', birthyear: 1910})
CREATE (michael:Person {name: 'Michael Redgrave', birthyear: 1908})
CREATE (vanessa:Person {name: 'Vanessa Redgrave', birthyear: 1937})
CREATE (corin:Person {name: 'Corin Redgrave', birthyear: 1939})
CREATE (liam:Person {name: 'Liam Neeson', birthyear: 1952})
CREATE (natasha:Person {name: 'Natasha Richardson', birthyear: 1963})
CREATE (richard:Person {name: 'Richard Harris', birthyear: 1930})
CREATE (dennis:Person {name: 'Dennis Quaid', birthyear: 1954})
CREATE (lindsay:Person {name: 'Lindsay Lohan', birthyear: 1986})
CREATE (jemma:Person {name: 'Jemma Redgrave', birthyear: 1965})
CREATE (roy:Person {name: 'Roy Redgrave', birthyear: 1873})
CREATE (john:Person {name: 'John Williams', birthyear: 1932})
CREATE (christopher:Person {name: 'Christopher Nolan', birthyear: 1970})
Expand Down Expand Up @@ -1869,17 +1894,17 @@ int test_create_yago(cypher::RTContext *ctx) {
"CALL db.createEdgeLabel('WROTE_MUSIC_FOR', '[]')",
"CALL db.createEdgeLabel('ACTED_IN', '[]', 'charactername', STRING, false)",
R"(
CREATE (rachel:Person:Actor {name: 'Rachel Kempson', birthyear: 1910})
CREATE (michael:Person:Actor {name: 'Michael Redgrave', birthyear: 1908})
CREATE (vanessa:Person:Actor {name: 'Vanessa Redgrave', birthyear: 1937})
CREATE (corin:Person:Actor {name: 'Corin Redgrave', birthyear: 1939})
CREATE (liam:Person:Actor {name: 'Liam Neeson', birthyear: 1952})
CREATE (natasha:Person:Actor {name: 'Natasha Richardson', birthyear: 1963})
CREATE (richard:Person:Actor {name: 'Richard Harris', birthyear: 1930})
CREATE (dennis:Person:Actor {name: 'Dennis Quaid', birthyear: 1954})
CREATE (lindsay:Person:Actor {name: 'Lindsay Lohan', birthyear: 1986})
CREATE (jemma:Person:Actor {name: 'Jemma Redgrave', birthyear: 1965})
CREATE (roy:Person:Actor {name: 'Roy Redgrave', birthyear: 1873})
CREATE (rachel:Person {name: 'Rachel Kempson', birthyear: 1910})
CREATE (michael:Person {name: 'Michael Redgrave', birthyear: 1908})
CREATE (vanessa:Person {name: 'Vanessa Redgrave', birthyear: 1937})
CREATE (corin:Person {name: 'Corin Redgrave', birthyear: 1939})
CREATE (liam:Person {name: 'Liam Neeson', birthyear: 1952})
CREATE (natasha:Person {name: 'Natasha Richardson', birthyear: 1963})
CREATE (richard:Person {name: 'Richard Harris', birthyear: 1930})
CREATE (dennis:Person {name: 'Dennis Quaid', birthyear: 1954})
CREATE (lindsay:Person {name: 'Lindsay Lohan', birthyear: 1986})
CREATE (jemma:Person {name: 'Jemma Redgrave', birthyear: 1965})
CREATE (roy:Person {name: 'Roy Redgrave', birthyear: 1873})
CREATE (john:Person {name: 'John Williams', birthyear: 1932})
CREATE (christopher:Person {name: 'Christopher Nolan', birthyear: 1970})
Expand Down Expand Up @@ -2137,11 +2162,14 @@ int test_opt(cypher::RTContext *ctx) {
{"MATCH ()-[r]->(:Person) RETURN count(r) /* 11 */", 1},
{"MATCH ()-[r]->(:Film) RETURN count(r) /* 11 */", 1},
{"MATCH (:Person)-[r]->(:Film) RETURN count(r) /* 11 */", 1},
{"MATCH (:Person)-[r]->(:NO_LABEL) RETURN count(r) /* 0 */", 1},
// TODO(botu.wzy)
// {"MATCH (:Person)-[r]->(:NO_LABEL) RETURN count(r) /* 0 */", 1},
{"MATCH ()-[r:MARRIED]->() RETURN count(r) /* 4 */", 1},
{"MATCH ()-[r:NO_LABEL]->() RETURN count(r) /* 0 */", 1},
// TODO(botu.wzy)
// {"MATCH ()-[r:NO_LABEL]->() RETURN count(r) /* 0 */", 1},
{"MATCH ()-[r:MARRIED|BORN_IN]->() RETURN count(r) /* 10 */", 1},
{"MATCH ()-[r:MARRIED|BORN_IN|NO_LABEL]->() RETURN count(r) /* 10 */", 1},
// TODO(botu.wzy)
// {"MATCH ()-[r:MARRIED|BORN_IN|NO_LABEL]->() RETURN count(r) /* 10 */", 1},
{"MATCH (:Person)-[r:MARRIED|BORN_IN]->() RETURN count(r) /* 10 */", 1},
{"MATCH (:City)-[r:MARRIED|BORN_IN]->() RETURN count(r) /* 0 */", 1},
{"MATCH ()-[r:MARRIED|BORN_IN]->(:Person) RETURN count(r) /* 4 */", 1},
Expand All @@ -2156,11 +2184,14 @@ int test_opt(cypher::RTContext *ctx) {
{"MATCH ()-[r]-(:Person) RETURN count(r) /* 39 */", 1},
{"MATCH ()-[r]-(:Film) RETURN count(r) /* 11 */", 1},
{"MATCH (:Person)-[r]-(:Film) RETURN count(r) /* 11 */", 1},
{"MATCH (:Person)-[r]-(:NO_LABEL) RETURN count(r) /* 0 */", 1},
// TODO(botu.wzy)
// {"MATCH (:Person)-[r]-(:NO_LABEL) RETURN count(r) /* 0 */", 1},
{"MATCH ()-[r:MARRIED]-() RETURN count(r) /* 8 */", 1},
{"MATCH ()-[r:NO_LABEL]-() RETURN count(r) /* 0 */", 1},
// TODO(botu.wzy)
// {"MATCH ()-[r:NO_LABEL]-() RETURN count(r) /* 0 */", 1},
{"MATCH ()-[r:MARRIED|BORN_IN]-() RETURN count(r) /* 20 */", 1},
{"MATCH ()-[r:MARRIED|BORN_IN|NO_LABEL]-() RETURN count(r) /* 20 */", 1},
// TODO(botu.wzy)
// {"MATCH ()-[r:MARRIED|BORN_IN|NO_LABEL]-() RETURN count(r) /* 20 */", 1},
{"MATCH (:Person)-[r:MARRIED|BORN_IN]-() RETURN count(r) /* 14 */", 1},
{"MATCH (:City)-[r:MARRIED|BORN_IN]-() RETURN count(r) /* 6 */", 1},
{"MATCH ()-[r:MARRIED|BORN_IN]-(:Person) RETURN count(r) /* 14 */", 1},
Expand Down Expand Up @@ -2494,6 +2525,7 @@ enum TestCase {
TC_CREATE_LABEL = 404,
TC_READONLY = 500,
TC_EDGE_ID = 501,
TC_INVALID_SCHEMA = 502,
TC_EMPTY_GRAPH = 700,
};

Expand Down Expand Up @@ -2682,6 +2714,9 @@ TEST_P(TestCypher, Cypher) {
case TC_EMPTY_GRAPH:
TestCypherEmptyGraph(&db);
break;
case TC_INVALID_SCHEMA:
test_invalid_schema(&db);
break;
default:
break;
}
Expand All @@ -2702,4 +2737,4 @@ INSTANTIATE_TEST_CASE_P(
ParamCypher{106, 1}, ParamCypher{107, 1}, ParamCypher{108, 2}, ParamCypher{109, 2},
ParamCypher{110, 2}, ParamCypher{111, 2}, ParamCypher{112, 1}, ParamCypher{113, 1},
ParamCypher{301, 2}, ParamCypher{401, 1}, ParamCypher{402, 1}, ParamCypher{403, 1},
ParamCypher{404, 2}, ParamCypher{500, 0}, ParamCypher{501, 1}));
ParamCypher{404, 2}, ParamCypher{500, 0}, ParamCypher{501, 1}, ParamCypher{502, 1}));
1 change: 1 addition & 0 deletions test/test_cypher_plan.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ void eval_query_check(cypher::RTContext *ctx, const std::string &query,

double t0, t1, t2;
t0 = fma_common::GetTime();
execution_plan.PreValidate(ctx, visitor.GetNodeProperty(), visitor.GetRelProperty());
execution_plan.Build(visitor.GetQuery(), visitor.CommandType(), ctx);
execution_plan.DumpGraph();
std::string res_plan = execution_plan.DumpPlan(0, false);
Expand Down
1 change: 1 addition & 0 deletions test/test_edge_constraint.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ static void eval_scripts(cypher::RTContext *ctx, const std::vector<std::string>
parser.addErrorListener(&CypherErrorListener::INSTANCE);
CypherBaseVisitor visitor(ctx, parser.oC_Cypher());
cypher::ExecutionPlan execution_plan;
execution_plan.PreValidate(ctx, visitor.GetNodeProperty(), visitor.GetRelProperty());
execution_plan.Build(visitor.GetQuery(), visitor.CommandType(), ctx);
execution_plan.Validate(ctx);
execution_plan.DumpGraph();
Expand Down
2 changes: 2 additions & 0 deletions test/test_query.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,8 @@ class TestQuery : public TuGraphTest {
LOG_INFO() << sql_query.ToString();
}
cypher::ExecutionPlan execution_plan;
execution_plan.PreValidate(ctx_.get(), visitor.GetNodeProperty(),
visitor.GetRelProperty());
execution_plan.Build(visitor.GetQuery(), visitor.CommandType(), ctx_.get());
execution_plan.Validate(ctx_.get());
LOG_INFO() << execution_plan.DumpGraph();
Expand Down

0 comments on commit cf84dce

Please sign in to comment.