From 672a43c812c1638b23ce9f2b044c2644b31c0585 Mon Sep 17 00:00:00 2001 From: Shylock Hg <33566796+Shylock-Hg@users.noreply.github.com> Date: Mon, 30 Aug 2021 17:58:06 +0800 Subject: [PATCH 1/2] Avoid the extra expression encode decode. --- src/clients/storage/GraphStorageClient.cpp | 12 ++-- src/clients/storage/GraphStorageClient.h | 4 +- src/graph/optimizer/OptimizerUtils.cpp | 2 +- .../rule/PushFilterDownGetNbrsRule.cpp | 9 ++- src/graph/planner/plan/Query.cpp | 3 +- src/graph/planner/plan/Query.h | 54 +++++++----------- src/graph/planner/plan/Scan.h | 56 +++++++++---------- src/graph/validator/FetchEdgesValidator.cpp | 2 +- src/graph/validator/FetchEdgesValidator.h | 2 +- .../validator/FetchVerticesValidator.cpp | 2 +- src/graph/validator/FetchVerticesValidator.h | 2 +- tests/Makefile | 2 +- 12 files changed, 68 insertions(+), 82 deletions(-) diff --git a/src/clients/storage/GraphStorageClient.cpp b/src/clients/storage/GraphStorageClient.cpp index 62c2419c72c..475f321ac82 100644 --- a/src/clients/storage/GraphStorageClient.cpp +++ b/src/clients/storage/GraphStorageClient.cpp @@ -25,7 +25,7 @@ folly::SemiFuture> GraphStorageCl bool random, const std::vector& orderBy, int64_t limit, - std::string filter, + const Expression* filter, folly::EventBase* evb) { auto cbStatus = getIdFromRow(space, false); if (!cbStatus.ok()) { @@ -69,8 +69,8 @@ folly::SemiFuture> GraphStorageCl spec.set_order_by(orderBy); } spec.set_limit(limit); - if (filter.size() > 0) { - spec.set_filter(filter); + if (filter != nullptr) { + spec.set_filter(filter->encode()); } req.set_traverse_spec(std::move(spec)); } @@ -167,7 +167,7 @@ folly::SemiFuture> GraphStorageClient: bool dedup, const std::vector& orderBy, int64_t limit, - std::string filter, + const Expression* filter, folly::EventBase* evb) { auto cbStatus = getIdFromRow(space, edgeProps != nullptr); if (!cbStatus.ok()) { @@ -202,8 +202,8 @@ folly::SemiFuture> GraphStorageClient: req.set_order_by(orderBy); } req.set_limit(limit); - if (filter.size() > 0) { - req.set_filter(filter); + if (filter != nullptr) { + req.set_filter(filter->encode()); } } diff --git a/src/clients/storage/GraphStorageClient.h b/src/clients/storage/GraphStorageClient.h index ddf27e3f8c9..83dafb784b6 100644 --- a/src/clients/storage/GraphStorageClient.h +++ b/src/clients/storage/GraphStorageClient.h @@ -47,7 +47,7 @@ class GraphStorageClient : public StorageClientBase& orderBy = std::vector(), int64_t limit = std::numeric_limits::max(), - std::string filter = std::string(), + const Expression* filter = nullptr, folly::EventBase* evb = nullptr); folly::SemiFuture> getProps( @@ -59,7 +59,7 @@ class GraphStorageClient : public StorageClientBase& orderBy = std::vector(), int64_t limit = std::numeric_limits::max(), - std::string filter = std::string(), + const Expression* filter = nullptr, folly::EventBase* evb = nullptr); folly::SemiFuture> addVertices( diff --git a/src/graph/optimizer/OptimizerUtils.cpp b/src/graph/optimizer/OptimizerUtils.cpp index 79ca8518634..53f7e89c240 100644 --- a/src/graph/optimizer/OptimizerUtils.cpp +++ b/src/graph/optimizer/OptimizerUtils.cpp @@ -922,7 +922,7 @@ void OptimizerUtils::copyIndexScanData(const nebula::graph::IndexScan* from, to->setDedup(from->dedup()); to->setOrderBy(from->orderBy()); to->setLimit(from->limit()); - to->setFilter(from->filter()); + to->setFilter(from->filter() == nullptr ? nullptr : from->filter()->clone()); } } // namespace graph diff --git a/src/graph/optimizer/rule/PushFilterDownGetNbrsRule.cpp b/src/graph/optimizer/rule/PushFilterDownGetNbrsRule.cpp index 3e74820e877..4ffa80807ae 100644 --- a/src/graph/optimizer/rule/PushFilterDownGetNbrsRule.cpp +++ b/src/graph/optimizer/rule/PushFilterDownGetNbrsRule.cpp @@ -74,11 +74,10 @@ StatusOr PushFilterDownGetNbrsRule::transform( newFilterGroupNode = OptGroupNode::create(ctx, newFilter, filterGroupNode->group()); } - auto newGNFilter = condition->encode(); - if (!gn->filter().empty()) { - auto filterExpr = Expression::decode(pool, gn->filter()); - auto logicExpr = LogicalExpression::makeAnd(pool, condition, filterExpr); - newGNFilter = logicExpr->encode(); + auto newGNFilter = condition; + if (gn->filter() != nullptr) { + auto logicExpr = LogicalExpression::makeAnd(pool, condition, gn->filter()->clone()); + newGNFilter = logicExpr; } auto newGN = static_cast(gn->clone()); diff --git a/src/graph/planner/plan/Query.cpp b/src/graph/planner/plan/Query.cpp index 02a3eead3e6..69b8632248a 100644 --- a/src/graph/planner/plan/Query.cpp +++ b/src/graph/planner/plan/Query.cpp @@ -23,8 +23,7 @@ std::unique_ptr Explore::explain() const { addDescription("space", folly::to(space_), desc.get()); addDescription("dedup", util::toJson(dedup_), desc.get()); addDescription("limit", folly::to(limit_), desc.get()); - auto filter = - filter_.empty() ? filter_ : Expression::decode(qctx_->objPool(), filter_)->toString(); + std::string filter = filter_ == nullptr ? "" : filter_->toString(); addDescription("filter", filter, desc.get()); addDescription("orderBy", folly::toJson(util::toJson(orderBy_)), desc.get()); return desc; diff --git a/src/graph/planner/plan/Query.h b/src/graph/planner/plan/Query.h index 92ba36dbcbc..315baa43840 100644 --- a/src/graph/planner/plan/Query.h +++ b/src/graph/planner/plan/Query.h @@ -37,7 +37,9 @@ class Explore : public SingleInputNode { int64_t limit() const { return limit_; } - const std::string& filter() const { return filter_; } + const Expression* filter() const { return filter_; } + + Expression* filter() { return filter_; } const std::vector& orderBy() const { return orderBy_; } @@ -45,7 +47,7 @@ class Explore : public SingleInputNode { void setLimit(int64_t limit) { limit_ = limit; } - void setFilter(std::string filter) { filter_ = std::move(filter); } + void setFilter(Expression* filter) { filter_ = filter; } void setOrderBy(std::vector orderBy) { orderBy_ = std::move(orderBy); } @@ -58,13 +60,13 @@ class Explore : public SingleInputNode { GraphSpaceID space, bool dedup, int64_t limit, - std::string filter, + Expression* filter, std::vector orderBy) : SingleInputNode(qctx, kind, input), space_(space), dedup_(dedup), limit_(limit), - filter_(std::move(filter)), + filter_(filter), orderBy_(std::move(orderBy)) {} Explore(QueryContext* qctx, Kind kind, PlanNode* input, GraphSpaceID space) @@ -76,7 +78,7 @@ class Explore : public SingleInputNode { GraphSpaceID space_; bool dedup_{false}; int64_t limit_{std::numeric_limits::max()}; - std::string filter_; + Expression* filter_{nullptr}; std::vector orderBy_; }; @@ -108,7 +110,7 @@ class GetNeighbors final : public Explore { bool random = false, std::vector orderBy = {}, int64_t limit = -1, - std::string filter = "") { + Expression* filter = nullptr) { auto gn = make(qctx, input, space); gn->setSrc(src); gn->setEdgeTypes(std::move(edgeTypes)); @@ -121,7 +123,7 @@ class GetNeighbors final : public Explore { gn->setDedup(dedup); gn->setOrderBy(std::move(orderBy)); gn->setLimit(limit); - gn->setFilter(std::move(filter)); + gn->setFilter(filter); return gn; } @@ -199,7 +201,7 @@ class GetVertices final : public Explore { bool dedup = false, std::vector orderBy = {}, int64_t limit = std::numeric_limits::max(), - std::string filter = "") { + Expression* filter = nullptr) { return qctx->objPool()->add(new GetVertices(qctx, input, space, @@ -209,7 +211,7 @@ class GetVertices final : public Explore { dedup, std::move(orderBy), limit, - std::move(filter))); + filter)); } Expression* src() const { return src_; } @@ -237,15 +239,8 @@ class GetVertices final : public Explore { bool dedup, std::vector orderBy, int64_t limit, - std::string filter) - : Explore(qctx, - Kind::kGetVertices, - input, - space, - dedup, - limit, - std::move(filter), - std::move(orderBy)), + Expression* filter) + : Explore(qctx, Kind::kGetVertices, input, space, dedup, limit, filter, std::move(orderBy)), src_(src), props_(std::move(props)), exprs_(std::move(exprs)) {} @@ -278,7 +273,7 @@ class GetEdges final : public Explore { bool dedup = false, int64_t limit = std::numeric_limits::max(), std::vector orderBy = {}, - std::string filter = "") { + Expression* filter = nullptr) { return qctx->objPool()->add(new GetEdges(qctx, input, space, @@ -291,7 +286,7 @@ class GetEdges final : public Explore { dedup, limit, std::move(orderBy), - std::move(filter))); + filter)); } Expression* src() const { return src_; } @@ -326,15 +321,8 @@ class GetEdges final : public Explore { bool dedup, int64_t limit, std::vector orderBy, - std::string filter) - : Explore(qctx, - Kind::kGetEdges, - input, - space, - dedup, - limit, - std::move(filter), - std::move(orderBy)), + Expression* filter) + : Explore(qctx, Kind::kGetEdges, input, space, dedup, limit, filter, std::move(orderBy)), src_(src), type_(type), ranking_(ranking), @@ -374,7 +362,7 @@ class IndexScan : public Explore { bool dedup = false, std::vector orderBy = {}, int64_t limit = std::numeric_limits::max(), - std::string filter = "") { + Expression* filter = nullptr) { return qctx->objPool()->add(new IndexScan(qctx, input, space, @@ -386,7 +374,7 @@ class IndexScan : public Explore { dedup, std::move(orderBy), limit, - std::move(filter))); + filter)); } const std::vector& queryContext() const { return contexts_; } @@ -426,9 +414,9 @@ class IndexScan : public Explore { bool dedup, std::vector orderBy, int64_t limit, - std::string filter, + Expression* filter, Kind kind = Kind::kIndexScan) - : Explore(qctx, kind, input, space, dedup, limit, std::move(filter), std::move(orderBy)) { + : Explore(qctx, kind, input, space, dedup, limit, filter, std::move(orderBy)) { contexts_ = std::move(contexts); returnCols_ = std::move(returnCols); isEdge_ = isEdge; diff --git a/src/graph/planner/plan/Scan.h b/src/graph/planner/plan/Scan.h index 03040d62012..4df13613d61 100644 --- a/src/graph/planner/plan/Scan.h +++ b/src/graph/planner/plan/Scan.h @@ -29,7 +29,7 @@ class EdgeIndexScan : public IndexScan { bool dedup, std::vector orderBy, int64_t limit, - std::string filter, + Expression* filter, Kind kind) : IndexScan(qctx, input, @@ -42,7 +42,7 @@ class EdgeIndexScan : public IndexScan { dedup, std::move(orderBy), limit, - std::move(filter), + filter, kind), edgeType_(edgeType) {} @@ -62,7 +62,7 @@ class EdgeIndexPrefixScan : public EdgeIndexScan { bool dedup = false, std::vector orderBy = {}, int64_t limit = std::numeric_limits::max(), - std::string filter = "") { + Expression* filter = nullptr) { return qctx->objPool()->add(new EdgeIndexPrefixScan(qctx, input, edgeType, @@ -74,7 +74,7 @@ class EdgeIndexPrefixScan : public EdgeIndexScan { dedup, std::move(orderBy), limit, - std::move(filter))); + filter)); } private: @@ -89,7 +89,7 @@ class EdgeIndexPrefixScan : public EdgeIndexScan { bool dedup, std::vector orderBy, int64_t limit, - std::string filter) + Expression* filter) : EdgeIndexScan(qctx, input, edgeType, @@ -101,7 +101,7 @@ class EdgeIndexPrefixScan : public EdgeIndexScan { dedup, std::move(orderBy), limit, - std::move(filter), + filter, Kind::kEdgeIndexPrefixScan) {} }; @@ -118,7 +118,7 @@ class EdgeIndexRangeScan : public EdgeIndexScan { bool dedup = false, std::vector orderBy = {}, int64_t limit = std::numeric_limits::max(), - std::string filter = "") { + Expression* filter = nullptr) { return qctx->objPool()->add(new EdgeIndexRangeScan(qctx, input, edgeType, @@ -130,7 +130,7 @@ class EdgeIndexRangeScan : public EdgeIndexScan { dedup, std::move(orderBy), limit, - std::move(filter))); + filter)); } private: @@ -145,7 +145,7 @@ class EdgeIndexRangeScan : public EdgeIndexScan { bool dedup, std::vector orderBy, int64_t limit, - std::string filter) + Expression* filter) : EdgeIndexScan(qctx, input, edgeType, @@ -157,7 +157,7 @@ class EdgeIndexRangeScan : public EdgeIndexScan { dedup, std::move(orderBy), limit, - std::move(filter), + filter, Kind::kEdgeIndexRangeScan) {} }; @@ -174,7 +174,7 @@ class EdgeIndexFullScan final : public EdgeIndexScan { bool dedup = false, std::vector orderBy = {}, int64_t limit = std::numeric_limits::max(), - std::string filter = "") { + Expression* filter = nullptr) { return qctx->objPool()->add(new EdgeIndexFullScan(qctx, input, edgeType, @@ -186,7 +186,7 @@ class EdgeIndexFullScan final : public EdgeIndexScan { dedup, std::move(orderBy), limit, - std::move(filter))); + filter)); } private: @@ -201,7 +201,7 @@ class EdgeIndexFullScan final : public EdgeIndexScan { bool dedup, std::vector orderBy, int64_t limit, - std::string filter) + Expression* filter) : EdgeIndexScan(qctx, input, edgeType, @@ -213,7 +213,7 @@ class EdgeIndexFullScan final : public EdgeIndexScan { dedup, std::move(orderBy), limit, - std::move(filter), + filter, Kind::kEdgeIndexFullScan) {} }; @@ -235,7 +235,7 @@ class TagIndexScan : public IndexScan { bool dedup, std::vector orderBy, int64_t limit, - std::string filter, + Expression* filter, Kind kind) : IndexScan(qctx, input, @@ -248,7 +248,7 @@ class TagIndexScan : public IndexScan { dedup, std::move(orderBy), limit, - std::move(filter), + filter, kind), tagName_(tagName) {} @@ -268,7 +268,7 @@ class TagIndexPrefixScan : public TagIndexScan { bool dedup = false, std::vector orderBy = {}, int64_t limit = std::numeric_limits::max(), - std::string filter = "") { + Expression* filter = nullptr) { return qctx->objPool()->add(new TagIndexPrefixScan(qctx, input, tagName, @@ -280,7 +280,7 @@ class TagIndexPrefixScan : public TagIndexScan { dedup, std::move(orderBy), limit, - std::move(filter))); + filter)); } private: @@ -295,7 +295,7 @@ class TagIndexPrefixScan : public TagIndexScan { bool dedup, std::vector orderBy, int64_t limit, - std::string filter) + Expression* filter) : TagIndexScan(qctx, input, tagName, @@ -307,7 +307,7 @@ class TagIndexPrefixScan : public TagIndexScan { dedup, std::move(orderBy), limit, - std::move(filter), + filter, Kind::kTagIndexPrefixScan) {} }; @@ -324,7 +324,7 @@ class TagIndexRangeScan : public TagIndexScan { bool dedup = false, std::vector orderBy = {}, int64_t limit = std::numeric_limits::max(), - std::string filter = "") { + Expression* filter = nullptr) { return qctx->objPool()->add(new TagIndexRangeScan(qctx, input, tagName, @@ -336,7 +336,7 @@ class TagIndexRangeScan : public TagIndexScan { dedup, std::move(orderBy), limit, - std::move(filter))); + filter)); } private: @@ -351,7 +351,7 @@ class TagIndexRangeScan : public TagIndexScan { bool dedup, std::vector orderBy, int64_t limit, - std::string filter) + Expression* filter) : TagIndexScan(qctx, input, tagName, @@ -363,7 +363,7 @@ class TagIndexRangeScan : public TagIndexScan { dedup, std::move(orderBy), limit, - std::move(filter), + filter, Kind::kTagIndexRangeScan) {} }; @@ -380,7 +380,7 @@ class TagIndexFullScan final : public TagIndexScan { bool dedup = false, std::vector orderBy = {}, int64_t limit = std::numeric_limits::max(), - std::string filter = "") { + Expression* filter = nullptr) { return qctx->objPool()->add(new TagIndexFullScan(qctx, input, tagName, @@ -392,7 +392,7 @@ class TagIndexFullScan final : public TagIndexScan { dedup, std::move(orderBy), limit, - std::move(filter))); + filter)); } private: @@ -407,7 +407,7 @@ class TagIndexFullScan final : public TagIndexScan { bool dedup, std::vector orderBy, int64_t limit, - std::string filter) + Expression* filter) : TagIndexScan(qctx, input, tagName, @@ -419,7 +419,7 @@ class TagIndexFullScan final : public TagIndexScan { dedup, std::move(orderBy), limit, - std::move(filter), + filter, Kind::kTagIndexFullScan) {} }; diff --git a/src/graph/validator/FetchEdgesValidator.cpp b/src/graph/validator/FetchEdgesValidator.cpp index 5909e1bbc4a..60c5636389f 100644 --- a/src/graph/validator/FetchEdgesValidator.cpp +++ b/src/graph/validator/FetchEdgesValidator.cpp @@ -46,7 +46,7 @@ Status FetchEdgesValidator::toPlan() { dedup_, limit_, std::move(orderBy_), - std::move(filter_)); + filter_); getEdgesNode->setInputVar(edgeKeysVar); getEdgesNode->setColNames(geColNames_); // the pipe will set the input variable diff --git a/src/graph/validator/FetchEdgesValidator.h b/src/graph/validator/FetchEdgesValidator.h index 6009b12e7e7..c20e94ca3d0 100644 --- a/src/graph/validator/FetchEdgesValidator.h +++ b/src/graph/validator/FetchEdgesValidator.h @@ -72,7 +72,7 @@ class FetchEdgesValidator final : public Validator { bool dedup_{false}; int64_t limit_{std::numeric_limits::max()}; std::vector orderBy_{}; - std::string filter_{""}; + Expression* filter_{nullptr}; // valid when yield expression not require storage // So expression like these will be evaluate in Project Executor bool withYield_{false}; diff --git a/src/graph/validator/FetchVerticesValidator.cpp b/src/graph/validator/FetchVerticesValidator.cpp index fc72408d85c..10f870ea231 100644 --- a/src/graph/validator/FetchVerticesValidator.cpp +++ b/src/graph/validator/FetchVerticesValidator.cpp @@ -37,7 +37,7 @@ Status FetchVerticesValidator::toPlan() { dedup_, std::move(orderBy_), limit_, - std::move(filter_)); + filter_); getVerticesNode->setInputVar(vidsVar); getVerticesNode->setColNames(gvColNames_); // pipe will set the input variable diff --git a/src/graph/validator/FetchVerticesValidator.h b/src/graph/validator/FetchVerticesValidator.h index 4a9dd4a1506..b915a3cc0de 100644 --- a/src/graph/validator/FetchVerticesValidator.h +++ b/src/graph/validator/FetchVerticesValidator.h @@ -52,7 +52,7 @@ class FetchVerticesValidator final : public Validator { bool dedup_{false}; std::vector orderBy_{}; int64_t limit_{std::numeric_limits::max()}; - std::string filter_{}; + Expression* filter_{nullptr}; // valid when yield expression not require storage // So expression like these will be evaluate in Project Executor bool withYield_{false}; diff --git a/tests/Makefile b/tests/Makefile index 10fc66d046b..dbfb82387bd 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -70,7 +70,7 @@ slow-query: currdir python3 -m pytest -n$(J) -m "not skip" tck/steps/test_kill_slow_query_via_same_service.py && \ python3 -m pytest -n$(J) -m "not skip" tck/steps/test_kill_slow_query_via_different_service.py -tck: slow-query +tck: python3 -m pytest -n$(J) -m "not skip" tck/steps/test_tck.py fail: currdir From a95f9346a54e34e86b5ebf09bd76c07318a788f1 Mon Sep 17 00:00:00 2001 From: Shylock Hg <33566796+Shylock-Hg@users.noreply.github.com> Date: Fri, 10 Sep 2021 10:45:01 +0800 Subject: [PATCH 2/2] Revert the mistake modification. --- tests/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Makefile b/tests/Makefile index dbfb82387bd..10fc66d046b 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -70,7 +70,7 @@ slow-query: currdir python3 -m pytest -n$(J) -m "not skip" tck/steps/test_kill_slow_query_via_same_service.py && \ python3 -m pytest -n$(J) -m "not skip" tck/steps/test_kill_slow_query_via_different_service.py -tck: +tck: slow-query python3 -m pytest -n$(J) -m "not skip" tck/steps/test_tck.py fail: currdir