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

[master < T1085] Remove double scan with expand from the planner #1085

Merged
merged 5 commits into from Sep 5, 2023

Conversation

Josipmrden
Copy link
Contributor

@Josipmrden Josipmrden commented Jul 20, 2023

Possibly it is not needed to do double ScanAll in the operator tree but expand from already existing node.

@vpavicic An optimization to the query engine was added in order to prefer at all times scanning + expansion of nodes instead of scanning both source and destination vertices and then expanding to the edge between them.
No need for documentation updates.

[master < Task] PR

  • Check, and update documentation if necessary
  • Provide the full content or a guide for the final git message

To keep docs changelog up to date, one more thing to do:

  • Write a release note here, including added/changed clauses
  • Tag someone from docs team in the comments

@Josipmrden Josipmrden added the feature feature label Jul 20, 2023
@Josipmrden Josipmrden added this to the mg-v2.10.0 milestone Jul 20, 2023
@Josipmrden Josipmrden requested a review from Ignition July 20, 2023 09:22
@Josipmrden Josipmrden self-assigned this Jul 20, 2023
@Josipmrden Josipmrden changed the title [master < T....] Remove double scan with expand from the planner [master < T1085] Remove double scan with expand from the planner Jul 20, 2023
@Ignition
Copy link
Contributor

Ignition commented Jul 25, 2023

@Josipmrden Looking at Grafana dashboard it does drop throughput on some of the mgbench workloads. I think we need to understand this some more.

The expand.common_.existing_node looks like it should prevents the change in the plan to the double scan. Maybe we should check why that isn't there in our case.

CREATE INDEX ON :Post;

// PLAN before data inserted (shouldn't be double scan because of post)
EXPLAIN MATCH (post:Post)
WHERE NOT EXISTS( (:Post)<-[:ROOT]-(post))
RETURN post;

UNWIND RANGE(0,1000) AS i
CREATE (:Post),(:Post),(:Post),(r:Post)
CREATE (:Post)-[:ROOT]->(r);

// PLAN remains unchanged
EXPLAIN MATCH (post:Post)
WHERE NOT EXISTS( (:Post)<-[:ROOT]-(post))
RETURN post;

@gitbuda gitbuda modified the milestones: mg-v2.10.0, mg-v2.11.0 Jul 31, 2023
@gitbuda gitbuda modified the milestones: mg-v2.11.0, mg-v2.12.0 Sep 1, 2023
@gitbuda
Copy link
Member

gitbuda commented Sep 1, 2023

Seems like a good update, but not good results on the mgbench -> investigation needed

@Josipmrden
Copy link
Contributor Author

@gitbuda After some talk with @Ignition we see there are no performance degradations on the benchmarks. We will proceed with the removal of the feature. Also, if the need occurs again, we will re-add the optimization with the appropriate benchmark tests that will point whether we have degraded any queries.

@Josipmrden Josipmrden marked this pull request as ready for review September 4, 2023 14:18
@Josipmrden Josipmrden merged commit 09fd593 into master Sep 5, 2023
6 checks passed
@Josipmrden Josipmrden deleted the fix-double-scan-expand branch September 5, 2023 09:02
@gitbuda gitbuda modified the milestones: mg-v2.12.0, mg-v2.11.0 Sep 9, 2023
@vpavicic vpavicic added the Docs - changelog only Docs - changelog only label Sep 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docs - changelog only Docs - changelog only feature feature
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants