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.

Sign up for GitHub

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

Jump to bottom

Return cartesian product on multi match clauses #1193

Merged
merged 56 commits into from
Oct 24, 2023
Merged

Return cartesian product on multi match clauses #1193

merged 56 commits into from
Oct 24, 2023

Conversation

Josipmrden
Copy link
Contributor

In multi match clauses, the result of 2 matches equals a cartesian product of rows. If both match clauses have filters, then there is a quadratic amount of filtering operations in the system.

[master < Task] PR

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

@Josipmrden Josipmrden added the feature feature label Aug 23, 2023
@Josipmrden Josipmrden added this to the mg-v2.12.0 milestone Aug 23, 2023
@Josipmrden Josipmrden self-assigned this Aug 23, 2023
@Josipmrden Josipmrden changed the title [master < TXXXX] Return cartesian product on multi match clauses [master < T1193] Return cartesian product on multi match clauses Aug 28, 2023
@gitbuda gitbuda changed the title [master < T1193] Return cartesian product on multi match clauses Return cartesian product on multi match clauses Sep 24, 2023
antepusic
antepusic requested changes Oct 18, 2023
View reviewed changes
Copy link
Contributor

@antepusic antepusic left a comment

Choose a reason for hiding this comment

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

Good job! I’ve left a few comments below:

tests/drivers/run.sh Show resolved Hide resolved
src/query/plan/pretty_print.cpp Outdated Show resolved Hide resolved
tests/unit/query_plan.cpp Show resolved Hide resolved
src/query/plan/rewrite/index_lookup.hpp Outdated Show resolved Hide resolved
src/query/plan/preprocess.cpp Outdated Show resolved Hide resolved
antoniofilipovic
antoniofilipovic requested changes Oct 19, 2023
View reviewed changes
Copy link
Collaborator

@antoniofilipovic antoniofilipovic left a comment

Choose a reason for hiding this comment

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

I checked PR and don't have many comments. Good work, left few suggestions otherwise it seems fine to me

tests/unit/query_plan_checker.hpp Outdated Show resolved Hide resolved
src/query/plan/operator.cpp Outdated Show resolved Hide resolved
src/query/plan/cost_estimator.hpp Show resolved Hide resolved
src/query/plan/preprocess.cpp Show resolved Hide resolved
src/query/plan/preprocess.cpp Show resolved Hide resolved
src/query/plan/rule_based_planner.hpp Outdated Show resolved Hide resolved
src/query/plan/preprocess.hpp Outdated Show resolved Hide resolved
antoniofilipovic
antoniofilipovic reviewed Oct 19, 2023
View reviewed changes
Copy link
Collaborator

@antoniofilipovic antoniofilipovic left a comment

Choose a reason for hiding this comment

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

I checked PR and don't have many comments. Good work, left few suggestions otherwise it seems fine to me

antepusic
antepusic requested changes Oct 23, 2023
View reviewed changes
Copy link
Contributor

@antepusic antepusic left a comment

Choose a reason for hiding this comment

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

Having investigated, the checks are failing because cartesian_product_enabled_ is turned off during unit test execution.

@Josipmrden Josipmrden added the Docs needed Docs needed label Oct 24, 2023
antepusic
antepusic reviewed Oct 24, 2023
View reviewed changes
Copy link
Contributor

@antepusic antepusic left a comment

Choose a reason for hiding this comment

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

This looks good and very close to mergeable, and I’ve resolved some of my earlier comments. Before I hit approve, please respond to the remaining two comments I had left 😄

@antepusic antepusic self-requested a review October 24, 2023 13:37
antepusic
antepusic approved these changes Oct 24, 2023
View reviewed changes
Copy link
Contributor

@antepusic antepusic left a comment

Choose a reason for hiding this comment

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

Everything looks good to me now, and the checks are now passing 🚀

antoniofilipovic
antoniofilipovic approved these changes Oct 24, 2023
View reviewed changes
Copy link
Collaborator

@antoniofilipovic antoniofilipovic 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

@Josipmrden Josipmrden mentioned this pull request Oct 24, 2023
Merged
9 tasks
@Josipmrden Josipmrden merged commit be16ca7 into master Oct 24, 2023
6 checks passed
@Josipmrden Josipmrden deleted the return-cartesian-product-on-multi-match-clauses branch October 24, 2023 19:54
as51340 pushed a commit that referenced this pull request Nov 10, 2023
@Josipmrden @as51340
Add cartesian and hash join operators (#1193)
1009042
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antepusic antepusic antepusic approved these changes

@antoniofilipovic antoniofilipovic antoniofilipovic approved these changes

Assignees

@Josipmrden Josipmrden

Labels
Docs needed Docs needed feature feature
Projects
No open projects
memgraph-v2 issues
Status: 👀 In review
Milestone
mg-v2.12.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Josipmrden @antepusic @antoniofilipovic