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

CREATE should not access its own unbounded entities #488

Merged
merged 7 commits into from Nov 1, 2023

Conversation

swilly22
Copy link
Contributor

@swilly22 swilly22 commented Nov 1, 2023

Resolves #425
The CREATE clause is not supposed to "see" its own newly created entities
e.g. CREATE (a {v:e.v+1})-[e:R {v:2}]->() CREATE (a {v:b.v}), (b {v:a.v})

@swilly22 swilly22 requested a review from AviAvni November 1, 2023 09:56
(
const cypher_astnode_t *root // root of AST
) {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we use this or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems like we don't

// 3. SET
// 4. DELETE
// 5. REMOVE
// 2. SET
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we need to introduce an optimization that merge this if we can?

Copy link
Contributor Author

@swilly22 swilly22 Nov 1, 2023

Choose a reason for hiding this comment

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

There might be situations where we might be able to merge, but the concept of separated CREATE clauses is there for a semantic reasons. users that know what they are doing create this separation on purpose.

@@ -916,26 +953,6 @@ static VISITOR_STRATEGY _Validate_shortest_path
return VISITOR_RECURSE;
}

// validate a pattern-path expression
static VISITOR_STRATEGY _Validate_pattern_path
Copy link
Contributor

Choose a reason for hiding this comment

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

why this was deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this function only served the CREATE clause, which now explicitly calls _Validate_CREATE_Entities
by removing this we get the default behavior which simply recurse, so no real change.

@@ -1566,21 +1582,135 @@ static VISITOR_STRATEGY _Validate_UNION_Clause
return VISITOR_RECURSE;
}

// qsort comparison function for strings
static int _compareStrings
Copy link
Contributor

Choose a reason for hiding this comment

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

it look like you don't use this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're correct, no longer in use.

redis_graph.query(query)
self.env.assertTrue(False)
except redis.exceptions.ResponseError as e:
self.env.assertContains("Variable `r` already declared", str(e))
Copy link
Contributor

Choose a reason for hiding this comment

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

why different error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case (a single CREATE clause) the validation first spots the fact that r is already defined.
in the other cases multiple CREATE clauses r is bounded.

@swilly22 swilly22 merged commit d8f20c0 into master Nov 1, 2023
8 checks passed
@swilly22 swilly22 deleted the multiple-create branch November 1, 2023 20:35
@swilly22 swilly22 changed the title Multiple create CREATE should not access its own unbounded entities Nov 2, 2023
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.

Query crashes in DataBlock_ItemIsDeleted
2 participants