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

Conditional execution procedures #203

Merged
merged 17 commits into from Apr 5, 2023

Conversation

antepusic
Copy link
Collaborator

@antepusic antepusic commented Mar 28, 2023

Description

This PR adds a query module that allows conditional execution of read-only procedures.

Pull request type

  • Algorithm/Module

######################################

Reviewer checklist (the reviewer checks this part)

Module/Algorithm

  • Core algorithm/module implementation
  • Query module implementation
  • Unit tests
  • End-to-end tests
  • Code documentation
  • README short description
  • Documentation on memgraph/docs

######################################

@antepusic antepusic added type: enhancement lang: python Issue on Python codebase status: ready PR is ready for review labels Mar 28, 2023
@antepusic antepusic self-assigned this Mar 28, 2023
python/do.py Outdated Show resolved Hide resolved
python/do.py Outdated Show resolved Hide resolved
python/do.py Outdated Show resolved Hide resolved
python/do.py Show resolved Hide resolved
python/do.py Outdated Show resolved Hide resolved
python/do.py Show resolved Hide resolved
python/do.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@as51340 as51340 left a comment

Choose a reason for hiding this comment

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

Nice work, very clean solution I really don't see many things that could be improved. However, some e2e tests are failing so you chekc this + please mention the comment about built, Python and gqla on the daily, we should discuss that.

python/do.py Show resolved Hide resolved
@as51340
Copy link
Collaborator

as51340 commented Mar 29, 2023

I think it would be good if you can provide some real queries for conditions not just true/false so we have "real" e2e example, e.g match on some person check some condition and then do something with it.

python/do.py Show resolved Hide resolved
@as51340
Copy link
Collaborator

as51340 commented Mar 29, 2023

Also, I think it would be good to test the behavior when there are no output values from do.case and do.when, e.g you user enter do.case and provides some query which doesn't yield any values so those empty values are sent into the other module/query and shouldn't do anything because there are none of them.

@as51340
Copy link
Collaborator

as51340 commented Mar 29, 2023

Also, can you the behavior of checking the string property value, when it has quotes?

Like this:

MATCH (n) WHERE n.id = "hello" RETURN n;

@antepusic
Copy link
Collaborator Author

antepusic commented Mar 30, 2023

@as51340:

#203 (review)

Will do. The check only failed because igraph e2e tests don’t make sure the database is empty - I’ll update the e2e script to do that before each test.

#203 (comment)

No problem - I only omitted such tests because evaluating the condition to true/false is the query engine’s job.

#203 (comment)

Good idea, I’ll add a test 👍

#203 (comment)

Do you mean strings that contain quote marks inside them?

@as51340
Copy link
Collaborator

as51340 commented Mar 30, 2023

Do you mean strings that contain quote marks inside them?

yes

@antepusic antepusic requested a review from as51340 March 30, 2023 16:09
python/do.py Outdated
Comment on lines 62 to 70
return [
mgp.Record(
value={
field_name: _gqlalchemy_type_to_mgp(ctx.graph, field_value)
for field_name, field_value in result.items()
}
)
for result in results
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not also _convert_results?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, thanks!



def is_global_operation(subquery: str) -> bool:
return any(subquery.startswith(pattern) for pattern in QUERY_PATTERNS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should support also lower case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added

@as51340
Copy link
Collaborator

as51340 commented Mar 30, 2023

I would like to add one more thing if you think it makes sense and that is testing whole vertex and relationship objects return result, not just size of the returned results so we are sure vertex and relationships are correctly returned from gqla to mage.

@antepusic
Copy link
Collaborator Author

@as51340:

#203 (comment)

This makes sense, added one test each for nodes, relationships and paths that make use of their peculiarities (node properties, type() for relationships and nodes() for paths).

@as51340 as51340 self-requested a review April 5, 2023 05:51
@antoniofilipovic antoniofilipovic merged commit 6c52aca into main Apr 5, 2023
4 checks passed
@antoniofilipovic antoniofilipovic added this to the 1.7.0 milestone Apr 5, 2023
@damianbenente
Copy link

[BUG] Failed do.when() when using database with authentication
#214

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang: python Issue on Python codebase status: ready PR is ready for review
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants