Skip to content

Conversation

@jrgemignani
Copy link
Contributor

@jrgemignani jrgemignani commented Mar 16, 2023

Fixed an issue where an already resolved variable, when used for
a property constraint, errored out. See #701 for more details.

This also fixed a regression test that was erroring out, but was
overlooked.

Adjusted and added additional regression tests.

Co-authored-by: Dehowe Feng dehowefeng@gmail.com

Fixed an issue where an already resolved variable, when used for
a property constraint, errored out. See apache#724 for more details.

This also fixed a regression test that was erroring out, but was
overlooked.

Adjusted and added additional regression tests.

Co-authored-by: Dehowe Feng <dehowefeng@gmail.com>
@dehowef
Copy link
Member

dehowef commented Mar 16, 2023

John and I worked closely on this issue to resolve the limitations of the match clause in reflexive pattern matching. If anyone wants to take a look and review what was done, it would be much appreciated

@MuhammadTahaNaveed
Copy link
Member

MuhammadTahaNaveed commented Mar 16, 2023

Hi @dehowef @jrgemignani,

I was wondering why the query mentioned in issue still errors out.

SELECT * from cypher('my_graph_name', $$
  MATCH (a {prop: “val”})-[]- (a {prop2: “val2”})
$$) as (a agtype);

I tried this query in context of regression tests

SELECT * FROM cypher('cypher_match', $$
	MATCH (a {i: 1})-[]-(a {j: 2}) RETURN a $$) as (a agtype);
ERROR:  variable a already exists
LINE 2:  MATCH (a {i: 1})-[]-(a {j: 2}) RETURN a $$) as (a agtype);

@jrgemignani
Copy link
Contributor Author

jrgemignani commented Mar 16, 2023

@MuhammadTahaNaveed Actually, that's a good point and a bit of an oversight on my part. My PR only addresses the clause to clause previously resolved variables, not within the same clause. For example -

MATCH (u) MATCH (u {name: "Jim"}) MATCH (u {age: 35}) RETURN u

I will try to add the other in tomorrow and update this PR.

@dehowef
Copy link
Member

dehowef commented Mar 16, 2023

@MuhammadTahaNaveed The issues with MATCH occur in several sections of the code-- we have chosen to address them incrementally for maximum transparency and visibility for the project.

I feel like this method has several advantages, one being that each commit will be itemized so that in the event that something is updated, we can go back to the commit where a certain change was implemented to easily view and address it. Thanks!

@jrgemignani
Copy link
Contributor Author

@dehowef @MuhammadTahaNaveed I'm reviewing my patch to make sure it covers all the cases adequately.

@jrgemignani jrgemignani changed the title Fix property constraints against resolved variables (#724) Fix property constraints against resolved variables (#701) Mar 16, 2023
@jrgemignani
Copy link
Contributor Author

@dehowef @MuhammadTahaNaveed I have updated the PR to point to the correct issue this PR resolves. If you can verify that it works against that issue? Sorry, for the confusion.

@dehowef dehowef merged commit c596757 into apache:master Mar 17, 2023
jrgemignani added a commit that referenced this pull request Mar 20, 2023
…CH clauses(#701) (#747)

Fixed an issue where an already resolved variable, when used for
a property constraint in a second MATCH clause, errored out.
See #701 and #724  for more details. This is part of a series of
PRs done to refactor MATCH clause behavior.

This also fixed a regression test that was erroring out, but was
overlooked.

Adjusted and added additional regression tests.

Co-authored-by: Dehowe Feng <dehowefeng@gmail.com>
jrgemignani added a commit that referenced this pull request Mar 24, 2023
…CH clauses(#701) (#747)

Fixed an issue where an already resolved variable, when used for
a property constraint in a second MATCH clause, errored out.
See #701 and #724  for more details. This is part of a series of
PRs done to refactor MATCH clause behavior.

This also fixed a regression test that was erroring out, but was
overlooked.

Adjusted and added additional regression tests.

Co-authored-by: Dehowe Feng <dehowefeng@gmail.com>
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.

3 participants