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

fix(plan): update pattern matching to specify successor counts #5047

Merged
merged 3 commits into from
Aug 9, 2022

Conversation

wolffcm
Copy link

@wolffcm wolffcm commented Aug 4, 2022

This PR builds upon @nathanielc's previous PR #4981 whose text I Include here:

The core of the problem is that the pattern matching logic
expects there to be only a single edge between a node and
its matched predecessor. This works and is required for rules
that intend to merge the matched nodes into a single node.
For example From -> Filter, if the from has other successors
you can't merge the filter into the from. In other cases this
restriction is not needed and prevents the rule for matching
when it is safe to do so. For example a join node whose predecessor
has a different successor, since the join rule doesn't rewrite its
predecessors it doesn't matter if they have other successors.

To solve this we need to be explicit about whether a pattern match
can have multiple successors or not. I propose we replace the existing
plan.Pat function with two functions plan.Multi and plan.Single.
plan.Multi will match a node and will allow that node to have multiple
successors, while the plan.Single will match a node and that node must
have exactly one successor. This changes two things about current patterns,
first it makes the number of successors expected explicit in the pattern,
second it moves the specification of which nodes can have multiple successors
and which cannot on the pattern that matches the node instead of the
successor of the node. Additionally plan.Any will allow multiple successors.

Fixes #4699

In addition to this I've updated this branch with a few changes:

  • Changed Single and Multi to SingleSuccessor etc, also updated comments in pattern.go
  • Fixed an issue with the join planner rule
  • Fixed an issue with the heuristic planner where if multiple rules fired for the same node successors would become disconnected (both of these issues were caught by the new join acceptance test)
  • Filed Issues with deleting a node from query plan in rules #5044 for future work, and added a test case for it in this PR

Checklist

Dear Author 👋, the following checks should be completed (or explicitly dismissed) before merging.

  • ✏️ Write a PR description, regardless of triviality, to include the value of this PR
  • 🔗 Reference related issues
  • 🏃 Test cases are included to exercise the new code
  • 🧪 If new packages are being introduced to stdlib, link to Working Group discussion notes and ensure it lands under experimental/ (N/A)
  • 📖 If language features are changing, ensure docs/Spec.md has been updated (N/A)

Dear Reviewer(s) 👋, you are responsible (among others) for ensuring the completeness and quality of the above before approval.

nathanielc and others added 2 commits August 4, 2022 07:26
The core of the problem is that the pattern matching logic
expects there to be only a single edge between a node and
its matched predecessor. This works and is required for rules
that intend to merge the matched nodes into a single node.
For example From -> Filter, if the from has other successors
you can't merge the filter into the from. In other cases this
restriction is not needed and prevents the rule for matching
when it is safe to do so. For example a join node whose predecessor
has a different successor, since the join rule doesn't rewrite its
predecessors it doesn't matter if they have other successors.

To solve this we need to be explicit about whether a pattern match
can have multiple successors or not. I propose we replace the existing
plan.Pat function with two functions plan.Multi and plan.Single.
plan.Multi will match a node and will allow that node to have multiple
successors, while the plan.Single will match a node and that node must
have exactly one successor. This changes two things about current patterns,
first it makes the number of successors expected explicit in the pattern,
second it moves the specification of which nodes can have multiple successors
and which cannot on the pattern that matches the node instead of the
successor of the node. Additionally plan.Any will allow multiple successors.

Fixes #4699
This commit builds on the original one comment by
- Addresses a topogical issue with the join rule
- Fixes an issue with the planner where successors would get lost
- Renames "Single" and "Multi" to "SingleSuccessor" etc
@wolffcm wolffcm requested a review from a team as a code owner August 4, 2022 19:31
@wolffcm wolffcm requested review from scbrickley and removed request for a team August 4, 2022 19:31
@wolffcm wolffcm changed the title Fix/plan ms fix(plan): update pattern matching to specify successor counts Aug 4, 2022
Copy link
Contributor

@scbrickley scbrickley left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for doing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Planner rules sometimes do not fire when they could
3 participants