From 8f7b96cbf0983cf9a8a94c2d255bb7e74df76af7 Mon Sep 17 00:00:00 2001 From: codesigner Date: Fri, 2 Dec 2022 14:26:24 +0800 Subject: [PATCH 01/15] refine AliasType deducing when planning --- src/graph/context/ast/CypherAstContext.h | 2 +- src/graph/planner/match/MatchPlanner.cpp | 8 +- src/graph/planner/match/MatchPlanner.h | 3 +- src/graph/validator/MatchValidator.cpp | 15 +- src/graph/visitor/CMakeLists.txt | 1 + src/graph/visitor/DeduceAliasTypeVisitor.cpp | 91 ++++++++++++ src/graph/visitor/DeduceAliasTypeVisitor.h | 100 +++++++++++++ src/graph/visitor/test/CMakeLists.txt | 1 + .../test/DeduceAliasTypeVisitorTest.cpp | 135 ++++++++++++++++++ .../features/bugfix/AliasTypeDeduce.feature | 17 +++ 10 files changed, 365 insertions(+), 8 deletions(-) create mode 100644 src/graph/visitor/DeduceAliasTypeVisitor.cpp create mode 100644 src/graph/visitor/DeduceAliasTypeVisitor.h create mode 100644 src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp create mode 100644 tests/tck/features/bugfix/AliasTypeDeduce.feature diff --git a/src/graph/context/ast/CypherAstContext.h b/src/graph/context/ast/CypherAstContext.h index eeb0857d5b0..268f6a6ce3e 100644 --- a/src/graph/context/ast/CypherAstContext.h +++ b/src/graph/context/ast/CypherAstContext.h @@ -55,7 +55,7 @@ struct EdgeInfo { Expression* filter{nullptr}; }; -enum class AliasType : int8_t { kNode, kEdge, kPath, kEdgeList, kDefault }; +enum class AliasType : int8_t { kNode, kEdge, kPath, kNodeList, kEdgeList, kRuntime }; struct ScanInfo { Expression* filter{nullptr}; diff --git a/src/graph/planner/match/MatchPlanner.cpp b/src/graph/planner/match/MatchPlanner.cpp index 1e68fa9beaf..176f55ca2ef 100644 --- a/src/graph/planner/match/MatchPlanner.cpp +++ b/src/graph/planner/match/MatchPlanner.cpp @@ -72,8 +72,12 @@ Status MatchPlanner::connectMatchPlan(SubPlan& queryPlan, MatchClauseContext* ma for (auto& alias : matchCtx->aliasesGenerated) { auto it = matchCtx->aliasesAvailable.find(alias.first); if (it != matchCtx->aliasesAvailable.end()) { - // Joined type should be same - if (it->second != alias.second) { + // Joined type should be same, + // If any type is kRuntime, leave the type check to runtime to check the type, + // Primitive types (Integer, String, etc.) or composite types(List, Map etc.) + // are deduced to kRuntime when cannot be deduced during planning. + if (it->second != alias.second && it->second != AliasType::kRuntime && + alias.second != AliasType::kRuntime) { return Status::SemanticError(fmt::format("{} binding to different type: {} vs {}", alias.first, AliasTypeName[static_cast(alias.second)], diff --git a/src/graph/planner/match/MatchPlanner.h b/src/graph/planner/match/MatchPlanner.h index 95c0cc7ad14..ec1c03d0779 100644 --- a/src/graph/planner/match/MatchPlanner.h +++ b/src/graph/planner/match/MatchPlanner.h @@ -23,7 +23,8 @@ class MatchPlanner final : public Planner { StatusOr transform(AstContext* astCtx) override; private: - static constexpr std::array AliasTypeName = {"Node", "Edge", "Path", "EdgeList", "Default"}; + static constexpr std::array AliasTypeName = { + "Node", "Edge", "Path", "NodeList", "EdgeList", "Default"}; bool tailConnected_{false}; StatusOr genPlan(CypherClauseContextBase* clauseCtx); Status connectMatchPlan(SubPlan& queryPlan, MatchClauseContext* matchCtx); diff --git a/src/graph/validator/MatchValidator.cpp b/src/graph/validator/MatchValidator.cpp index aca9729dd8b..cbdd22f0271 100644 --- a/src/graph/validator/MatchValidator.cpp +++ b/src/graph/validator/MatchValidator.cpp @@ -7,6 +7,7 @@ #include "graph/planner/match/MatchSolver.h" #include "graph/util/ExpressionUtils.h" +#include "graph/visitor/DeduceAliasTypeVisitor.h" #include "graph/visitor/ExtractGroupSuiteVisitor.h" #include "graph/visitor/RewriteVisitor.h" #include "graph/visitor/ValidatePatternExpressionVisitor.h" @@ -546,13 +547,19 @@ Status MatchValidator::validateWith(const WithClause *with, exprs.reserve(withClauseCtx.yield->yieldColumns->size()); for (auto *col : withClauseCtx.yield->yieldColumns->columns()) { auto labelExprs = ExpressionUtils::collectAll(col->expr(), {Expression::Kind::kLabel}); - auto aliasType = AliasType::kDefault; + auto aliasType = AliasType::kRuntime; for (auto *labelExpr : labelExprs) { auto label = static_cast(labelExpr)->name(); if (!withClauseCtx.yield->aliasesAvailable.count(label)) { return Status::SemanticError("Alias `%s` not defined", label.c_str()); } - aliasType = withClauseCtx.yield->aliasesAvailable.at(label); + AliasType inputType = withClauseCtx.yield->aliasesAvailable.at(label); + DeduceAliasTypeVisitor visitor(qctx_, vctx_, space_.id, inputType); + const_cast(col->expr())->accept(&visitor); + if (!visitor.ok()) { + return std::move(visitor).status(); + } + aliasType = visitor.outputType(); } if (col->alias().empty()) { if (col->expr()->kind() == Expression::Kind::kLabel) { @@ -632,7 +639,7 @@ Status MatchValidator::validateUnwind(const UnwindClause *unwindClause, // set z to the same type // else // set z to default - AliasType aliasType = AliasType::kDefault; + AliasType aliasType = AliasType::kRuntime; if (types.size() > 0 && std::adjacent_find(types.begin(), types.end(), std::not_equal_to<>()) == types.end()) { aliasType = types[0]; @@ -922,7 +929,7 @@ Status MatchValidator::checkAlias( const Expression *refExpr, const std::unordered_map &aliasesAvailable) const { auto kind = refExpr->kind(); - AliasType aliasType = AliasType::kDefault; + AliasType aliasType = AliasType::kRuntime; switch (kind) { case Expression::Kind::kLabel: { diff --git a/src/graph/visitor/CMakeLists.txt b/src/graph/visitor/CMakeLists.txt index cdd75ac16fc..01ccba3cd1a 100644 --- a/src/graph/visitor/CMakeLists.txt +++ b/src/graph/visitor/CMakeLists.txt @@ -18,6 +18,7 @@ nebula_add_library( EvaluableExprVisitor.cpp ExtractGroupSuiteVisitor.cpp ValidatePatternExpressionVisitor.cpp + DeduceAliasTypeVisitor.cpp ) nebula_add_library( diff --git a/src/graph/visitor/DeduceAliasTypeVisitor.cpp b/src/graph/visitor/DeduceAliasTypeVisitor.cpp new file mode 100644 index 00000000000..5fc5e61252d --- /dev/null +++ b/src/graph/visitor/DeduceAliasTypeVisitor.cpp @@ -0,0 +1,91 @@ +/* Copyright (c) 2022 vesoft inc. All rights reserved. + * + * This source code is licensed under Apache 2.0 License. + */ + +#include "graph/visitor/DeduceAliasTypeVisitor.h" + +#include +#include + +#include "common/datatypes/DataSet.h" +#include "common/datatypes/Edge.h" +#include "common/datatypes/List.h" +#include "common/datatypes/Map.h" +#include "common/datatypes/Path.h" +#include "common/datatypes/Set.h" +#include "common/function/FunctionManager.h" +#include "graph/context/QueryContext.h" +#include "graph/context/QueryExpressionContext.h" +#include "graph/context/ValidateContext.h" +#include "graph/visitor/EvaluableExprVisitor.h" + +namespace nebula { +namespace graph { + +DeduceAliasTypeVisitor::DeduceAliasTypeVisitor(QueryContext *qctx, + ValidateContext *vctx, + GraphSpaceID space, + AliasType inputType) + : qctx_(qctx), vctx_(vctx), space_(space), inputType_(inputType) { + UNUSED(qctx_); + UNUSED(vctx_); + UNUSED(space_); +} + +void DeduceAliasTypeVisitor::visit(VertexExpression *expr) { + UNUSED(expr); + outputType_ = AliasType::kNode; +} + +void DeduceAliasTypeVisitor::visit(EdgeExpression *expr) { + UNUSED(expr); + outputType_ = AliasType::kEdge; +} + +void DeduceAliasTypeVisitor::visit(PathBuildExpression *expr) { + UNUSED(expr); + outputType_ = AliasType::kPath; +} + +void DeduceAliasTypeVisitor::visit(FunctionCallExpression *expr) { + auto funName = expr->name(); + if (funName == "nodes") { + outputType_ = AliasType::kNodeList; + } else if (funName == "relationships") { + outputType_ = AliasType::kEdgeList; + } else if (funName == "reversepath") { + outputType_ = AliasType::kPath; + } else if (funName == "startnode" || funName == "endnode") { + outputType_ = AliasType::kNode; + } +} + +void DeduceAliasTypeVisitor::visit(SubscriptExpression *expr) { + UNUSED(expr); + // This is not accurate, since there exist List of List...Edges/Nodes, + // may have opportunities when analyze more detail of the expr. + if (inputType_ == AliasType::kEdgeList) { + outputType_ = AliasType::kEdge; + } else if (inputType_ == AliasType::kNodeList) { + outputType_ = AliasType::kNode; + } else { + outputType_ = inputType_; + } +} + +void DeduceAliasTypeVisitor::visit(SubscriptRangeExpression *expr) { + UNUSED(expr); + // This is not accurate, since there exist List of List...Edges/Nodes, + // may have opportunities when analyze more detail of the expr. + if (inputType_ == AliasType::kEdgeList) { + outputType_ = AliasType::kEdgeList; + } else if (inputType_ == AliasType::kNodeList) { + outputType_ = AliasType::kNodeList; + } else { + outputType_ = inputType_; + } +} + +} // namespace graph +} // namespace nebula diff --git a/src/graph/visitor/DeduceAliasTypeVisitor.h b/src/graph/visitor/DeduceAliasTypeVisitor.h new file mode 100644 index 00000000000..d20b03e54dc --- /dev/null +++ b/src/graph/visitor/DeduceAliasTypeVisitor.h @@ -0,0 +1,100 @@ +/* Copyright (c) 2022 vesoft inc. All rights reserved. + * + * This source code is licensed under Apache 2.0 License. + */ + +#ifndef NEBULA_GRAPH_DEDUCEALIASTYPEVISITOR_H +#define NEBULA_GRAPH_DEDUCEALIASTYPEVISITOR_H + +#include "common/base/Status.h" +#include "common/datatypes/Value.h" +#include "common/expression/ExprVisitor.h" +#include "graph/context/ValidateContext.h" +#include "graph/context/ast/CypherAstContext.h" + +namespace nebula { +namespace graph { + +class QueryContext; + +// An expression visitor enable deducing AliasType when possible. +class DeduceAliasTypeVisitor final : public ExprVisitor { + public: + DeduceAliasTypeVisitor(QueryContext *qctx, + ValidateContext *vctx, + GraphSpaceID space, + AliasType inputType); + + ~DeduceAliasTypeVisitor() = default; + + bool ok() const { + return status_.ok(); + } + + Status status() && { + return std::move(status_); + } + + AliasType outputType() const { + return outputType_; + } + + private: + // Most expression cannot be used to deducing, + // or deduced to primitive types, use the default kRuntime type + void visit(ConstantExpression *) override {} + void visit(UnaryExpression *) override {} + void visit(TypeCastingExpression *) override {} + void visit(LabelExpression *) override {} + void visit(LabelAttributeExpression *) override {} + void visit(ArithmeticExpression *) override {} + void visit(RelationalExpression *) override {} + void visit(AttributeExpression *) override {} + void visit(LogicalExpression *) override {} + void visit(AggregateExpression *) override {} + void visit(UUIDExpression *) override {} + void visit(VariableExpression *) override {} + void visit(VersionedVariableExpression *) override {} + void visit(ListExpression *) override {} + void visit(SetExpression *) override {} + void visit(MapExpression *) override {} + void visit(LabelTagPropertyExpression *) override {} + void visit(TagPropertyExpression *) override {} + void visit(EdgePropertyExpression *) override {} + void visit(InputPropertyExpression *) override {} + void visit(VariablePropertyExpression *) override {} + void visit(DestPropertyExpression *) override {} + void visit(SourcePropertyExpression *) override {} + void visit(EdgeSrcIdExpression *) override {} + void visit(EdgeTypeExpression *) override {} + void visit(EdgeRankExpression *) override {} + void visit(EdgeDstIdExpression *) override {} + void visit(CaseExpression *) override {} + void visit(ColumnExpression *) override {} + void visit(PredicateExpression *) override {} + void visit(ListComprehensionExpression *) override {} + void visit(ReduceExpression *) override {} + void visit(MatchPathPatternExpression *) override {} + + // Expression may have deducing potential + void visit(VertexExpression *expr) override; + void visit(EdgeExpression *expr) override; + void visit(PathBuildExpression *expr) override; + void visit(FunctionCallExpression *expr) override; + void visit(SubscriptExpression *expr) override; + void visit(SubscriptRangeExpression *expr) override; + + private: + const QueryContext *qctx_{nullptr}; + const ValidateContext *vctx_{nullptr}; + GraphSpaceID space_; + Status status_; + AliasType inputType_{AliasType::kRuntime}; + // Default output type is Runtime + AliasType outputType_{AliasType::kRuntime}; +}; + +} // namespace graph +} // namespace nebula + +#endif // NEBULA_GRAPH_DEDUCEALIASTYPEVISITOR_H diff --git a/src/graph/visitor/test/CMakeLists.txt b/src/graph/visitor/test/CMakeLists.txt index e7fa08ce8d9..d3d3acd68f0 100644 --- a/src/graph/visitor/test/CMakeLists.txt +++ b/src/graph/visitor/test/CMakeLists.txt @@ -20,6 +20,7 @@ nebula_add_test( TestMain.cpp FoldConstantExprVisitorTest.cpp DeduceTypeVisitorTest.cpp + DeduceAliasTypeVisitorTest.cpp RewriteUnaryNotExprVisitorTest.cpp RewriteRelExprVisitorTest.cpp FilterTransformTest.cpp diff --git a/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp b/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp new file mode 100644 index 00000000000..9cd0c1762f6 --- /dev/null +++ b/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp @@ -0,0 +1,135 @@ +/* Copyright (c) 2022 vesoft inc. All rights reserved. + * + * This source code is licensed under Apache 2.0 License. + */ + +#include + +#include "graph/visitor/DeduceAliasTypeVisitor.h" +#include "graph/visitor/test/VisitorTestBase.h" + +namespace nebula { +namespace graph { +class DeduceAliasTypeVisitorTest : public VisitorTestBase { +}; + +TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { + { + auto* expr = VertexExpression::make(pool); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + expr->accept(&visitor); + EXPECT_TRUE(visitor.ok()); + EXPECT_EQ(visitor.outputType(), AliasType::kNode); + } + { + auto* expr = EdgeExpression::make(pool); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + expr->accept(&visitor); + EXPECT_TRUE(visitor.ok()); + EXPECT_EQ(visitor.outputType(), AliasType::kEdge); + } + { + auto* expr = PathBuildExpression::make(pool); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + expr->accept(&visitor); + EXPECT_TRUE(visitor.ok()); + EXPECT_EQ(visitor.outputType(), AliasType::kPath); + } + // FunctionCallExpression + { + auto* expr = FunctionCallExpression::make(pool, "nodes"); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + expr->accept(&visitor); + EXPECT_TRUE(visitor.ok()); + EXPECT_EQ(visitor.outputType(), AliasType::kNodeList); + } + { + auto* expr = FunctionCallExpression::make(pool, "relationships"); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + expr->accept(&visitor); + EXPECT_TRUE(visitor.ok()); + EXPECT_EQ(visitor.outputType(), AliasType::kEdgeList); + } + { + auto* expr = FunctionCallExpression::make(pool, "reversepath"); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + expr->accept(&visitor); + EXPECT_TRUE(visitor.ok()); + EXPECT_EQ(visitor.outputType(), AliasType::kPath); + } + { + auto* expr = FunctionCallExpression::make(pool, "startnode"); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + expr->accept(&visitor); + EXPECT_TRUE(visitor.ok()); + EXPECT_EQ(visitor.outputType(), AliasType::kNode); + } + { + auto* expr = FunctionCallExpression::make(pool, "endnode"); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + expr->accept(&visitor); + EXPECT_TRUE(visitor.ok()); + EXPECT_EQ(visitor.outputType(), AliasType::kNode); + } + + // SubscriptExpression + { + auto* items = ExpressionList::make(pool); + auto expr = SubscriptExpression::make( + pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + expr->accept(&visitor); + EXPECT_TRUE(visitor.ok()); + EXPECT_EQ(visitor.outputType(), AliasType::kRuntime); + } + { + auto* items = ExpressionList::make(pool); + auto expr = SubscriptExpression::make( + pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kEdgeList); + expr->accept(&visitor); + EXPECT_TRUE(visitor.ok()); + EXPECT_EQ(visitor.outputType(), AliasType::kEdge); + } + { + auto* items = ExpressionList::make(pool); + auto expr = SubscriptExpression::make( + pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kNodeList); + expr->accept(&visitor); + EXPECT_TRUE(visitor.ok()); + EXPECT_EQ(visitor.outputType(), AliasType::kNode); + } + + // SubscriptRangeExpression + { + auto* items = ExpressionList::make(pool); + auto expr = SubscriptRangeExpression::make( + pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + expr->accept(&visitor); + EXPECT_TRUE(visitor.ok()); + EXPECT_EQ(visitor.outputType(), AliasType::kRuntime); + } + { + auto* items = ExpressionList::make(pool); + auto expr = SubscriptRangeExpression::make( + pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kEdgeList); + expr->accept(&visitor); + EXPECT_TRUE(visitor.ok()); + EXPECT_EQ(visitor.outputType(), AliasType::kEdgeList); + } + { + auto* items = ExpressionList::make(pool); + auto expr = SubscriptRangeExpression::make( + pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kNodeList); + expr->accept(&visitor); + EXPECT_TRUE(visitor.ok()); + EXPECT_EQ(visitor.outputType(), AliasType::kNodeList); + } +} + +} // namespace graph +} // namespace nebula diff --git a/tests/tck/features/bugfix/AliasTypeDeduce.feature b/tests/tck/features/bugfix/AliasTypeDeduce.feature new file mode 100644 index 00000000000..cbf44de03c5 --- /dev/null +++ b/tests/tck/features/bugfix/AliasTypeDeduce.feature @@ -0,0 +1,17 @@ +# Copyright (c) 2022 vesoft inc. All rights reserved. +# +# This source code is licensed under Apache 2.0 License. +@tag1 +Feature: Test extract filter + + Background: + Given a graph with space named "nba" + + Scenario: Extract filter bug + When executing query: + """ + match p=allShortestPaths((v1)-[:like*1..3]-(v2)) where id(v1)=="Tim Duncan" and id(v2)=="Tony Parker" with nodes(p) as pathNodes WITH [n IN pathNodes | id(n)] AS personIdsInPath, [idx IN range(1, size(pathNodes)-1) | [prev IN [pathNodes[idx-1]] | [curr IN [pathNodes[idx]] | [prev, curr]]]] AS vertList UNWIND vertList AS c WITH c[0][0][0] AS prev , c[0][0][1] AS curr OPTIONAL MATCH (curr)<-[e:like]-(:player)-[:serve]->(:team)-[:teammate]->(prev) RETURN count(e) AS cnt1, prev, curr + """ + Then the result should be, in any order: + | cnt1 | prev | curr | + | 0 | ("Tim Duncan" :player{age: 42, name: "Tim Duncan"} :bachelor{name: "Tim Duncan", speciality: "psychology"}) | ("Tony Parker" :player{age: 36, name: "Tony Parker"}) | From c3ec135dcfd2479b3b0a99f8262799aa9f405a0e Mon Sep 17 00:00:00 2001 From: codesigner Date: Fri, 2 Dec 2022 14:49:45 +0800 Subject: [PATCH 02/15] refine --- src/graph/planner/match/MatchPlanner.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/graph/planner/match/MatchPlanner.cpp b/src/graph/planner/match/MatchPlanner.cpp index 176f55ca2ef..095ea0802d7 100644 --- a/src/graph/planner/match/MatchPlanner.cpp +++ b/src/graph/planner/match/MatchPlanner.cpp @@ -73,7 +73,7 @@ Status MatchPlanner::connectMatchPlan(SubPlan& queryPlan, MatchClauseContext* ma auto it = matchCtx->aliasesAvailable.find(alias.first); if (it != matchCtx->aliasesAvailable.end()) { // Joined type should be same, - // If any type is kRuntime, leave the type check to runtime to check the type, + // If any type is kRuntime, leave the type check to runtime, // Primitive types (Integer, String, etc.) or composite types(List, Map etc.) // are deduced to kRuntime when cannot be deduced during planning. if (it->second != alias.second && it->second != AliasType::kRuntime && From d33f42b55da31a76586c016ab849705c1e046ab0 Mon Sep 17 00:00:00 2001 From: codesigner Date: Fri, 2 Dec 2022 14:54:49 +0800 Subject: [PATCH 03/15] fix lint --- src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp b/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp index 9cd0c1762f6..1b75f4c6512 100644 --- a/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp +++ b/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp @@ -10,8 +10,7 @@ namespace nebula { namespace graph { -class DeduceAliasTypeVisitorTest : public VisitorTestBase { -}; +class DeduceAliasTypeVisitorTest : public VisitorTestBase {}; TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { { From e141654bafaa4418cb5d359d0aca6ca53318a52d Mon Sep 17 00:00:00 2001 From: codesigner Date: Fri, 2 Dec 2022 15:59:53 +0800 Subject: [PATCH 04/15] fix tck --- tests/tck/features/bugfix/AliasTypeDeduce.feature | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/tck/features/bugfix/AliasTypeDeduce.feature b/tests/tck/features/bugfix/AliasTypeDeduce.feature index cbf44de03c5..885d0f881d1 100644 --- a/tests/tck/features/bugfix/AliasTypeDeduce.feature +++ b/tests/tck/features/bugfix/AliasTypeDeduce.feature @@ -1,7 +1,6 @@ # Copyright (c) 2022 vesoft inc. All rights reserved. # # This source code is licensed under Apache 2.0 License. -@tag1 Feature: Test extract filter Background: From 542e8113f1d5cc248d0b2af59c46dfbac891c09e Mon Sep 17 00:00:00 2001 From: codesigner Date: Mon, 5 Dec 2022 10:42:44 +0800 Subject: [PATCH 05/15] address comment --- src/graph/validator/MatchValidator.cpp | 2 +- src/graph/visitor/DeduceAliasTypeVisitor.cpp | 13 ++------- src/graph/visitor/DeduceAliasTypeVisitor.h | 5 +--- .../test/DeduceAliasTypeVisitorTest.cpp | 28 +++++++++---------- 4 files changed, 19 insertions(+), 29 deletions(-) diff --git a/src/graph/validator/MatchValidator.cpp b/src/graph/validator/MatchValidator.cpp index cbdd22f0271..ad059aa9f16 100644 --- a/src/graph/validator/MatchValidator.cpp +++ b/src/graph/validator/MatchValidator.cpp @@ -554,7 +554,7 @@ Status MatchValidator::validateWith(const WithClause *with, return Status::SemanticError("Alias `%s` not defined", label.c_str()); } AliasType inputType = withClauseCtx.yield->aliasesAvailable.at(label); - DeduceAliasTypeVisitor visitor(qctx_, vctx_, space_.id, inputType); + DeduceAliasTypeVisitor visitor(inputType); const_cast(col->expr())->accept(&visitor); if (!visitor.ok()) { return std::move(visitor).status(); diff --git a/src/graph/visitor/DeduceAliasTypeVisitor.cpp b/src/graph/visitor/DeduceAliasTypeVisitor.cpp index 5fc5e61252d..afb517aa649 100644 --- a/src/graph/visitor/DeduceAliasTypeVisitor.cpp +++ b/src/graph/visitor/DeduceAliasTypeVisitor.cpp @@ -23,15 +23,7 @@ namespace nebula { namespace graph { -DeduceAliasTypeVisitor::DeduceAliasTypeVisitor(QueryContext *qctx, - ValidateContext *vctx, - GraphSpaceID space, - AliasType inputType) - : qctx_(qctx), vctx_(vctx), space_(space), inputType_(inputType) { - UNUSED(qctx_); - UNUSED(vctx_); - UNUSED(space_); -} +DeduceAliasTypeVisitor::DeduceAliasTypeVisitor(AliasType inputType) : inputType_(inputType) {} void DeduceAliasTypeVisitor::visit(VertexExpression *expr) { UNUSED(expr); @@ -49,7 +41,8 @@ void DeduceAliasTypeVisitor::visit(PathBuildExpression *expr) { } void DeduceAliasTypeVisitor::visit(FunctionCallExpression *expr) { - auto funName = expr->name(); + std::string funName = expr->name(); + std::transform(funName.begin(), funName.end(), funName.begin(), ::tolower); if (funName == "nodes") { outputType_ = AliasType::kNodeList; } else if (funName == "relationships") { diff --git a/src/graph/visitor/DeduceAliasTypeVisitor.h b/src/graph/visitor/DeduceAliasTypeVisitor.h index d20b03e54dc..28d6157075b 100644 --- a/src/graph/visitor/DeduceAliasTypeVisitor.h +++ b/src/graph/visitor/DeduceAliasTypeVisitor.h @@ -20,10 +20,7 @@ class QueryContext; // An expression visitor enable deducing AliasType when possible. class DeduceAliasTypeVisitor final : public ExprVisitor { public: - DeduceAliasTypeVisitor(QueryContext *qctx, - ValidateContext *vctx, - GraphSpaceID space, - AliasType inputType); + explicit DeduceAliasTypeVisitor(AliasType inputType); ~DeduceAliasTypeVisitor() = default; diff --git a/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp b/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp index 1b75f4c6512..3101f82ec89 100644 --- a/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp +++ b/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp @@ -15,21 +15,21 @@ class DeduceAliasTypeVisitorTest : public VisitorTestBase {}; TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { { auto* expr = VertexExpression::make(pool); - DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kNode); } { auto* expr = EdgeExpression::make(pool); - DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kEdge); } { auto* expr = PathBuildExpression::make(pool); - DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kPath); @@ -37,35 +37,35 @@ TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { // FunctionCallExpression { auto* expr = FunctionCallExpression::make(pool, "nodes"); - DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kNodeList); } { auto* expr = FunctionCallExpression::make(pool, "relationships"); - DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kEdgeList); } { auto* expr = FunctionCallExpression::make(pool, "reversepath"); - DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kPath); } { auto* expr = FunctionCallExpression::make(pool, "startnode"); - DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kNode); } { auto* expr = FunctionCallExpression::make(pool, "endnode"); - DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kNode); @@ -76,7 +76,7 @@ TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { auto* items = ExpressionList::make(pool); auto expr = SubscriptExpression::make( pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); - DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kRuntime); @@ -85,7 +85,7 @@ TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { auto* items = ExpressionList::make(pool); auto expr = SubscriptExpression::make( pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); - DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kEdgeList); + DeduceAliasTypeVisitor visitor(AliasType::kEdgeList); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kEdge); @@ -94,7 +94,7 @@ TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { auto* items = ExpressionList::make(pool); auto expr = SubscriptExpression::make( pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); - DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kNodeList); + DeduceAliasTypeVisitor visitor(AliasType::kNodeList); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kNode); @@ -105,7 +105,7 @@ TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { auto* items = ExpressionList::make(pool); auto expr = SubscriptRangeExpression::make( pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); - DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kRuntime); @@ -114,7 +114,7 @@ TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { auto* items = ExpressionList::make(pool); auto expr = SubscriptRangeExpression::make( pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); - DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kEdgeList); + DeduceAliasTypeVisitor visitor(AliasType::kEdgeList); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kEdgeList); @@ -123,7 +123,7 @@ TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { auto* items = ExpressionList::make(pool); auto expr = SubscriptRangeExpression::make( pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); - DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kNodeList); + DeduceAliasTypeVisitor visitor(AliasType::kNodeList); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kNodeList); From a20cd1272ca9c4a37d0b6697ccbcf2742b94be05 Mon Sep 17 00:00:00 2001 From: codesigner Date: Mon, 5 Dec 2022 11:01:43 +0800 Subject: [PATCH 06/15] refine --- src/graph/validator/MatchValidator.cpp | 2 +- src/graph/visitor/DeduceAliasTypeVisitor.cpp | 10 ++++++- src/graph/visitor/DeduceAliasTypeVisitor.h | 5 +++- .../test/DeduceAliasTypeVisitorTest.cpp | 28 +++++++++---------- 4 files changed, 28 insertions(+), 17 deletions(-) diff --git a/src/graph/validator/MatchValidator.cpp b/src/graph/validator/MatchValidator.cpp index ad059aa9f16..cbdd22f0271 100644 --- a/src/graph/validator/MatchValidator.cpp +++ b/src/graph/validator/MatchValidator.cpp @@ -554,7 +554,7 @@ Status MatchValidator::validateWith(const WithClause *with, return Status::SemanticError("Alias `%s` not defined", label.c_str()); } AliasType inputType = withClauseCtx.yield->aliasesAvailable.at(label); - DeduceAliasTypeVisitor visitor(inputType); + DeduceAliasTypeVisitor visitor(qctx_, vctx_, space_.id, inputType); const_cast(col->expr())->accept(&visitor); if (!visitor.ok()) { return std::move(visitor).status(); diff --git a/src/graph/visitor/DeduceAliasTypeVisitor.cpp b/src/graph/visitor/DeduceAliasTypeVisitor.cpp index afb517aa649..0de60d7ef8f 100644 --- a/src/graph/visitor/DeduceAliasTypeVisitor.cpp +++ b/src/graph/visitor/DeduceAliasTypeVisitor.cpp @@ -23,7 +23,15 @@ namespace nebula { namespace graph { -DeduceAliasTypeVisitor::DeduceAliasTypeVisitor(AliasType inputType) : inputType_(inputType) {} +DeduceAliasTypeVisitor::DeduceAliasTypeVisitor(QueryContext *qctx, + ValidateContext *vctx, + GraphSpaceID space, + AliasType inputType) + : qctx_(qctx), vctx_(vctx), space_(space), inputType_(inputType) { + UNUSED(qctx_); + UNUSED(vctx_); + UNUSED(space_); +} void DeduceAliasTypeVisitor::visit(VertexExpression *expr) { UNUSED(expr); diff --git a/src/graph/visitor/DeduceAliasTypeVisitor.h b/src/graph/visitor/DeduceAliasTypeVisitor.h index 28d6157075b..d20b03e54dc 100644 --- a/src/graph/visitor/DeduceAliasTypeVisitor.h +++ b/src/graph/visitor/DeduceAliasTypeVisitor.h @@ -20,7 +20,10 @@ class QueryContext; // An expression visitor enable deducing AliasType when possible. class DeduceAliasTypeVisitor final : public ExprVisitor { public: - explicit DeduceAliasTypeVisitor(AliasType inputType); + DeduceAliasTypeVisitor(QueryContext *qctx, + ValidateContext *vctx, + GraphSpaceID space, + AliasType inputType); ~DeduceAliasTypeVisitor() = default; diff --git a/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp b/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp index 3101f82ec89..1b75f4c6512 100644 --- a/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp +++ b/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp @@ -15,21 +15,21 @@ class DeduceAliasTypeVisitorTest : public VisitorTestBase {}; TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { { auto* expr = VertexExpression::make(pool); - DeduceAliasTypeVisitor visitor(AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kNode); } { auto* expr = EdgeExpression::make(pool); - DeduceAliasTypeVisitor visitor(AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kEdge); } { auto* expr = PathBuildExpression::make(pool); - DeduceAliasTypeVisitor visitor(AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kPath); @@ -37,35 +37,35 @@ TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { // FunctionCallExpression { auto* expr = FunctionCallExpression::make(pool, "nodes"); - DeduceAliasTypeVisitor visitor(AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kNodeList); } { auto* expr = FunctionCallExpression::make(pool, "relationships"); - DeduceAliasTypeVisitor visitor(AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kEdgeList); } { auto* expr = FunctionCallExpression::make(pool, "reversepath"); - DeduceAliasTypeVisitor visitor(AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kPath); } { auto* expr = FunctionCallExpression::make(pool, "startnode"); - DeduceAliasTypeVisitor visitor(AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kNode); } { auto* expr = FunctionCallExpression::make(pool, "endnode"); - DeduceAliasTypeVisitor visitor(AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kNode); @@ -76,7 +76,7 @@ TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { auto* items = ExpressionList::make(pool); auto expr = SubscriptExpression::make( pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); - DeduceAliasTypeVisitor visitor(AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kRuntime); @@ -85,7 +85,7 @@ TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { auto* items = ExpressionList::make(pool); auto expr = SubscriptExpression::make( pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); - DeduceAliasTypeVisitor visitor(AliasType::kEdgeList); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kEdgeList); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kEdge); @@ -94,7 +94,7 @@ TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { auto* items = ExpressionList::make(pool); auto expr = SubscriptExpression::make( pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); - DeduceAliasTypeVisitor visitor(AliasType::kNodeList); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kNodeList); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kNode); @@ -105,7 +105,7 @@ TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { auto* items = ExpressionList::make(pool); auto expr = SubscriptRangeExpression::make( pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); - DeduceAliasTypeVisitor visitor(AliasType::kRuntime); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kRuntime); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kRuntime); @@ -114,7 +114,7 @@ TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { auto* items = ExpressionList::make(pool); auto expr = SubscriptRangeExpression::make( pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); - DeduceAliasTypeVisitor visitor(AliasType::kEdgeList); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kEdgeList); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kEdgeList); @@ -123,7 +123,7 @@ TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { auto* items = ExpressionList::make(pool); auto expr = SubscriptRangeExpression::make( pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); - DeduceAliasTypeVisitor visitor(AliasType::kNodeList); + DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kNodeList); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kNodeList); From 09bb8397fa6d61d0ae854def7f3327ea527255fb Mon Sep 17 00:00:00 2001 From: codesigner Date: Tue, 6 Dec 2022 18:00:31 +0800 Subject: [PATCH 07/15] refine code & add tck case --- src/graph/context/ast/CypherAstContext.h | 3 +++ src/graph/planner/match/MatchPlanner.h | 2 -- src/graph/validator/MatchValidator.cpp | 12 +++++++++--- src/graph/visitor/DeduceAliasTypeVisitor.cpp | 15 +++++++++------ src/graph/visitor/DeduceAliasTypeVisitor.h | 4 ++-- tests/tck/features/bugfix/AliasTypeDeduce.feature | 7 +++++++ 6 files changed, 30 insertions(+), 13 deletions(-) diff --git a/src/graph/context/ast/CypherAstContext.h b/src/graph/context/ast/CypherAstContext.h index 268f6a6ce3e..d4b647bb183 100644 --- a/src/graph/context/ast/CypherAstContext.h +++ b/src/graph/context/ast/CypherAstContext.h @@ -57,6 +57,9 @@ struct EdgeInfo { enum class AliasType : int8_t { kNode, kEdge, kPath, kNodeList, kEdgeList, kRuntime }; +static constexpr std::array AliasTypeName = { + "Node", "Edge", "Path", "NodeList", "EdgeList", "kRuntime"}; + struct ScanInfo { Expression* filter{nullptr}; std::vector schemaIds; diff --git a/src/graph/planner/match/MatchPlanner.h b/src/graph/planner/match/MatchPlanner.h index ec1c03d0779..d61f2932a08 100644 --- a/src/graph/planner/match/MatchPlanner.h +++ b/src/graph/planner/match/MatchPlanner.h @@ -23,8 +23,6 @@ class MatchPlanner final : public Planner { StatusOr transform(AstContext* astCtx) override; private: - static constexpr std::array AliasTypeName = { - "Node", "Edge", "Path", "NodeList", "EdgeList", "Default"}; bool tailConnected_{false}; StatusOr genPlan(CypherClauseContextBase* clauseCtx); Status connectMatchPlan(SubPlan& queryPlan, MatchClauseContext* matchCtx); diff --git a/src/graph/validator/MatchValidator.cpp b/src/graph/validator/MatchValidator.cpp index cbdd22f0271..c3629a305b2 100644 --- a/src/graph/validator/MatchValidator.cpp +++ b/src/graph/validator/MatchValidator.cpp @@ -1100,7 +1100,9 @@ Status MatchValidator::validateMatchPathExpr( matchPath.alias()->c_str()); } if (find->second != AliasType::kPath) { - return Status::SemanticError("Alias `%s' should be Path.", matchPath.alias()->c_str()); + return Status::SemanticError("Alias `%s' should be Path, but got type '%s", + matchPath.alias()->c_str(), + AliasTypeName[static_cast(find->second)]); } } for (const auto &node : matchPath.nodes()) { @@ -1116,7 +1118,9 @@ Status MatchValidator::validateMatchPathExpr( node->alias().c_str()); } if (find->second != AliasType::kNode) { - return Status::SemanticError("Alias `%s' should be Node.", node->alias().c_str()); + return Status::SemanticError("Alias `%s' should be Node, but got type '%s", + node->alias().c_str(), + AliasTypeName[static_cast(find->second)]); } } } @@ -1129,7 +1133,9 @@ Status MatchValidator::validateMatchPathExpr( edge->alias().c_str()); } if (find->second != AliasType::kEdge) { - return Status::SemanticError("Alias `%s' should be Edge.", edge->alias().c_str()); + return Status::SemanticError("Alias `%s' should be Edge, but got type '%s'", + edge->alias().c_str(), + AliasTypeName[static_cast(find->second)]); } } } diff --git a/src/graph/visitor/DeduceAliasTypeVisitor.cpp b/src/graph/visitor/DeduceAliasTypeVisitor.cpp index 0de60d7ef8f..d3fecc6edd5 100644 --- a/src/graph/visitor/DeduceAliasTypeVisitor.cpp +++ b/src/graph/visitor/DeduceAliasTypeVisitor.cpp @@ -27,11 +27,7 @@ DeduceAliasTypeVisitor::DeduceAliasTypeVisitor(QueryContext *qctx, ValidateContext *vctx, GraphSpaceID space, AliasType inputType) - : qctx_(qctx), vctx_(vctx), space_(space), inputType_(inputType) { - UNUSED(qctx_); - UNUSED(vctx_); - UNUSED(space_); -} + : qctx_(qctx), vctx_(vctx), space_(space), inputType_(inputType) {} void DeduceAliasTypeVisitor::visit(VertexExpression *expr) { UNUSED(expr); @@ -63,7 +59,14 @@ void DeduceAliasTypeVisitor::visit(FunctionCallExpression *expr) { } void DeduceAliasTypeVisitor::visit(SubscriptExpression *expr) { - UNUSED(expr); + Expression *leftExpr = expr->left(); + DeduceAliasTypeVisitor childVisitor(qctx_, vctx_, space_, inputType_); + leftExpr->accept(&childVisitor); + if (!childVisitor.ok()) { + status_ = std::move(childVisitor).status(); + return; + } + inputType_ = childVisitor.outputType(); // This is not accurate, since there exist List of List...Edges/Nodes, // may have opportunities when analyze more detail of the expr. if (inputType_ == AliasType::kEdgeList) { diff --git a/src/graph/visitor/DeduceAliasTypeVisitor.h b/src/graph/visitor/DeduceAliasTypeVisitor.h index d20b03e54dc..1b132f6e84e 100644 --- a/src/graph/visitor/DeduceAliasTypeVisitor.h +++ b/src/graph/visitor/DeduceAliasTypeVisitor.h @@ -85,8 +85,8 @@ class DeduceAliasTypeVisitor final : public ExprVisitor { void visit(SubscriptRangeExpression *expr) override; private: - const QueryContext *qctx_{nullptr}; - const ValidateContext *vctx_{nullptr}; + QueryContext *qctx_{nullptr}; + ValidateContext *vctx_{nullptr}; GraphSpaceID space_; Status status_; AliasType inputType_{AliasType::kRuntime}; diff --git a/tests/tck/features/bugfix/AliasTypeDeduce.feature b/tests/tck/features/bugfix/AliasTypeDeduce.feature index 885d0f881d1..f67db225a1a 100644 --- a/tests/tck/features/bugfix/AliasTypeDeduce.feature +++ b/tests/tck/features/bugfix/AliasTypeDeduce.feature @@ -14,3 +14,10 @@ Feature: Test extract filter Then the result should be, in any order: | cnt1 | prev | curr | | 0 | ("Tim Duncan" :player{age: 42, name: "Tim Duncan"} :bachelor{name: "Tim Duncan", speciality: "psychology"}) | ("Tony Parker" :player{age: 36, name: "Tony Parker"}) | + When executing query: + """ + match p=(a:player)-[e:like*1..3]->(b) where b.player.age>42 with relationships(p)[1] AS e1 match (b)-[:serve]->(c) where c.team.name>"S" and (b)-[e1]->() return count(c) + """ + Then the result should be, in any order: + | count(c) | + | 2250 | From d2cb0711f3832466676ee7b4ccd4cd222048157e Mon Sep 17 00:00:00 2001 From: codesigner Date: Tue, 6 Dec 2022 18:46:53 +0800 Subject: [PATCH 08/15] refine --- src/graph/visitor/DeduceAliasTypeVisitor.cpp | 9 ++++++++- tests/tck/features/bugfix/AliasTypeDeduce.feature | 7 +++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/graph/visitor/DeduceAliasTypeVisitor.cpp b/src/graph/visitor/DeduceAliasTypeVisitor.cpp index d3fecc6edd5..571eabf3e71 100644 --- a/src/graph/visitor/DeduceAliasTypeVisitor.cpp +++ b/src/graph/visitor/DeduceAliasTypeVisitor.cpp @@ -79,7 +79,14 @@ void DeduceAliasTypeVisitor::visit(SubscriptExpression *expr) { } void DeduceAliasTypeVisitor::visit(SubscriptRangeExpression *expr) { - UNUSED(expr); + Expression *leftExpr = expr->list(); + DeduceAliasTypeVisitor childVisitor(qctx_, vctx_, space_, inputType_); + leftExpr->accept(&childVisitor); + if (!childVisitor.ok()) { + status_ = std::move(childVisitor).status(); + return; + } + inputType_ = childVisitor.outputType(); // This is not accurate, since there exist List of List...Edges/Nodes, // may have opportunities when analyze more detail of the expr. if (inputType_ == AliasType::kEdgeList) { diff --git a/tests/tck/features/bugfix/AliasTypeDeduce.feature b/tests/tck/features/bugfix/AliasTypeDeduce.feature index f67db225a1a..7fe2257987b 100644 --- a/tests/tck/features/bugfix/AliasTypeDeduce.feature +++ b/tests/tck/features/bugfix/AliasTypeDeduce.feature @@ -21,3 +21,10 @@ Feature: Test extract filter Then the result should be, in any order: | count(c) | | 2250 | + When executing query: + """ + match p=(a:player)-[e:like*1..3]->(b) where b.player.age>42 with relationships(p)[1..2][0] AS e1 match (b)-[:serve]->(c) where c.team.name>"S" and (b)-[e1]->() return count(c) + """ + Then the result should be, in any order: + | count(c) | + | 2250 | From 1a19a722265964cbc02fe600d7306efc4536df6c Mon Sep 17 00:00:00 2001 From: codesigner Date: Wed, 7 Dec 2022 09:57:48 +0800 Subject: [PATCH 09/15] fix compile --- src/graph/context/ast/CypherAstContext.h | 18 ++++++++++++++++-- src/graph/planner/match/MatchPlanner.cpp | 4 ++-- src/graph/validator/MatchValidator.cpp | 6 +++--- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/graph/context/ast/CypherAstContext.h b/src/graph/context/ast/CypherAstContext.h index d4b647bb183..91e0e7dc2df 100644 --- a/src/graph/context/ast/CypherAstContext.h +++ b/src/graph/context/ast/CypherAstContext.h @@ -57,8 +57,22 @@ struct EdgeInfo { enum class AliasType : int8_t { kNode, kEdge, kPath, kNodeList, kEdgeList, kRuntime }; -static constexpr std::array AliasTypeName = { - "Node", "Edge", "Path", "NodeList", "EdgeList", "kRuntime"}; +std::string AliasTypeName(AliasType type) { + switch (type) { + case AliasType::kNode: + return "Node"; + case AliasType::kEdge: + return "Edge"; + case AliasType::kPath: + return "Path"; + case AliasType::kNodeList: + return "NodeList"; + case AliasType::kEdgeList: + return "EdgeList"; + case AliasType::kRuntime: + return "Runtime"; + } +} struct ScanInfo { Expression* filter{nullptr}; diff --git a/src/graph/planner/match/MatchPlanner.cpp b/src/graph/planner/match/MatchPlanner.cpp index 0748be0d4a7..2f0d346d245 100644 --- a/src/graph/planner/match/MatchPlanner.cpp +++ b/src/graph/planner/match/MatchPlanner.cpp @@ -80,8 +80,8 @@ Status MatchPlanner::connectMatchPlan(SubPlan& queryPlan, MatchClauseContext* ma alias.second != AliasType::kRuntime) { return Status::SemanticError(fmt::format("{} binding to different type: {} vs {}", alias.first, - AliasTypeName[static_cast(alias.second)], - AliasTypeName[static_cast(it->second)])); + AliasTypeName(alias.second), + AliasTypeName(it->second))); } // Joined On EdgeList is not supported if (alias.second == AliasType::kEdgeList) { diff --git a/src/graph/validator/MatchValidator.cpp b/src/graph/validator/MatchValidator.cpp index c3629a305b2..1992c569688 100644 --- a/src/graph/validator/MatchValidator.cpp +++ b/src/graph/validator/MatchValidator.cpp @@ -1102,7 +1102,7 @@ Status MatchValidator::validateMatchPathExpr( if (find->second != AliasType::kPath) { return Status::SemanticError("Alias `%s' should be Path, but got type '%s", matchPath.alias()->c_str(), - AliasTypeName[static_cast(find->second)]); + AliasTypeName(find->second).data()); } } for (const auto &node : matchPath.nodes()) { @@ -1120,7 +1120,7 @@ Status MatchValidator::validateMatchPathExpr( if (find->second != AliasType::kNode) { return Status::SemanticError("Alias `%s' should be Node, but got type '%s", node->alias().c_str(), - AliasTypeName[static_cast(find->second)]); + AliasTypeName(find->second).data()); } } } @@ -1135,7 +1135,7 @@ Status MatchValidator::validateMatchPathExpr( if (find->second != AliasType::kEdge) { return Status::SemanticError("Alias `%s' should be Edge, but got type '%s'", edge->alias().c_str(), - AliasTypeName[static_cast(find->second)]); + AliasTypeName(find->second).data()); } } } From d0ae131a956f533e3aba5d2ee2b0ae15454de800 Mon Sep 17 00:00:00 2001 From: codesigner Date: Wed, 7 Dec 2022 11:10:55 +0800 Subject: [PATCH 10/15] fix compile --- src/graph/context/ast/CypherAstContext.h | 17 +---------------- src/graph/planner/match/MatchPlanner.cpp | 4 ++-- src/graph/validator/MatchValidator.cpp | 6 +++--- 3 files changed, 6 insertions(+), 21 deletions(-) diff --git a/src/graph/context/ast/CypherAstContext.h b/src/graph/context/ast/CypherAstContext.h index 91e0e7dc2df..3284438ac52 100644 --- a/src/graph/context/ast/CypherAstContext.h +++ b/src/graph/context/ast/CypherAstContext.h @@ -57,22 +57,7 @@ struct EdgeInfo { enum class AliasType : int8_t { kNode, kEdge, kPath, kNodeList, kEdgeList, kRuntime }; -std::string AliasTypeName(AliasType type) { - switch (type) { - case AliasType::kNode: - return "Node"; - case AliasType::kEdge: - return "Edge"; - case AliasType::kPath: - return "Path"; - case AliasType::kNodeList: - return "NodeList"; - case AliasType::kEdgeList: - return "EdgeList"; - case AliasType::kRuntime: - return "Runtime"; - } -} +constexpr std::array AliasTypeName = {"Node", "Edge", "Path", "NodeList", "EdgeList", "Runtime"}; struct ScanInfo { Expression* filter{nullptr}; diff --git a/src/graph/planner/match/MatchPlanner.cpp b/src/graph/planner/match/MatchPlanner.cpp index 2f0d346d245..0748be0d4a7 100644 --- a/src/graph/planner/match/MatchPlanner.cpp +++ b/src/graph/planner/match/MatchPlanner.cpp @@ -80,8 +80,8 @@ Status MatchPlanner::connectMatchPlan(SubPlan& queryPlan, MatchClauseContext* ma alias.second != AliasType::kRuntime) { return Status::SemanticError(fmt::format("{} binding to different type: {} vs {}", alias.first, - AliasTypeName(alias.second), - AliasTypeName(it->second))); + AliasTypeName[static_cast(alias.second)], + AliasTypeName[static_cast(it->second)])); } // Joined On EdgeList is not supported if (alias.second == AliasType::kEdgeList) { diff --git a/src/graph/validator/MatchValidator.cpp b/src/graph/validator/MatchValidator.cpp index 1992c569688..c3629a305b2 100644 --- a/src/graph/validator/MatchValidator.cpp +++ b/src/graph/validator/MatchValidator.cpp @@ -1102,7 +1102,7 @@ Status MatchValidator::validateMatchPathExpr( if (find->second != AliasType::kPath) { return Status::SemanticError("Alias `%s' should be Path, but got type '%s", matchPath.alias()->c_str(), - AliasTypeName(find->second).data()); + AliasTypeName[static_cast(find->second)]); } } for (const auto &node : matchPath.nodes()) { @@ -1120,7 +1120,7 @@ Status MatchValidator::validateMatchPathExpr( if (find->second != AliasType::kNode) { return Status::SemanticError("Alias `%s' should be Node, but got type '%s", node->alias().c_str(), - AliasTypeName(find->second).data()); + AliasTypeName[static_cast(find->second)]); } } } @@ -1135,7 +1135,7 @@ Status MatchValidator::validateMatchPathExpr( if (find->second != AliasType::kEdge) { return Status::SemanticError("Alias `%s' should be Edge, but got type '%s'", edge->alias().c_str(), - AliasTypeName(find->second).data()); + AliasTypeName[static_cast(find->second)]); } } } From 3142f3add75055e1bb88442d87dbadc34645b508 Mon Sep 17 00:00:00 2001 From: codesigner Date: Wed, 7 Dec 2022 11:54:35 +0800 Subject: [PATCH 11/15] fix compile --- src/graph/context/ast/CypherAstContext.h | 19 ++++++++++++++++++- src/graph/planner/match/MatchPlanner.cpp | 4 ++-- src/graph/validator/MatchValidator.cpp | 6 +++--- 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/graph/context/ast/CypherAstContext.h b/src/graph/context/ast/CypherAstContext.h index 3284438ac52..d5fa6e066d5 100644 --- a/src/graph/context/ast/CypherAstContext.h +++ b/src/graph/context/ast/CypherAstContext.h @@ -57,7 +57,24 @@ struct EdgeInfo { enum class AliasType : int8_t { kNode, kEdge, kPath, kNodeList, kEdgeList, kRuntime }; -constexpr std::array AliasTypeName = {"Node", "Edge", "Path", "NodeList", "EdgeList", "Runtime"}; +struct AliasTypeName { + static std::string get(AliasType type) { + switch (type) { + case AliasType::kNode: + return "Node"; + case AliasType::kEdge: + return "Edge"; + case AliasType::kPath: + return "Path"; + case AliasType::kNodeList: + return "NodeList"; + case AliasType::kEdgeList: + return "EdgeList"; + case AliasType::kRuntime: + return "Runtime"; + } + } +}; struct ScanInfo { Expression* filter{nullptr}; diff --git a/src/graph/planner/match/MatchPlanner.cpp b/src/graph/planner/match/MatchPlanner.cpp index 0748be0d4a7..72f4cf31581 100644 --- a/src/graph/planner/match/MatchPlanner.cpp +++ b/src/graph/planner/match/MatchPlanner.cpp @@ -80,8 +80,8 @@ Status MatchPlanner::connectMatchPlan(SubPlan& queryPlan, MatchClauseContext* ma alias.second != AliasType::kRuntime) { return Status::SemanticError(fmt::format("{} binding to different type: {} vs {}", alias.first, - AliasTypeName[static_cast(alias.second)], - AliasTypeName[static_cast(it->second)])); + AliasTypeName::get(alias.second), + AliasTypeName::get(it->second))); } // Joined On EdgeList is not supported if (alias.second == AliasType::kEdgeList) { diff --git a/src/graph/validator/MatchValidator.cpp b/src/graph/validator/MatchValidator.cpp index c3629a305b2..b16da90855e 100644 --- a/src/graph/validator/MatchValidator.cpp +++ b/src/graph/validator/MatchValidator.cpp @@ -1102,7 +1102,7 @@ Status MatchValidator::validateMatchPathExpr( if (find->second != AliasType::kPath) { return Status::SemanticError("Alias `%s' should be Path, but got type '%s", matchPath.alias()->c_str(), - AliasTypeName[static_cast(find->second)]); + AliasTypeName::get(find->second).c_str()); } } for (const auto &node : matchPath.nodes()) { @@ -1120,7 +1120,7 @@ Status MatchValidator::validateMatchPathExpr( if (find->second != AliasType::kNode) { return Status::SemanticError("Alias `%s' should be Node, but got type '%s", node->alias().c_str(), - AliasTypeName[static_cast(find->second)]); + AliasTypeName::get(find->second).c_str()); } } } @@ -1135,7 +1135,7 @@ Status MatchValidator::validateMatchPathExpr( if (find->second != AliasType::kEdge) { return Status::SemanticError("Alias `%s' should be Edge, but got type '%s'", edge->alias().c_str(), - AliasTypeName[static_cast(find->second)]); + AliasTypeName::get(find->second).c_str()); } } } From cec92f33d562cd62bd5da9982cd4db0b3b141f4f Mon Sep 17 00:00:00 2001 From: codesigner Date: Wed, 7 Dec 2022 12:27:20 +0800 Subject: [PATCH 12/15] fix compile --- src/graph/context/ast/CypherAstContext.h | 1 + 1 file changed, 1 insertion(+) diff --git a/src/graph/context/ast/CypherAstContext.h b/src/graph/context/ast/CypherAstContext.h index d5fa6e066d5..af77ec1723a 100644 --- a/src/graph/context/ast/CypherAstContext.h +++ b/src/graph/context/ast/CypherAstContext.h @@ -73,6 +73,7 @@ struct AliasTypeName { case AliasType::kRuntime: return "Runtime"; } + return "Error"; // should not reach here } }; From 8e3e92c1a892a26b44b4140c9950288dc5b7e41e Mon Sep 17 00:00:00 2001 From: codesigner Date: Wed, 7 Dec 2022 12:46:10 +0800 Subject: [PATCH 13/15] fix compile --- .../test/DeduceAliasTypeVisitorTest.cpp | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp b/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp index 1b75f4c6512..eeee87f39d5 100644 --- a/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp +++ b/src/graph/visitor/test/DeduceAliasTypeVisitorTest.cpp @@ -82,18 +82,17 @@ TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { EXPECT_EQ(visitor.outputType(), AliasType::kRuntime); } { - auto* items = ExpressionList::make(pool); - auto expr = SubscriptExpression::make( - pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); + auto expr = SubscriptExpression::make(pool, + FunctionCallExpression::make(pool, "relationships"), + ConstantExpression::make(pool, 1)); DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kEdgeList); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kEdge); } { - auto* items = ExpressionList::make(pool); auto expr = SubscriptExpression::make( - pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); + pool, FunctionCallExpression::make(pool, "nodes"), ConstantExpression::make(pool, 1)); DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kNodeList); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); @@ -111,18 +110,17 @@ TEST_F(DeduceAliasTypeVisitorTest, SubscriptExpr) { EXPECT_EQ(visitor.outputType(), AliasType::kRuntime); } { - auto* items = ExpressionList::make(pool); - auto expr = SubscriptRangeExpression::make( - pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); + auto expr = SubscriptRangeExpression::make(pool, + FunctionCallExpression::make(pool, "relationships"), + ConstantExpression::make(pool, 1)); DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kEdgeList); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); EXPECT_EQ(visitor.outputType(), AliasType::kEdgeList); } { - auto* items = ExpressionList::make(pool); auto expr = SubscriptRangeExpression::make( - pool, ListExpression::make(pool, items), ConstantExpression::make(pool, 1)); + pool, FunctionCallExpression::make(pool, "nodes"), ConstantExpression::make(pool, 1)); DeduceAliasTypeVisitor visitor(nullptr, nullptr, 0, AliasType::kNodeList); expr->accept(&visitor); EXPECT_TRUE(visitor.ok()); From 4d1353bc46c72e2f08826f56c63f6a25066ecebd Mon Sep 17 00:00:00 2001 From: codesigner Date: Wed, 7 Dec 2022 14:05:44 +0800 Subject: [PATCH 14/15] fix tck --- tests/tck/features/bugfix/AliasTypeDeduce.feature | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/tck/features/bugfix/AliasTypeDeduce.feature b/tests/tck/features/bugfix/AliasTypeDeduce.feature index 7fe2257987b..60a1652ff73 100644 --- a/tests/tck/features/bugfix/AliasTypeDeduce.feature +++ b/tests/tck/features/bugfix/AliasTypeDeduce.feature @@ -20,11 +20,11 @@ Feature: Test extract filter """ Then the result should be, in any order: | count(c) | - | 2250 | + | 3225 | When executing query: """ match p=(a:player)-[e:like*1..3]->(b) where b.player.age>42 with relationships(p)[1..2][0] AS e1 match (b)-[:serve]->(c) where c.team.name>"S" and (b)-[e1]->() return count(c) """ Then the result should be, in any order: | count(c) | - | 2250 | + | 3225 | From 9f2e98b0a9a2b55666794eef7bfcedbe49d354e9 Mon Sep 17 00:00:00 2001 From: codesigner Date: Wed, 7 Dec 2022 14:55:09 +0800 Subject: [PATCH 15/15] fix tck --- tests/tck/features/match/PathExpr.feature | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/tck/features/match/PathExpr.feature b/tests/tck/features/match/PathExpr.feature index 41286a192db..d4366d179c6 100644 --- a/tests/tck/features/match/PathExpr.feature +++ b/tests/tck/features/match/PathExpr.feature @@ -50,22 +50,22 @@ Feature: Basic match """ MATCH (v:player) WITH (v)-[v]-() AS p RETURN p """ - Then a SemanticError should be raised at runtime: Alias `v' should be Edge. + Then a SemanticError should be raised at runtime: Alias `v' should be Edge, but got type 'Node' When executing query: """ MATCH (v:player) UNWIND (v)-[v]-() AS p RETURN p """ - Then a SemanticError should be raised at runtime: Alias `v' should be Edge. + Then a SemanticError should be raised at runtime: Alias `v' should be Edge, but got type 'Node' When executing query: """ MATCH (v:player) WHERE (v)-[v]-() RETURN v """ - Then a SemanticError should be raised at runtime: Alias `v' should be Edge. + Then a SemanticError should be raised at runtime: Alias `v' should be Edge, but got type 'Node' When executing query: """ MATCH (v:player) RETURN (v)-[v]-() """ - Then a SemanticError should be raised at runtime: Alias `v' should be Edge. + Then a SemanticError should be raised at runtime: Alias `v' should be Edge, but got type 'Node' Scenario: In Where When executing query: