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.

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

Enhancement/pattern expression #4916

Merged

Conversation

czpmango
Copy link
Contributor

@czpmango czpmango commented Nov 23, 2022

What type of PR is this?

  • bug
  • feature
  • enhancement

Description:

This pr is optimized for the case of pattern expression as a predicate. In this case, the runtime doesn't actually have to construct the path. The principle is similar to sql semi apply related optimization.

Compared to previous implementations, this pr has three main optimization points:

  • The runtime does not construct paths to pattern predicate(overhead of Project(pathBuild expression)).
  • Avoid the overhead of RollUpJoin, which is likely to have high cpu overhead(hash build and probe)and the large table being constructed will also have high memory overhead.
  • Avoid the overhead of Filter executor.

For example:
Query statement: MATCH (v:player)-->(n) where (v)-[:like]->() and size((v)-[:serve]-())>2 and v.age>3 and not (v)-[:teammate]-(n) RETURN id(v)
Old plan:
graphviz (3)
Optimized plan:
graphviz (4)

Special notes for your reviewer

I will fix some FIXMEs related to handling of expression later. Expression visitor needs to cover all expressions, and currently supports only simple expressions.

Checklist:

Tests:

  • Unit test(positive and negative cases)
  • Function test
  • Performance test
  • N/A

Affects:

  • Documentation affected (Please add the label if documentation needs to be modified.)
  • Incompatibility (If it breaks the compatibility, please describe it and add the label.)
  • If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
  • Performance impacted: Consumes more CPU/Memory

Release notes:

Optimize the implementation of pattern predicate.

@czpmango czpmango added the ready-for-testing PR: ready for the CI test label Nov 23, 2022
@czpmango czpmango force-pushed the enhancement/pattern-expression branch from e31cca8 to 6b9e137 Compare November 23, 2022 08:54
@czpmango czpmango marked this pull request as ready for review November 23, 2022 09:43
@czpmango czpmango force-pushed the enhancement/pattern-expression branch 2 times, most recently from 201dd5b to 23181f1 Compare November 23, 2022 09:56
jievince
jievince previously approved these changes Nov 23, 2022
Copy link
Contributor

@jievince jievince left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done!

@czpmango czpmango force-pushed the enhancement/pattern-expression branch 2 times, most recently from 634ea61 to 8ef67c7 Compare November 23, 2022 11:49
@czpmango
Copy link
Contributor Author

We should add PushFilterDownRollUpApply and PushFilterDownPatternApply rules.

@czpmango czpmango force-pushed the enhancement/pattern-expression branch from 8ef67c7 to 3b3ad6d Compare November 23, 2022 12:07
@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2022

Codecov Report

Base: 76.76% // Head: 76.79% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (8ca99eb) compared to base (8842efb).
Patch coverage: 83.39% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4916      +/-   ##
==========================================
+ Coverage   76.76%   76.79%   +0.03%     
==========================================
  Files        1101     1103       +2     
  Lines       81180    81425     +245     
==========================================
+ Hits        62317    62533     +216     
- Misses      18863    18892      +29     
Impacted Files Coverage Δ
src/graph/context/ast/CypherAstContext.h 100.00% <ø> (ø)
src/graph/optimizer/rule/RemoveNoopProjectRule.cpp 93.61% <ø> (ø)
src/graph/planner/plan/PlanNode.h 89.42% <ø> (ø)
src/graph/util/ExpressionUtils.h 100.00% <ø> (ø)
src/graph/planner/plan/Query.cpp 66.91% <30.43%> (-1.28%) ⬇️
src/graph/executor/query/PatternApplyExecutor.cpp 82.79% <82.79%> (ø)
src/graph/validator/MatchValidator.cpp 89.06% <84.93%> (-0.49%) ⬇️
src/graph/util/ExpressionUtils.cpp 94.27% <94.11%> (-0.01%) ⬇️
src/graph/executor/Executor.cpp 79.75% <100.00%> (+0.09%) ⬆️
src/graph/executor/query/PatternApplyExecutor.h 100.00% <100.00%> (ø)
... and 39 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Shylock-Hg
Copy link
Contributor

Please add test.

@czpmango
Copy link
Contributor Author

Please add test.

This pr can be considered as an optimization or refactoring, and all the statement behavior is guaranteed by previous tests related to pattern expression.
If the test case coverage is not complete, it will be evaluated and completed in the future planning.

jievince
jievince previously approved these changes Nov 24, 2022
@jievince jievince requested review from Aiee and nevermore3 November 24, 2022 08:30
jievince
jievince previously approved these changes Nov 25, 2022
Shylock-Hg
Shylock-Hg previously approved these changes Nov 25, 2022
@yixinglu
Copy link
Contributor

Now we don't hurry to tuning the LDBC according to the PR, you could transform the implementation to the optimizer rule.

@czpmango
Copy link
Contributor Author

Now we don't hurry to tuning the LDBC according to the PR, you could transform the implementation to the optimizer rule.

IMHO, implementing this in the optimizer is not necessarily better.
#4916 (comment)

@Sophie-Xie
Copy link
Contributor

Pls don‘t merge, wait for @yixinglu .

yixinglu
yixinglu previously approved these changes Dec 4, 2022
@Sophie-Xie Sophie-Xie added ready-for-testing PR: ready for the CI test and removed ready-for-testing PR: ready for the CI test labels Dec 5, 2022
jievince
jievince previously approved these changes Dec 5, 2022
fix pattern apply executor

add pattern predicate executor

add pattern predicate flag

fix edge cases

fix unused rollup apply

remove unnecessary project before PatternApply

small rename

fix anti-predicate

fix anti-predicate

fix where planner

fix not flattened expression

fix tck

fix tck

small delete
@czpmango czpmango dismissed stale reviews from jievince and yixinglu via e51fe53 December 5, 2022 07:40
@czpmango czpmango force-pushed the enhancement/pattern-expression branch from 59fb7a7 to e51fe53 Compare December 5, 2022 07:40
jievince
jievince previously approved these changes Dec 6, 2022
Copy link
Contributor

@yixinglu yixinglu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need some tck cases

@czpmango
Copy link
Contributor Author

czpmango commented Dec 6, 2022

Need some tck cases

As it says here: #4916 (comment)

@Shinji-IkariG Shinji-IkariG merged commit d9752a0 into vesoft-inc:master Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review ready-for-testing PR: ready for the CI test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants