Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix property pruner do not parse none exist tag correctly #4949

Merged
merged 11 commits into from
Nov 29, 2022
Merged

Fix property pruner do not parse none exist tag correctly #4949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a1bc64a
Fix property pruner do not parse none exist tag correctly
codesigner Nov 29, 2022
f0f3423
refine tck
codesigner Nov 29, 2022
868f03c
refine tck
codesigner Nov 29, 2022
0ae1b57
refine code
codesigner Nov 29, 2022
707a7bd
refine tck format
codesigner Nov 29, 2022
7d0f455
fix ut
codesigner Nov 29, 2022
fab4d9f
fix tck error msg
codesigner Nov 29, 2022
24de8f8
remove tck tag
codesigner Nov 29, 2022
0b52b57
remove tck tag
codesigner Nov 29, 2022
bdcf9b3
fix tck
codesigner Nov 29, 2022
0587d59
Merge branch 'master' into bugfix_prop_pruner_on_invalid_tag
yixinglu Nov 29, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 33 additions & 33 deletions src/clients/meta/MetaClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,13 +829,13 @@ Status MetaClient::handleResponse(const RESP& resp) {
case nebula::cpp2::ErrorCode::E_EXISTED:
return Status::Error("Existed!");
case nebula::cpp2::ErrorCode::E_SPACE_NOT_FOUND:
return Status::Error("Space not existed!");
return Status::SpaceNotFound("Space not existed!");
case nebula::cpp2::ErrorCode::E_TAG_NOT_FOUND:
return Status::Error("Tag not existed!");
return Status::TagNotFound("Tag not existed!");
case nebula::cpp2::ErrorCode::E_EDGE_NOT_FOUND:
return Status::Error("Edge not existed!");
return Status::EdgeNotFound("Edge not existed!");
case nebula::cpp2::ErrorCode::E_INDEX_NOT_FOUND:
return Status::Error("Index not existed!");
return Status::IndexNotFound("Index not existed!");
case nebula::cpp2::ErrorCode::E_STATS_NOT_FOUND:
return Status::Error(
"There is no any stats info to show, please execute "
Expand Down Expand Up @@ -1389,7 +1389,7 @@ StatusOr<GraphSpaceID> MetaClient::getSpaceIdByNameFromCache(const std::string&
if (it != metadata.spaceIndexByName_.end()) {
return it->second;
}
return Status::SpaceNotFound();
return Status::SpaceNotFound(fmt::format("SpaceName `{}`", name));
}

StatusOr<std::string> MetaClient::getSpaceNameByIdFromCache(GraphSpaceID spaceId) {
Expand All @@ -1401,7 +1401,7 @@ StatusOr<std::string> MetaClient::getSpaceNameByIdFromCache(GraphSpaceID spaceId
auto spaceIt = metadata.localCache_.find(spaceId);
if (spaceIt == metadata.localCache_.end()) {
LOG(ERROR) << "Space " << spaceId << " not found!";
return Status::Error("Space %d not found", spaceId);
return Status::SpaceNotFound(fmt::format("SpaceId `{}`", spaceId));
}
return spaceIt->second->spaceDesc_.get_space_name();
}
Expand All @@ -1415,7 +1415,7 @@ StatusOr<TagID> MetaClient::getTagIDByNameFromCache(const GraphSpaceID& space,
const auto& metadata = *metadata_.load();
auto it = metadata.spaceTagIndexByName_.find(std::make_pair(space, name));
if (it == metadata.spaceTagIndexByName_.end()) {
return Status::Error("TagName `%s' is nonexistent", name.c_str());
return Status::TagNotFound(fmt::format("TagName `{}`", name));
}
return it->second;
}
Expand All @@ -1429,7 +1429,7 @@ StatusOr<std::string> MetaClient::getTagNameByIdFromCache(const GraphSpaceID& sp
const auto& metadata = *metadata_.load();
auto it = metadata.spaceTagIndexById_.find(std::make_pair(space, tagId));
if (it == metadata.spaceTagIndexById_.end()) {
return Status::Error("TagID `%d' is nonexistent", tagId);
return Status::TagNotFound(fmt::format("TagID `{}`", tagId));
}
return it->second;
}
Expand All @@ -1443,7 +1443,7 @@ StatusOr<EdgeType> MetaClient::getEdgeTypeByNameFromCache(const GraphSpaceID& sp
const auto& metadata = *metadata_.load();
auto it = metadata.spaceEdgeIndexByName_.find(std::make_pair(space, name));
if (it == metadata.spaceEdgeIndexByName_.end()) {
return Status::Error("EdgeName `%s' is nonexistent", name.c_str());
return Status::EdgeNotFound(fmt::format("EdgeName `{}`", name));
}
return it->second;
}
Expand All @@ -1457,7 +1457,7 @@ StatusOr<std::string> MetaClient::getEdgeNameByTypeFromCache(const GraphSpaceID&
const auto& metadata = *metadata_.load();
auto it = metadata.spaceEdgeIndexByType_.find(std::make_pair(space, edgeType));
if (it == metadata.spaceEdgeIndexByType_.end()) {
return Status::Error("EdgeType `%d' is nonexistent", edgeType);
return Status::EdgeNotFound(fmt::format("EdgeType `{}`", edgeType));
}
return it->second;
}
Expand All @@ -1470,7 +1470,7 @@ StatusOr<std::vector<std::string>> MetaClient::getAllEdgeFromCache(const GraphSp
const auto& metadata = *metadata_.load();
auto it = metadata.spaceAllEdgeMap_.find(space);
if (it == metadata.spaceAllEdgeMap_.end()) {
return Status::Error("SpaceId `%d' is nonexistent", space);
return Status::SpaceNotFound(fmt::format("SpaceId `{}`", space));
}
return it->second;
}
Expand Down Expand Up @@ -1542,7 +1542,7 @@ StatusOr<int32_t> MetaClient::partsNum(GraphSpaceID spaceId) {
const auto& metadata = *metadata_.load();
auto it = metadata.localCache_.find(spaceId);
if (it == metadata.localCache_.end()) {
return Status::Error("Space not found, spaceid: %d", spaceId);
return Status::SpaceNotFound(fmt::format("SpaceId `{}`", spaceId));
}
return it->second->partsAlloc_.size();
}
Expand Down Expand Up @@ -1937,7 +1937,7 @@ StatusOr<int32_t> MetaClient::getSpaceVidLen(const GraphSpaceID& spaceId) {
auto spaceIt = metadata.localCache_.find(spaceId);
if (spaceIt == metadata.localCache_.end()) {
LOG(ERROR) << "Space " << spaceId << " not found!";
return Status::Error("Space %d not found", spaceId);
return Status::SpaceNotFound(fmt::format("SpaceId `{}`", spaceId));
}
auto& vidType = spaceIt->second->spaceDesc_.get_vid_type();
auto vIdLen = vidType.type_length_ref().has_value() ? *vidType.get_type_length() : 0;
Expand All @@ -1956,7 +1956,7 @@ StatusOr<nebula::cpp2::PropertyType> MetaClient::getSpaceVidType(const GraphSpac
auto spaceIt = metadata.localCache_.find(spaceId);
if (spaceIt == metadata.localCache_.end()) {
LOG(ERROR) << "Space " << spaceId << " not found!";
return Status::Error("Space %d not found", spaceId);
return Status::SpaceNotFound(fmt::format("SpaceId `{}`", spaceId));
}
auto vIdType = spaceIt->second->spaceDesc_.get_vid_type().get_type();
if (vIdType != nebula::cpp2::PropertyType::INT64 &&
Expand All @@ -1969,16 +1969,16 @@ StatusOr<nebula::cpp2::PropertyType> MetaClient::getSpaceVidType(const GraphSpac
return vIdType;
}

StatusOr<cpp2::SpaceDesc> MetaClient::getSpaceDesc(const GraphSpaceID& space) {
StatusOr<cpp2::SpaceDesc> MetaClient::getSpaceDesc(const GraphSpaceID& spaceId) {
if (!ready_) {
return Status::Error("Not ready!");
}
folly::rcu_reader guard;
const auto& metadata = *metadata_.load();
auto spaceIt = metadata.localCache_.find(space);
auto spaceIt = metadata.localCache_.find(spaceId);
if (spaceIt == metadata.localCache_.end()) {
LOG(ERROR) << "Space " << space << " not found!";
return Status::Error("Space %d not found", space);
LOG(ERROR) << "Space " << spaceId << " not found!";
return Status::SpaceNotFound(fmt::format("SpaceId `{}`", spaceId));
}
return spaceIt->second->spaceDesc_;
}
Expand Down Expand Up @@ -2042,7 +2042,7 @@ StatusOr<TagSchemas> MetaClient::getAllVerTagSchema(GraphSpaceID spaceId) {
const auto& metadata = *metadata_.load();
auto iter = metadata.localCache_.find(spaceId);
if (iter == metadata.localCache_.end()) {
return Status::Error("Space %d not found", spaceId);
return Status::SpaceNotFound(fmt::format("SpaceId `{}`", spaceId));
}
return iter->second->tagSchemas_;
}
Expand All @@ -2055,7 +2055,7 @@ StatusOr<TagSchema> MetaClient::getAllLatestVerTagSchema(const GraphSpaceID& spa
const auto& metadata = *metadata_.load();
auto iter = metadata.localCache_.find(spaceId);
if (iter == metadata.localCache_.end()) {
return Status::Error("Space %d not found", spaceId);
return Status::SpaceNotFound(fmt::format("SpaceId `{}`", spaceId));
}
TagSchema tagsSchema;
tagsSchema.reserve(iter->second->tagSchemas_.size());
Expand All @@ -2074,7 +2074,7 @@ StatusOr<EdgeSchemas> MetaClient::getAllVerEdgeSchema(GraphSpaceID spaceId) {
const auto& metadata = *metadata_.load();
auto iter = metadata.localCache_.find(spaceId);
if (iter == metadata.localCache_.end()) {
return Status::Error("Space %d not found", spaceId);
return Status::SpaceNotFound(fmt::format("SpaceId `{}`", spaceId));
}
return iter->second->edgeSchemas_;
}
Expand All @@ -2087,7 +2087,7 @@ StatusOr<EdgeSchema> MetaClient::getAllLatestVerEdgeSchemaFromCache(const GraphS
const auto& metadata = *metadata_.load();
auto iter = metadata.localCache_.find(spaceId);
if (iter == metadata.localCache_.end()) {
return Status::Error("Space %d not found", spaceId);
return Status::SpaceNotFound(fmt::format("SpaceId `{}`", spaceId));
}
EdgeSchema edgesSchema;
edgesSchema.reserve(iter->second->edgeSchemas_.size());
Expand Down Expand Up @@ -2140,7 +2140,7 @@ StatusOr<std::shared_ptr<cpp2::IndexItem>> MetaClient::getTagIndexByNameFromCach
std::pair<GraphSpaceID, std::string> key(space, name);
auto iter = tagNameIndexMap_.find(key);
if (iter == tagNameIndexMap_.end()) {
return Status::IndexNotFound();
return Status::IndexNotFound(fmt::format("Index: {}:{}", space, name));
}
auto indexID = iter->second;
auto itemStatus = getTagIndexFromCache(space, indexID);
Expand All @@ -2158,7 +2158,7 @@ StatusOr<std::shared_ptr<cpp2::IndexItem>> MetaClient::getEdgeIndexByNameFromCac
std::pair<GraphSpaceID, std::string> key(space, name);
auto iter = edgeNameIndexMap_.find(key);
if (iter == edgeNameIndexMap_.end()) {
return Status::IndexNotFound();
return Status::IndexNotFound(fmt::format("Index: {}:{}", space, name));
}
auto indexID = iter->second;
auto itemStatus = getEdgeIndexFromCache(space, indexID);
Expand All @@ -2179,7 +2179,7 @@ StatusOr<std::shared_ptr<cpp2::IndexItem>> MetaClient::getTagIndexFromCache(Grap
auto spaceIt = metadata.localCache_.find(spaceId);
if (spaceIt == metadata.localCache_.end()) {
VLOG(3) << "Space " << spaceId << " not found!";
return Status::SpaceNotFound();
return Status::SpaceNotFound(fmt::format("SpaceId `{}`", spaceId));
} else {
auto iter = spaceIt->second->tagIndexes_.find(indexID);
if (iter == spaceIt->second->tagIndexes_.end()) {
Expand Down Expand Up @@ -2207,7 +2207,7 @@ StatusOr<TagID> MetaClient::getRelatedTagIDByIndexNameFromCache(const GraphSpace
}

StatusOr<std::shared_ptr<cpp2::IndexItem>> MetaClient::getEdgeIndexFromCache(GraphSpaceID spaceId,
IndexID indexID) {
IndexID indexId) {
if (!ready_) {
return Status::Error("Not ready!");
}
Expand All @@ -2217,12 +2217,12 @@ StatusOr<std::shared_ptr<cpp2::IndexItem>> MetaClient::getEdgeIndexFromCache(Gra
auto spaceIt = metadata.localCache_.find(spaceId);
if (spaceIt == metadata.localCache_.end()) {
VLOG(3) << "Space " << spaceId << " not found!";
return Status::SpaceNotFound();
return Status::SpaceNotFound(fmt::format("SpaceId `{}`", spaceId));
} else {
auto iter = spaceIt->second->edgeIndexes_.find(indexID);
auto iter = spaceIt->second->edgeIndexes_.find(indexId);
if (iter == spaceIt->second->edgeIndexes_.end()) {
VLOG(3) << "Space " << spaceId << ", Edge Index " << indexID << " not found!";
return Status::IndexNotFound();
VLOG(3) << "Space " << spaceId << ", Edge Index " << indexId << " not found!";
return Status::IndexNotFound(fmt::format("Index: {}:{}", spaceId, indexId));
} else {
return iter->second;
}
Expand Down Expand Up @@ -2255,7 +2255,7 @@ StatusOr<std::vector<std::shared_ptr<cpp2::IndexItem>>> MetaClient::getTagIndexe
auto spaceIt = metadata.localCache_.find(spaceId);
if (spaceIt == metadata.localCache_.end()) {
VLOG(3) << "Space " << spaceId << " not found!";
return Status::SpaceNotFound();
return Status::SpaceNotFound(fmt::format("SpaceId `{}`", spaceId));
} else {
auto tagIndexes = spaceIt->second->tagIndexes_;
auto iter = tagIndexes.begin();
Expand All @@ -2279,7 +2279,7 @@ StatusOr<std::vector<std::shared_ptr<cpp2::IndexItem>>> MetaClient::getEdgeIndex
auto spaceIt = metadata.localCache_.find(spaceId);
if (spaceIt == metadata.localCache_.end()) {
VLOG(3) << "Space " << spaceId << " not found!";
return Status::SpaceNotFound();
return Status::SpaceNotFound(fmt::format("SpaceId `{}`", spaceId));
} else {
auto edgeIndexes = spaceIt->second->edgeIndexes_;
auto iter = edgeIndexes.begin();
Expand Down Expand Up @@ -2490,7 +2490,7 @@ StatusOr<SchemaVer> MetaClient::getLatestEdgeVersionFromCache(const GraphSpaceID
const auto& metadata = *metadata_.load();
auto it = metadata.spaceNewestEdgeVerMap_.find(std::make_pair(space, edgeType));
if (it == metadata.spaceNewestEdgeVerMap_.end()) {
return Status::EdgeNotFound();
return Status::EdgeNotFound(fmt::format("EdgeType `{}`", edgeType));
}
return it->second;
}
Expand Down
12 changes: 6 additions & 6 deletions src/graph/validator/test/MockSchemaManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ StatusOr<GraphSpaceID> MockSchemaManager::toGraphSpaceID(folly::StringPiece spac
if (findIt != spaceNameIds_.end()) {
return findIt->second;
}
return Status::Error("Space `%s' not found", spaceName.str().c_str());
return Status::SpaceNotFound("Space `%s' not found", spaceName.str().c_str());
}

StatusOr<std::string> MockSchemaManager::toGraphSpaceName(GraphSpaceID space) {
Expand All @@ -108,7 +108,7 @@ StatusOr<std::string> MockSchemaManager::toGraphSpaceName(GraphSpaceID space) {
return s.first;
}
}
return Status::Error("Space `%d' not found", space);
return Status::SpaceNotFound("Space `%d' not found", space);
}

StatusOr<TagID> MockSchemaManager::toTagID(GraphSpaceID space, const folly::StringPiece tagName) {
Expand All @@ -120,7 +120,7 @@ StatusOr<TagID> MockSchemaManager::toTagID(GraphSpaceID space, const folly::Stri
if (tagFindIt != tagNameIds_.end()) {
return tagFindIt->second;
}
return Status::Error("TagName `%s' not found", tagName.str().c_str());
return Status::TagNotFound("TagName `%s' not found", tagName.str().c_str());
}

StatusOr<std::string> MockSchemaManager::toTagName(GraphSpaceID space, TagID tagId) {
Expand All @@ -132,7 +132,7 @@ StatusOr<std::string> MockSchemaManager::toTagName(GraphSpaceID space, TagID tag
if (tagFindIt != tagIdNames_.end()) {
return tagFindIt->second;
}
return Status::Error("TagID `%d' not found", tagId);
return Status::TagNotFound("TagID `%d' not found", tagId);
}

StatusOr<EdgeType> MockSchemaManager::toEdgeType(GraphSpaceID space, folly::StringPiece typeName) {
Expand All @@ -144,7 +144,7 @@ StatusOr<EdgeType> MockSchemaManager::toEdgeType(GraphSpaceID space, folly::Stri
if (edgeFindIt != edgeNameIds_.end()) {
return edgeFindIt->second;
}
return Status::Error("EdgeName `%s' not found", typeName.str().c_str());
return Status::EdgeNotFound("EdgeName `%s' not found", typeName.str().c_str());
}

StatusOr<std::string> MockSchemaManager::toEdgeName(GraphSpaceID space, EdgeType edgeType) {
Expand All @@ -156,7 +156,7 @@ StatusOr<std::string> MockSchemaManager::toEdgeName(GraphSpaceID space, EdgeType
if (edgeFindIt != edgeIdNames_.end()) {
return edgeFindIt->second;
}
return Status::Error("EdgeType `%d' not found", edgeType);
return Status::EdgeNotFound("EdgeType `%d' not found", edgeType);
}

StatusOr<std::vector<std::string>> MockSchemaManager::getAllEdge(GraphSpaceID) {
Expand Down
7 changes: 7 additions & 0 deletions src/graph/visitor/PrunePropertiesVisitor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,13 @@ void PrunePropertiesVisitor::visitCurrent(Project *node) {
auto *expr = col->expr();
status_ = extractPropsFromExpr(expr);
if (!status_.ok()) {
// Project a not exit tag, should not break other columns
// e.g. Vertex tage {{"name", "string"}, {"age", "int64"}}
// Project "... RETURN v.name, v.xxx.yyy, v.player.age"
// v.xxx.yyy should not break v.player.age
if (status_.isTagNotFound()) {
continue;
}
return;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/meta/test/MetaClientTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,7 @@ TEST(MetaClientTest, TagIndexTest) {
client->createTagIndex(space, "tag_not_exist_index", "tag_not_exist", std::move(fields))
.get();
ASSERT_FALSE(result.ok());
ASSERT_EQ(Status::Error("not existed!"), result.status());
ASSERT_EQ(Status::TagNotFound("not existed!"), result.status());
}
{
std::vector<cpp2::IndexFieldDef> fields;
Expand Down
2 changes: 1 addition & 1 deletion tests/tck/features/mutate/ClearSpace.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ Feature: Clear space test
"""
CLEAR SPACE clear_space_0;
"""
Then a ExecutionError should be raised at runtime: Space not existed!
Then a ExecutionError should be raised at runtime: SpaceNotFound: Space not existed!
And drop the used space

Scenario: Clear space function test
Expand Down
27 changes: 27 additions & 0 deletions tests/tck/features/optimizer/PrunePropertiesRule.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -851,3 +851,30 @@ Feature: Prune Properties rule
| __NULL__ | __NULL__ | NULL | NULL | NULL | __NULL__ |
| __NULL__ | __NULL__ | NULL | NULL | NULL | __NULL__ |
Then drop the used space

Scenario: Project on not exist tag
Given a graph with space named "nba"
When executing query:
"""
MATCH (v:player)-[e:like]->(t) WHERE v.player.name=='Tim Duncan' RETURN v.player.name, v.x.y, v.player.age
"""
Then the result should be, in order, with relax comparison:
| v.player.name | v.x.y | v.player.age |
| "Tim Duncan" | __NULL__ | 42 |
| "Tim Duncan" | __NULL__ | 42 |
When executing query:
"""
MATCH (v:player)-[:like]->(t) WHERE v.player.name=="Tim Duncan" RETURN v.player.name, properties(v), t
"""
Then the result should be, in order, with relax comparison:
| v.player.name | properties(v) | t |
| "Tim Duncan" | {age: 42, name: "Tim Duncan", speciality: "psychology"} | ("Tony Parker" :player{age: 36, name: "Tony Parker"}) |
| "Tim Duncan" | {age: 42, name: "Tim Duncan", speciality: "psychology"} | ("Manu Ginobili" :player{age: 41, name: "Manu Ginobili"}) |
When executing query:
"""
MATCH (v:player)-[:like]->(t) WHERE v.player.name=="Tim Duncan" RETURN v.player.name, t.errortag.name, properties(v), t
"""
Then the result should be, in order, with relax comparison:
| v.player.name | t.errortag.name | properties(v) | t |
| "Tim Duncan" | __NULL__ | {age: 42, name: "Tim Duncan", speciality: "psychology"} | ("Tony Parker" :player{age: 36, name: "Tony Parker"}) |
| "Tim Duncan" | __NULL__ | {age: 42, name: "Tim Duncan", speciality: "psychology"} | ("Manu Ginobili" :player{age: 41, name: "Manu Ginobili"}) |
8 changes: 4 additions & 4 deletions tests/tck/features/schema/Schema.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ Feature: Insert string vid of vertex and edge
"""
DESCRIBE TAG not_exist
"""
Then a ExecutionError should be raised at runtime: Tag not existed!
Then a ExecutionError should be raised at runtime: TagNotFound: Tag not existed!
# unreserved keyword
When executing query:
"""
Expand Down Expand Up @@ -193,7 +193,7 @@ Feature: Insert string vid of vertex and edge
"""
DROP TAG not_exist_tag
"""
Then a ExecutionError should be raised at runtime: Tag not existed!
Then a ExecutionError should be raised at runtime: TagNotFound: Tag not existed!
# drop if exists with not exist tag
When executing query:
"""
Expand Down Expand Up @@ -300,7 +300,7 @@ Feature: Insert string vid of vertex and edge
"""
DESCRIBE EDGE not_exist
"""
Then a ExecutionError should be raised at runtime: Edge not existed!
Then a ExecutionError should be raised at runtime: EdgeNotFound: Edge not existed!
# create edge with timestamp
When executing query:
"""
Expand Down Expand Up @@ -378,7 +378,7 @@ Feature: Insert string vid of vertex and edge
"""
DROP EDGE not_exist_edge
"""
Then a ExecutionError should be raised at runtime: Edge not existed!
Then a ExecutionError should be raised at runtime: EdgeNotFound: Edge not existed!
# drop if exists
When executing query:
"""
Expand Down
Loading