fix(interactive): Support Multiple Matches in IR Core #3134

BingqingLyu · 2023-08-22T02:52:20Z

What do these changes do?

As titled, support Multiple Matches in IR Core

From now, the logical plan will contain contain a dummy RootScan after initialization by default. This dummy RootScan may be later used to support multiple scan.
When a plan of a pattern matching is append to a node, if the plan is naive plan, it will directly append to the assigned node. Otherwise, it the plan is extend plan, then it must have its own scan operator, so it have to be append to the dummy RootScan
When a sink operator is append to the logical plan, it suggests that the logical plan building is finished. Therefore, it will call clean_redundant_nodes method to clean useless nodes. There are two kinds of reduandant nodes: Scan node with no child and dummy RootScan with zero or only single child.

Related issue number

Fixes #3052 #3062

Committed-by: bingqing.lbq from Dev container

1. Fix the bug of multiple pattern matching 2. Add more test cases for pattern matching

1. Edit testcases 2. Fix some bugs 3. Now the new multiple match implementation with dummy source can pass all the tests in the rust side

1. Update clean redundant methods. Now it will firstly remove all the redundant scan nodes, and then check the redundant dummy root node 2. It will ensure remove a redundant dummy root nodes which followed by another several of redundant scan nodes

…hScope into ir_multiple_match

BingqingLyu · 2023-08-22T02:52:49Z

interactive_engine/executor/ir/common/src/utils.rs

-                            .unwrap_or(true)
-                }
-                _ => false,
+impl pb::LogicalPlan {


This can be removed.

BingqingLyu · 2023-08-22T02:53:48Z

interactive_engine/executor/ir/core/src/plan/logical.rs

@@ -155,7 +168,7 @@ impl TryFrom<pb::LogicalPlan> for LogicalPlan {
                return Err(ParsePbError::EmptyFieldError("Node::opr".to_string()));
            }
        }
-
+        plan.clean_redundant_nodes();


Seems would occur a duplicated clean_redundant_nodes() since it is also called in append_operator_as_node()?

Fixed, and edit a testcase

BingqingLyu · 2023-08-22T02:54:18Z

interactive_engine/executor/ir/core/src/plan/logical.rs

-        let new_curr_node_rst = match opr.opr.as_ref().unwrap() {
-            Opr::Pattern(pattern) => {
+
+        let new_curr_node_rst = match opr.opr.as_ref() {


can directly unwrap() since it is already checked.

I think it is better to avoid unwrap in official code

BingqingLyu · 2023-08-22T03:07:41Z

interactive_engine/executor/ir/core/src/plan/logical.rs

+            false
+        }
+    } else {
+        false


Even if store_meta is not provided, params.tables.is_empty() also indicates all_labels()?

1. remove `append_root` method 2. avoid repeated `clean_redundant_nodes()` use in try_from pb::LogicalPlan to LogicalPlan 3. Change the logic of `is_all_parameters()`. Now, as well as the params.tables.is_empty(), it is all parameters, no matter whether the store meta is provided or not 4. Remove an error testcase about multiple matches because it contains nested branch, which will be supoorted in the future version

…hScope into ir_multiple_match

1. In TryFrom pb::LogicalPlan to LogicalPlan, use pb.roots to suggests the root instead of id=0 2. Add comment about redirecting the parents of pb.roots to the dummy root of the LogicalPlan

codecov-commenter · 2023-08-22T10:13:40Z

Codecov Report

Merging #3134 (180e6c7) into main (88c5180) will not change coverage.
Report is 1 commits behind head on main.
The diff coverage is n/a.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3134   +/-   ##
=======================================
  Coverage   42.64%   42.64%           
=======================================
  Files         100      100           
  Lines       10817    10817           
=======================================
  Hits         4613     4613           
  Misses       6204     6204

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f701fb8...180e6c7. Read the comment docs.

longbinlai · 2023-08-24T14:05:15Z

interactive_engine/executor/ir/core/src/plan/logical.rs

+        }
+}
+
+fn is_node_dummy(node: &Node) -> bool {


Why not implement as a member function of Node? Similar for is_node_scan. Furthermore, dummy not a good name for the function, call it root

longbinlai · 2023-08-24T14:07:39Z

interactive_engine/executor/ir/proto/algebra.proto

@@ -469,6 +469,9 @@ message SinkVineyard {
  schema.Schema graph_schema = 2;
 }

+// A dummy node to delegate a source opr for multiple scan cases.
+message RootScan {}


Do not like RootScan, Simply call it Root.

longbinlai

LGTM

BingqingLyu and others added 9 commits July 28, 2023 11:13

[Proto] add a Dummy RootScan opr in proto

1503ceb

Committed-by: bingqing.lbq from Dev container

test for multi-match logical plan and physical plan

35c4d9f

Committed-by: bingqing.lbq from Dev container

[Fix]

45a2c8b

1. Fix the bug of multiple pattern matching 2. Add more test cases for pattern matching

[Test]

f463f75

1. Edit testcases 2. Fix some bugs 3. Now the new multiple match implementation with dummy source can pass all the tests in the rust side

minor: test for multi-source-multi-match logical plan and physical plan

ab0d61d

[Fix]

6c5b1d3

1. Update clean redundant methods. Now it will firstly remove all the redundant scan nodes, and then check the redundant dummy root node 2. It will ensure remove a redundant dummy root nodes which followed by another several of redundant scan nodes

Merge branch 'ir_multiple_match' of https://github.com/shirly121/Grap…

992a61d

…hScope into ir_multiple_match

[CI Tests] more multi source related ci tests

df19908

minor: fix ci plan

097b7db

longbinlai changed the title ~~fix(interactive) Support Multiple Matches in IR Core~~ fix(interactive): Support Multiple Matches in IR Core Aug 22, 2023

BingqingLyu commented Aug 22, 2023

View reviewed changes

BingqingLyu and others added 5 commits August 22, 2023 11:33

fix ci tests

653252b

fix ci tests

833c583

Merge branch 'ir_multiple_match' of https://github.com/shirly121/Grap…

dcba2a2

…hScope into ir_multiple_match

[Fix]

89e0f3f

1. In TryFrom pb::LogicalPlan to LogicalPlan, use pb.roots to suggests the root instead of id=0 2. Add comment about redirecting the parents of pb.roots to the dummy root of the LogicalPlan

BingqingLyu marked this pull request as draft August 22, 2023 10:08

shirly121 and others added 2 commits August 24, 2023 11:22

[GIE Compiler] remove ffi build unit test from compiler

86a0cc4

Merge branch 'main' into ir_multiple_match

180e6c7

BingqingLyu marked this pull request as ready for review August 24, 2023 04:15

BingqingLyu requested review from longbinlai and shirly121 August 24, 2023 04:15

longbinlai reviewed Aug 24, 2023

View reviewed changes

BingqingLyu and others added 2 commits August 28, 2023 10:50

minor refine

02a1118

Merge branch 'main' into ir_multiple_match

cdcf14f

longbinlai approved these changes Aug 28, 2023

View reviewed changes

shirly121 approved these changes Aug 28, 2023

View reviewed changes

BingqingLyu merged commit f71a08d into alibaba:main Aug 28, 2023

BingqingLyu deleted the ir_multiple_match branch August 28, 2023 05:26

BingqingLyu mentioned this pull request Sep 18, 2023

[BUG (interactive)] Scan operator is undesirably removed while appending a Pattern operator #3062

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(interactive): Support Multiple Matches in IR Core #3134

fix(interactive): Support Multiple Matches in IR Core #3134

BingqingLyu commented Aug 22, 2023 •

edited by MeloYang05

Loading

BingqingLyu Aug 22, 2023

MeloYang05 Aug 22, 2023

BingqingLyu Aug 22, 2023

MeloYang05 Aug 22, 2023

BingqingLyu Aug 22, 2023

MeloYang05 Aug 22, 2023

BingqingLyu Aug 22, 2023 •

edited

Loading

MeloYang05 Aug 22, 2023

codecov-commenter commented Aug 22, 2023 •

edited

Loading

longbinlai Aug 24, 2023 •

edited

Loading

BingqingLyu Aug 28, 2023

longbinlai Aug 24, 2023

BingqingLyu Aug 28, 2023

longbinlai left a comment

fix(interactive): Support Multiple Matches in IR Core #3134

fix(interactive): Support Multiple Matches in IR Core #3134

Conversation

BingqingLyu commented Aug 22, 2023 • edited by MeloYang05 Loading

What do these changes do?

Related issue number

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BingqingLyu Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 22, 2023 • edited Loading

Codecov Report

longbinlai Aug 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

longbinlai left a comment

Choose a reason for hiding this comment

BingqingLyu commented Aug 22, 2023 •

edited by MeloYang05

Loading

BingqingLyu Aug 22, 2023 •

edited

Loading

codecov-commenter commented Aug 22, 2023 •

edited

Loading

longbinlai Aug 24, 2023 •

edited

Loading