add rule and update op_var_len_expand #485
Conversation
peace-dove
commented
Apr 24, 2024
peace-dove
commented
- add rule: push edge filter into op_var_len_expand
- update: non-recursive version of DFS in op_var_len_expand, searching edge with pruning
|
|
peace-dove
force-pushed
the
dev_op_var_len_expand
branch
from
0b11481 to
3b17978
Compare
peace-dove
force-pushed
the
dev_op_var_len_expand
branch
from
3b17978 to
6a986b7
Compare
peace-dove
force-pushed
the
dev_op_var_len_expand
branch
from
5f5707c to
4b4b9ca
Compare
|
Please reapprove the workflow, thanks. @qishipengqsp |
|
Return to optimized version 1, where all edges in the path need to be scanned during the expansion. |
|
It is ready for review. @qishipengqsp |
There was a problem hiding this comment.
Add more unit test cases to check the functionality and the OPT rule.
Generally LGTM to me in the discussed specific case. Can be more pervasive with more general scenarios as input.
OPT checks in this PR in the future:
- Hard encoding of the function names to check the rule
- The design of predicates for scalability consideration
- Double-check the correctness after making it more pervasive
| } | ||
| } | ||
|
|
||
| bool _FindEdgeFilter(OpBase *root, OpFilter *&op_filter, VarLenExpand *&op_varlenexpand) { |
There was a problem hiding this comment.
do we check the pattern isAsc() and isDesc() ?
There was a problem hiding this comment.
Now, all predicates for op_var_len_expand allowed by cypher can be optimized by this rule, so maybe there is no need to check here now.
There was a problem hiding this comment.
more test cases for this rule are expected
There was a problem hiding this comment.
I find some tests in tugraph-db/test/resource/cases/finbench/cypher/. I did not find test cases for other RBO rules, I dare not add test files at random.
There was a problem hiding this comment.
nice try to move the code to source file
There was a problem hiding this comment.
And this can speed up compilation.
Should I keep it in the source file or keep it consistent with other files and move it to the header file?
| if (tmp_filter->GetAeLeft().op.type == cypher::ArithOpNode::AR_OP_FUNC) { | ||
| std::string func_name = tmp_filter->GetAeLeft().op.func_name; | ||
| std::transform(func_name.begin(), func_name.end(), func_name.begin(), ::tolower); | ||
| if (func_name == "isasc") { |
There was a problem hiding this comment.
hard coding of func_name is not a good idea.
There was a problem hiding this comment.
Does this mean to use BuiltinFunction::FUNC to determine which function?
There was a problem hiding this comment.
The applying of the edge filter is relevant to the language expression of RPQ.
There was a problem hiding this comment.
Yes, but it's still not clear whether there is any predicate on the path that can be optimized in this way.
| addPredicate(std::move(p)); | ||
| } else if (func_name == "isdesc") { | ||
| auto p = std::make_unique<IsDescPredicate>(); | ||
| addPredicate(std::move(p)); |
There was a problem hiding this comment.
The move function is redundant to the one inside addPredicate
There was a problem hiding this comment.
It seems that without this std::move, the compilation will fail.
Reviewed. Will discuss this PR offline |
peace-dove
force-pushed
the
dev_op_var_len_expand
branch
from
fcab98b to
b49f88a
Compare
