Skip to content

Commit

Permalink
Fix Issue 945 - incorrect count(*) return values (#1288) (#1312)
Browse files Browse the repository at this point in the history
Fixed issue 945 where count(*) would have incorrect return values.

NOTE -

We need to re-evaluate if we want to use output_node or not.
If output_node is set to false, then it basically short circuits
match for instances where a variable isn't specified -

    MATCH () RETURN 0;
    MATCH () MATCH () RETURN 0;

While, on the surface, this appears to be a good way to improve
execution time of commands that won't do anything, it also
causes chained commands to not work correctly. This is because
a match without a variable will still feed its tuples to the next
stage(s) even though they won't necessarily be sent to the output.
For example, with count(*) -

    MATCH () RETURN count(*);
    MATCH () MATCH RETURN count(*);

With output_node set to false, it won't send the tuples. We will
likely need to remove all of the output_node logic. However, this
needs to be reviewed. For now, we just set it to true and update
the output of the regression tests.

Updated regression tests to accomodate the change.
Added new regression tests to cover overlooked cases.
  • Loading branch information
jrgemignani committed Oct 27, 2023
1 parent dc82c93 commit 162bf30
Show file tree
Hide file tree
Showing 6 changed files with 312 additions and 46 deletions.
2 changes: 1 addition & 1 deletion regress/expected/cypher_delete.out
Original file line number Diff line number Diff line change
Expand Up @@ -707,8 +707,8 @@ SELECT * FROM cypher('detach_delete', $$ MATCH ()-[e]->() RETURN e.name $$) as (
-------
"ab"
"cd"
"nm"
"am"
"nm"
"pq"
(5 rows)

Expand Down
282 changes: 247 additions & 35 deletions regress/expected/cypher_match.out

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion regress/expected/cypher_set.out
Original file line number Diff line number Diff line change
Expand Up @@ -167,8 +167,8 @@ SELECT * FROM cypher('cypher_set', $$MATCH ()-[n]->(:other_v) RETURN n$$) AS (a
----------------------------------------------------------------------------------------------------------------------------------------
{"id": 1125899906842625, "label": "e", "end_id": 1407374883553281, "start_id": 281474976710657, "properties": {"i": 3, "j": 20}}::edge
{"id": 1125899906842626, "label": "e", "end_id": 1407374883553282, "start_id": 844424930131969, "properties": {"i": 3, "j": 20}}::edge
{"id": 1125899906842627, "label": "e", "end_id": 1407374883553283, "start_id": 844424930131971, "properties": {"i": 3, "j": 20}}::edge
{"id": 1125899906842628, "label": "e", "end_id": 1407374883553284, "start_id": 844424930131970, "properties": {"i": 3, "j": 20}}::edge
{"id": 1125899906842627, "label": "e", "end_id": 1407374883553283, "start_id": 844424930131971, "properties": {"i": 3, "j": 20}}::edge
(4 rows)

SELECT * FROM cypher('cypher_set', $$
Expand Down
16 changes: 8 additions & 8 deletions regress/expected/expr.out
Original file line number Diff line number Diff line change
Expand Up @@ -2230,10 +2230,10 @@ SELECT * FROM cypher('expr', $$ MATCH (v) RETURN v $$) AS (expression agtype);
SELECT * FROM cypher('expr', $$ MATCH ()-[e]-() RETURN e $$) AS (expression agtype);
expression
---------------------------------------------------------------------------------------------------------------------------
{"id": 1407374883553282, "label": "e1", "end_id": 1125899906842626, "start_id": 1125899906842625, "properties": {}}::edge
{"id": 1407374883553281, "label": "e1", "end_id": 1125899906842627, "start_id": 1125899906842626, "properties": {}}::edge
{"id": 1407374883553282, "label": "e1", "end_id": 1125899906842626, "start_id": 1125899906842625, "properties": {}}::edge
{"id": 1407374883553281, "label": "e1", "end_id": 1125899906842627, "start_id": 1125899906842626, "properties": {}}::edge
{"id": 1407374883553282, "label": "e1", "end_id": 1125899906842626, "start_id": 1125899906842625, "properties": {}}::edge
{"id": 1407374883553282, "label": "e1", "end_id": 1125899906842626, "start_id": 1125899906842625, "properties": {}}::edge
(4 rows)

-- id()
Expand All @@ -2242,10 +2242,10 @@ SELECT * FROM cypher('expr', $$
$$) AS (id agtype);
id
------------------
1407374883553282
1407374883553281
1407374883553282
1407374883553281
1407374883553282
1407374883553282
(4 rows)

SELECT * FROM cypher('expr', $$
Expand Down Expand Up @@ -2284,10 +2284,10 @@ SELECT * FROM cypher('expr', $$
$$) AS (start_id agtype);
start_id
------------------
1125899906842625
1125899906842626
1125899906842625
1125899906842626
1125899906842625
1125899906842625
(4 rows)

-- should return null
Expand Down Expand Up @@ -2317,10 +2317,10 @@ SELECT * FROM cypher('expr', $$
$$) AS (end_id agtype);
end_id
------------------
1125899906842626
1125899906842627
1125899906842626
1125899906842627
1125899906842626
1125899906842626
(4 rows)

-- should return null
Expand Down
38 changes: 38 additions & 0 deletions regress/sql/cypher_match.sql
Original file line number Diff line number Diff line change
Expand Up @@ -1132,12 +1132,50 @@ SELECT count(*) FROM cypher('test_enable_containment', $$ MATCH (x:Customer {pho
SELECT count(*) FROM cypher('test_enable_containment', $$ MATCH (x:Customer {phone:[654765876]}) RETURN x $$) as (a agtype);
SELECT * FROM cypher('test_enable_containment', $$ EXPLAIN (COSTS OFF) MATCH (x:Customer {school:{name:'XYZ',program:{degree:'BSc'}},phone:[987654321],parents:{}}) RETURN x $$) as (a agtype);

--
-- Issue 945
--
SELECT create_graph('issue_945');
SELECT * FROM cypher('issue_945', $$
CREATE (a:Part {part_num: '123'}),
(b:Part {part_num: '345'}),
(c:Part {part_num: '456'}),
(d:Part {part_num: '789'})
$$) as (result agtype);

-- should match 4
SELECT * FROM cypher('issue_945', $$
MATCH (a:Part) RETURN a
$$) as (result agtype);

-- each should return 4
SELECT * FROM cypher('issue_945', $$ MATCH (:Part) RETURN count(*) $$) as (result agtype);
SELECT * FROM cypher('issue_945', $$ MATCH (a:Part) RETURN count(*) $$) as (result agtype);

-- each should return 4 rows of 0
SELECT * FROM cypher('issue_945', $$ MATCH (:Part) RETURN 0 $$) as (result agtype);
SELECT * FROM cypher('issue_945', $$ MATCH (a:Part) RETURN 0 $$) as (result agtype);

-- each should return 16 rows of 0
SELECT * FROM cypher('issue_945', $$ MATCH (:Part) MATCH (:Part) RETURN 0 $$) as (result agtype);
SELECT * FROM cypher('issue_945', $$ MATCH (a:Part) MATCH (:Part) RETURN 0 $$) as (result agtype);
SELECT * FROM cypher('issue_945', $$ MATCH (:Part) MATCH (b:Part) RETURN 0 $$) as (result agtype);
SELECT * FROM cypher('issue_945', $$ MATCH (a:Part) MATCH (b:Part) RETURN 0 $$) as (result agtype);

-- each should return a count of 16
SELECT * FROM cypher('issue_945', $$ MATCH (:Part) MATCH (:Part) RETURN count(*) $$) as (result agtype);
SELECT * FROM cypher('issue_945', $$ MATCH (a:Part) MATCH (:Part) RETURN count(*) $$) as (result agtype);
SELECT * FROM cypher('issue_945', $$ MATCH (:Part) MATCH (b:Part) RETURN count(*) $$) as (result agtype);
SELECT * FROM cypher('issue_945', $$ MATCH (a:Part) MATCH (b:Part) RETURN count(*) $$) as (result agtype);


--
-- Clean up
--
SELECT drop_graph('cypher_match', true);
SELECT drop_graph('test_retrieve_var', true);
SELECT drop_graph('test_enable_containment', true);
SELECT drop_graph('issue_945', true);

--
-- End
Expand Down
18 changes: 17 additions & 1 deletion src/backend/parser/cypher_clause.c
Original file line number Diff line number Diff line change
Expand Up @@ -4078,7 +4078,7 @@ static List *transform_match_entities(cypher_parsestate *cpstate, Query *query,
node->name),
parser_errposition(pstate, node->location)));
}

/*
* Checks the previous clauses to see if the variable already
* exists.
Expand All @@ -4095,6 +4095,22 @@ static List *transform_match_entities(cypher_parsestate *cpstate, Query *query,
output_node = (special_VLE_case && !node->name && !node->props) ?
false :
INCLUDE_NODE_IN_JOIN_TREE(path, node);
/*
* TODO
*
* We need to re-evaluate if we want to use output_node or not.
* If output_node is set to false, then it basically short circuits
* the match for instances where a variable isn't specified. While,
* on the surface, this appears to be a good way to improve
* execution time of commands that won't do anything, it also
* causes chained commands to not work correctly. This is because
* a match without a variable will still feed its tuples to the next
* stage(s). With this set to false, it won't. So we likely need to
* remove all of the output_node logic. This needs to be reviewed,
* though. For now, we just set it to true and update the output of
* the regression tests.
*/
output_node = true;

/* transform vertex */
expr = transform_cypher_node(cpstate, node, &query->targetList,
Expand Down

0 comments on commit 162bf30

Please sign in to comment.