From d06f80e3f3d2b283f9404af3ba369d66b5e757cf Mon Sep 17 00:00:00 2001 From: Teon Banek Date: Wed, 3 May 2017 14:57:46 +0200 Subject: [PATCH] Plan Distinct Summary: Support WITH/RETURN DISTINCT in test macros. Test planning Distinct. Implement OutputSymbols in Distinct operator. Reviewers: florijan, mislav.bradac Reviewed By: mislav.bradac Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D340 --- src/query/plan/operator.cpp | 5 +++++ src/query/plan/operator.hpp | 1 + src/query/plan/planner.cpp | 13 ++++++++----- tests/unit/query_common.hpp | 14 ++++++++++---- tests/unit/query_planner.cpp | 30 ++++++++++++++++++++++++++++++ 5 files changed, 54 insertions(+), 9 deletions(-) diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index 9eda6b76c..aef907d73 100644 --- a/src/query/plan/operator.cpp +++ b/src/query/plan/operator.cpp @@ -1587,6 +1587,11 @@ std::unique_ptr Distinct::MakeCursor(GraphDbAccessor &db) { return std::make_unique(*this, db); } +std::vector Distinct::OutputSymbols(const SymbolTable &symbol_table) { + // Propagate this to potential Produce. + return input_->OutputSymbols(symbol_table); +} + Distinct::DistinctCursor::DistinctCursor(Distinct &self, GraphDbAccessor &db) : self_(self), input_cursor_(self.input_->MakeCursor(db)) {} diff --git a/src/query/plan/operator.hpp b/src/query/plan/operator.hpp index 6e22aa43a..0b348a366 100644 --- a/src/query/plan/operator.hpp +++ b/src/query/plan/operator.hpp @@ -1311,6 +1311,7 @@ class Distinct : public LogicalOperator { const std::vector &value_symbols); void Accept(LogicalOperatorVisitor &visitor) override; std::unique_ptr MakeCursor(GraphDbAccessor &db) override; + std::vector OutputSymbols(const SymbolTable &) override; private: const std::shared_ptr input_; diff --git a/src/query/plan/planner.cpp b/src/query/plan/planner.cpp index 820a57f9d..c1d387cbd 100644 --- a/src/query/plan/planner.cpp +++ b/src/query/plan/planner.cpp @@ -364,10 +364,6 @@ class ReturnBodyContext : public TreeVisitorBase { auto GenReturnBody(LogicalOperator *input_op, bool advance_command, const ReturnBodyContext &body, bool accumulate = false) { - if (body.distinct()) { - // TODO: Plan with distinct, when operator available. - throw utils::NotYetImplemented(); - } std::vector used_symbols(body.used_symbols().begin(), body.used_symbols().end()); auto last_op = input_op; @@ -390,6 +386,12 @@ auto GenReturnBody(LogicalOperator *input_op, bool advance_command, last_op = new Filter(std::shared_ptr(last_op), body.where()->expression_); } + // Distinct in ReturnBody only makes Produce values unique, so plan after it. + // Hopefully, it is more efficient to have Filter before Distinct. + if (body.distinct()) { + last_op = new Distinct(std::shared_ptr(last_op), + body.output_symbols()); + } // Like Where, OrderBy can read from symbols established by named expressions // in Produce, so it must come after it. if (!body.order_by().empty()) { @@ -538,7 +540,8 @@ std::unique_ptr MakeLogicalPlan( unwind->named_expression_->expression_, symbol_table.at(*unwind->named_expression_)); } else { - throw utils::NotYetImplemented(); + throw utils::NotYetImplemented( + "Encountered a clause which cannot be converted to operator(s)"); } } return std::unique_ptr(input_op); diff --git a/tests/unit/query_common.hpp b/tests/unit/query_common.hpp index 730a2b72c..60f1f6342 100644 --- a/tests/unit/query_common.hpp +++ b/tests/unit/query_common.hpp @@ -233,8 +233,9 @@ void FillReturnBody(ReturnBody &body, NamedExpression *named_expr, T... rest) { /// /// @sa GetWith template -auto GetReturn(AstTreeStorage &storage, T... exprs) { +auto GetReturn(AstTreeStorage &storage, bool distinct, T... exprs) { auto ret = storage.Create(); + ret->body_.distinct = distinct; FillReturnBody(ret->body_, exprs...); return ret; } @@ -246,8 +247,9 @@ auto GetReturn(AstTreeStorage &storage, T... exprs) { /// /// @sa GetReturn template -auto GetWith(AstTreeStorage &storage, T... exprs) { +auto GetWith(AstTreeStorage &storage, bool distinct, T... exprs) { auto with = storage.Create(); + with->body_.distinct = distinct; FillReturnBody(with->body_, exprs...); return with; } @@ -378,8 +380,12 @@ auto GetMerge(AstTreeStorage &storage, Pattern *pattern, OnMatch on_match, // Expression. It should be used with RETURN or WITH. For example: // RETURN(IDENT("n"), AS("n")) vs. RETURN(NEXPR("n", IDENT("n"))). #define AS(name) storage.Create((name)) -#define RETURN(...) query::test_common::GetReturn(storage, __VA_ARGS__) -#define WITH(...) query::test_common::GetWith(storage, __VA_ARGS__) +#define RETURN(...) query::test_common::GetReturn(storage, false, __VA_ARGS__) +#define WITH(...) query::test_common::GetWith(storage, false, __VA_ARGS__) +#define RETURN_DISTINCT(...) \ + query::test_common::GetReturn(storage, true, __VA_ARGS__) +#define WITH_DISTINCT(...) \ + query::test_common::GetWith(storage, true, __VA_ARGS__) #define UNWIND(...) query::test_common::GetUnwind(storage, __VA_ARGS__) #define ORDER_BY(...) query::test_common::GetOrderBy(__VA_ARGS__) #define SKIP(expr) \ diff --git a/tests/unit/query_planner.cpp b/tests/unit/query_planner.cpp index 2a7f9a089..93c0046b3 100644 --- a/tests/unit/query_planner.cpp +++ b/tests/unit/query_planner.cpp @@ -73,6 +73,7 @@ class PlanChecker : public LogicalOperatorVisitor { return false; } void Visit(Unwind &op) override { CheckOp(op); } + void Visit(Distinct &op) override { CheckOp(op); } std::list checkers_; @@ -119,6 +120,7 @@ using ExpectSkip = OpChecker; using ExpectLimit = OpChecker; using ExpectOrderBy = OpChecker; using ExpectUnwind = OpChecker; +using ExpectDistinct = OpChecker; class ExpectAccumulate : public OpChecker { public: @@ -680,4 +682,32 @@ TEST(TestLogicalPlanner, MatchUnwindReturn) { CheckPlan(*query, ExpectScanAll(), ExpectUnwind(), ExpectProduce()); } +TEST(TestLogicalPlanner, ReturnDistinctOrderBySkipLimit) { + // Test RETURN DISTINCT 1 ORDER BY 1 SKIP 1 LIMIT 1 + AstTreeStorage storage; + auto query = QUERY(RETURN_DISTINCT(LITERAL(1), AS("1"), ORDER_BY(LITERAL(1)), + SKIP(LITERAL(1)), LIMIT(LITERAL(1)))); + CheckPlan(*query, ExpectProduce(), ExpectDistinct(), ExpectOrderBy(), + ExpectSkip(), ExpectLimit()); +} + +TEST(TestLogicalPlanner, CreateWithDistinctSumWhereReturn) { + // Test CREATE (n) WITH DISTINCT SUM(n.prop) AS s WHERE s < 42 RETURN s + Dbms dbms; + auto dba = dbms.active(); + auto prop = dba->property("prop"); + AstTreeStorage storage; + auto node_n = NODE("n"); + auto sum = SUM(PROPERTY_LOOKUP("n", prop)); + auto query = + QUERY(CREATE(PATTERN(node_n)), WITH_DISTINCT(sum, AS("s")), + WHERE(LESS(IDENT("s"), LITERAL(42))), RETURN(IDENT("s"), AS("s"))); + auto symbol_table = MakeSymbolTable(*query); + auto acc = ExpectAccumulate({symbol_table.at(*node_n->identifier_)}); + auto aggr = ExpectAggregate({sum}, {}); + auto plan = MakeLogicalPlan(*query, symbol_table); + CheckPlan(*plan, symbol_table, ExpectCreateNode(), acc, aggr, ExpectProduce(), + ExpectFilter(), ExpectDistinct(), ExpectProduce()); +} + } // namespace