You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@age.apache.org by "jrgemignani (via GitHub)" <gi...@apache.org> on 2023/06/02 21:56:26 UTC
[GitHub] [age] jrgemignani opened a new pull request, #961: Fix complex MERGE causes crash (#897)
jrgemignani opened a new pull request, #961:
URL: https://github.com/apache/age/pull/961
Fixed the complex MERGE crash, which was due to storing variables in tuple positions that did not exist. This happened on MERGE commands where there wasn't a RETURN clause.
Added logic to catch these and handle appropriately. Meaning, discarding the variable results, instead of storing them.
Fixing this issue has highlighted the need to fix variable reuse in the MERGE transform itself. This is because MERGE preprocesses the path before handing it off to anything else.
Added regression tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [age] jrgemignani commented on a diff in pull request #961: Fix complex MERGE causes crash (#897)
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on code in PR #961:
URL: https://github.com/apache/age/pull/961#discussion_r1218418120
##########
regress/sql/cypher_merge.sql:
##########
@@ -479,6 +479,22 @@ SELECT * FROM cypher('cypher_merge', $$ MATCH (n:node) RETURN n $$) AS (n agtype
SELECT * FROM cypher('cypher_merge', $$ MERGE (n:node {name: 'Jason'}) SET n.name = 'Lisa', n.age = 23, n.gender = 'Female' RETURN n $$) AS (n agtype);
SELECT * FROM cypher('cypher_merge', $$ MATCH (n:node) RETURN n $$) AS (n agtype);
+--
+-- Complex MERGE w/wo RETURN values
+--
+-- These should each create a path, if it doesn't already exist.
+-- TODO Until the issue with variable reuse of 'x' in MERGE is corrected,
+-- these commands will each create a new path.
+SELECT * FROM cypher('cypher_merge', $$ MERGE ()-[:B]->(x:C)-[:E]->(x:C)<-[f:F]-(y:I) $$) AS (x agtype);
+SELECT * FROM cypher('cypher_merge', $$ MERGE ()-[:B]->(x:C)-[:E]->(x:C)<-[f:F]-(y:I) RETURN x $$) AS (x agtype);
Review Comment:
I noticed that message when testing against Neo4j as well.
Neo4j is complaining because x is already defined and it looks like we are redefining it. But are we really redefining it? It is the same variable, label, and property. I only qualified it here because MERGE has additional logic that preprocesses the command that hasn't been fixed yet. Once that is done, you will be able to just use x in the second spot, and that is legal in Neo4j.
This patch is only for fixing the memory issues that caused the crash. And, we still need to address the issue that x, as the logic currently is, is being recreated.
Hope that is helpful
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [age] jrgemignani commented on a diff in pull request #961: Fix complex MERGE causes crash (#897)
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on code in PR #961:
URL: https://github.com/apache/age/pull/961#discussion_r1218439131
##########
regress/sql/cypher_merge.sql:
##########
@@ -479,6 +479,22 @@ SELECT * FROM cypher('cypher_merge', $$ MATCH (n:node) RETURN n $$) AS (n agtype
SELECT * FROM cypher('cypher_merge', $$ MERGE (n:node {name: 'Jason'}) SET n.name = 'Lisa', n.age = 23, n.gender = 'Female' RETURN n $$) AS (n agtype);
SELECT * FROM cypher('cypher_merge', $$ MATCH (n:node) RETURN n $$) AS (n agtype);
+--
+-- Complex MERGE w/wo RETURN values
+--
+-- These should each create a path, if it doesn't already exist.
+-- TODO Until the issue with variable reuse of 'x' in MERGE is corrected,
+-- these commands will each create a new path.
+SELECT * FROM cypher('cypher_merge', $$ MERGE ()-[:B]->(x:C)-[:E]->(x:C)<-[f:F]-(y:I) $$) AS (x agtype);
+SELECT * FROM cypher('cypher_merge', $$ MERGE ()-[:B]->(x:C)-[:E]->(x:C)<-[f:F]-(y:I) RETURN x $$) AS (x agtype);
Review Comment:
It was good to bring it up because it highlights that MERGE is not done yet. We still have work to do with it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [age] MuhammadTahaNaveed commented on a diff in pull request #961: Fix complex MERGE causes crash (#897)
Posted by "MuhammadTahaNaveed (via GitHub)" <gi...@apache.org>.
MuhammadTahaNaveed commented on code in PR #961:
URL: https://github.com/apache/age/pull/961#discussion_r1216853373
##########
regress/sql/cypher_merge.sql:
##########
@@ -479,6 +479,22 @@ SELECT * FROM cypher('cypher_merge', $$ MATCH (n:node) RETURN n $$) AS (n agtype
SELECT * FROM cypher('cypher_merge', $$ MERGE (n:node {name: 'Jason'}) SET n.name = 'Lisa', n.age = 23, n.gender = 'Female' RETURN n $$) AS (n agtype);
SELECT * FROM cypher('cypher_merge', $$ MATCH (n:node) RETURN n $$) AS (n agtype);
+--
+-- Complex MERGE w/wo RETURN values
+--
+-- These should each create a path, if it doesn't already exist.
+-- TODO Until the issue with variable reuse of 'x' in MERGE is corrected,
+-- these commands will each create a new path.
+SELECT * FROM cypher('cypher_merge', $$ MERGE ()-[:B]->(x:C)-[:E]->(x:C)<-[f:F]-(y:I) $$) AS (x agtype);
+SELECT * FROM cypher('cypher_merge', $$ MERGE ()-[:B]->(x:C)-[:E]->(x:C)<-[f:F]-(y:I) RETURN x $$) AS (x agtype);
Review Comment:
These queries error out in neo4j saying `Can't create node x with labels or properties here.` pointing towards second reference of x. Shoudn't these fail in age as well ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [age] MuhammadTahaNaveed commented on a diff in pull request #961: Fix complex MERGE causes crash (#897)
Posted by "MuhammadTahaNaveed (via GitHub)" <gi...@apache.org>.
MuhammadTahaNaveed commented on code in PR #961:
URL: https://github.com/apache/age/pull/961#discussion_r1218441044
##########
regress/sql/cypher_merge.sql:
##########
@@ -479,6 +479,22 @@ SELECT * FROM cypher('cypher_merge', $$ MATCH (n:node) RETURN n $$) AS (n agtype
SELECT * FROM cypher('cypher_merge', $$ MERGE (n:node {name: 'Jason'}) SET n.name = 'Lisa', n.age = 23, n.gender = 'Female' RETURN n $$) AS (n agtype);
SELECT * FROM cypher('cypher_merge', $$ MATCH (n:node) RETURN n $$) AS (n agtype);
+--
+-- Complex MERGE w/wo RETURN values
+--
+-- These should each create a path, if it doesn't already exist.
+-- TODO Until the issue with variable reuse of 'x' in MERGE is corrected,
+-- these commands will each create a new path.
+SELECT * FROM cypher('cypher_merge', $$ MERGE ()-[:B]->(x:C)-[:E]->(x:C)<-[f:F]-(y:I) $$) AS (x agtype);
+SELECT * FROM cypher('cypher_merge', $$ MERGE ()-[:B]->(x:C)-[:E]->(x:C)<-[f:F]-(y:I) RETURN x $$) AS (x agtype);
Review Comment:
Yup I knew that but was confused about the constraints on merge....
Clear on that now..
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [age] dehowef commented on pull request #961: Fix complex MERGE causes crash (#897)
Posted by "dehowef (via GitHub)" <gi...@apache.org>.
dehowef commented on PR #961:
URL: https://github.com/apache/age/pull/961#issuecomment-1574393439
I looked over it, and good job preventing the crash. If @muhammadshoaib could take a look too, that'd be great
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [age] jrgemignani commented on a diff in pull request #961: Fix complex MERGE causes crash (#897)
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on code in PR #961:
URL: https://github.com/apache/age/pull/961#discussion_r1218418120
##########
regress/sql/cypher_merge.sql:
##########
@@ -479,6 +479,22 @@ SELECT * FROM cypher('cypher_merge', $$ MATCH (n:node) RETURN n $$) AS (n agtype
SELECT * FROM cypher('cypher_merge', $$ MERGE (n:node {name: 'Jason'}) SET n.name = 'Lisa', n.age = 23, n.gender = 'Female' RETURN n $$) AS (n agtype);
SELECT * FROM cypher('cypher_merge', $$ MATCH (n:node) RETURN n $$) AS (n agtype);
+--
+-- Complex MERGE w/wo RETURN values
+--
+-- These should each create a path, if it doesn't already exist.
+-- TODO Until the issue with variable reuse of 'x' in MERGE is corrected,
+-- these commands will each create a new path.
+SELECT * FROM cypher('cypher_merge', $$ MERGE ()-[:B]->(x:C)-[:E]->(x:C)<-[f:F]-(y:I) $$) AS (x agtype);
+SELECT * FROM cypher('cypher_merge', $$ MERGE ()-[:B]->(x:C)-[:E]->(x:C)<-[f:F]-(y:I) RETURN x $$) AS (x agtype);
Review Comment:
I noticed that message when testing against Neo4j as well.
Neo4j is complaining because x is already defined and it looks like we are redefining it. But are we really redefining it? It is the same variable, label, and property. I only qualified it here because MERGE has additional logic that preprocesses the command that hasn't been fixed yet. Once that is done, you will be able to just use x in the second spot, and that is legal in Neo4j.
This patch is only for fixing the memory issues that caused the crash. And, we still need to address the issue that x, as the logic currently is, is being recreated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [age] jrgemignani commented on pull request #961: Fix complex MERGE causes crash (#897)
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on PR #961:
URL: https://github.com/apache/age/pull/961#issuecomment-1574399560
Added @MuhammadTahaNaveed as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [age] dehowef merged pull request #961: Fix complex MERGE causes crash (#897)
Posted by "dehowef (via GitHub)" <gi...@apache.org>.
dehowef merged PR #961:
URL: https://github.com/apache/age/pull/961
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [age] MuhammadTahaNaveed commented on a diff in pull request #961: Fix complex MERGE causes crash (#897)
Posted by "MuhammadTahaNaveed (via GitHub)" <gi...@apache.org>.
MuhammadTahaNaveed commented on code in PR #961:
URL: https://github.com/apache/age/pull/961#discussion_r1218437112
##########
regress/sql/cypher_merge.sql:
##########
@@ -479,6 +479,22 @@ SELECT * FROM cypher('cypher_merge', $$ MATCH (n:node) RETURN n $$) AS (n agtype
SELECT * FROM cypher('cypher_merge', $$ MERGE (n:node {name: 'Jason'}) SET n.name = 'Lisa', n.age = 23, n.gender = 'Female' RETURN n $$) AS (n agtype);
SELECT * FROM cypher('cypher_merge', $$ MATCH (n:node) RETURN n $$) AS (n agtype);
+--
+-- Complex MERGE w/wo RETURN values
+--
+-- These should each create a path, if it doesn't already exist.
+-- TODO Until the issue with variable reuse of 'x' in MERGE is corrected,
+-- these commands will each create a new path.
+SELECT * FROM cypher('cypher_merge', $$ MERGE ()-[:B]->(x:C)-[:E]->(x:C)<-[f:F]-(y:I) $$) AS (x agtype);
+SELECT * FROM cypher('cypher_merge', $$ MERGE ()-[:B]->(x:C)-[:E]->(x:C)<-[f:F]-(y:I) RETURN x $$) AS (x agtype);
Review Comment:
okay thanks... my confusion is clear on this thing now...
Marking it as resolved...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@age.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org