Skip to content

Fixes for zero-length traversals #1504

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

Merged
merged 4 commits into from
Dec 25, 2020
Merged

Fixes for zero-length traversals #1504

merged 4 commits into from
Dec 25, 2020

Conversation

jeffreylovitz
Copy link
Contributor

This PR fixes 2 misbehaviors:
The query:

MATCH (a)-[]-(:L{})<-[*0]-(b:L) RETURN a

Caused the server to crash.

The above query produced a different execution plan than the logically equivalent variant:

MATCH (a)-[]-(:L)<-[*0]-(b:L) RETURN a

(The second query does not have the empty property map.)

However, there are still problems evaluating 0-hop traversals. If the intermediate node does have filters, as in:

CREATE (:L {v:1})-[:E]->(:L {v: 2})<-[:E]-(:L {v: 3})
MATCH (a)-[]-(:L{v: 2})<-[*0]-(b:L) RETURN a

No results will be returned.

The difference can be seen in the execution plans:

redis-cli GRAPH.EXPLAIN G "MATCH (a)-[]-(:L)<-[*0]-(b:L) RETURN a"
1) "Results"
2) "    Project"
3) "        Conditional Traverse | (b:L)->(a)"
4) "            Node By Label Scan | (b:L)"
redis-cli GRAPH.EXPLAIN G "MATCH (a)-[]-(:L{v: 2})<-[*0]-(b:L) RETURN a"
1) "Results"
2) "    Project"
3) "        Conditional Traverse | (anon_1:L)->(a)"
4) "            Conditional Traverse | (b:L)->(b:L)"
5) "                Filter"
6) "                    Node By Label Scan | (anon_1:L)"

@jeffreylovitz jeffreylovitz self-assigned this Dec 21, 2020
Copy link
Contributor

@swilly22 swilly22 left a comment

Choose a reason for hiding this comment

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

Great addition, zero hop is confusing and easily can lead into issues, one question request.

@@ -60,6 +60,8 @@ static void _AST_MapOrderByReferences(AST *ast, const cypher_astnode_t *order_by
static void _AST_MapReferencedNode(AST *ast, const cypher_astnode_t *node, bool force_mapping) {

const cypher_astnode_t *properties = cypher_ast_node_pattern_get_properties(node);
// Disregard empty property maps.
if(properties && cypher_astnode_nchildren(properties) == 0) properties = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

Comment on lines 357 to 358
while(current->type == AL_OPERATION &&
!(current->operation.op == AL_EXP_TRANSPOSE && FIRST_CHILD(current)->type == AL_OPERAND)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why this loop condition was changed?
I would really like to keep the original version as it is straightforward and easy to reason about

@jeffreylovitz jeffreylovitz force-pushed the zero-len-traverse-fixes branch from b8eb1c2 to 2dada7a Compare December 22, 2020 16:57
@swilly22 swilly22 force-pushed the zero-len-traverse-fixes branch from b65ef28 to ed02874 Compare December 24, 2020 14:58
@swilly22 swilly22 merged commit b3ab565 into master Dec 25, 2020
@swilly22 swilly22 deleted the zero-len-traverse-fixes branch December 25, 2020 06:53
jeffreylovitz added a commit that referenced this pull request Jan 20, 2021
* Fixes for zero-length traversals

* Address PR comments

* handle removal of nested algebraic operand

Co-authored-by: Roi Lipman <[email protected]>
Co-authored-by: swilly22 <[email protected]>
(cherry picked from commit b3ab565)
swilly22 added a commit that referenced this pull request Jan 20, 2021
* Updating 'redis-modules-sdk' inside README and adding to clients.md (#1515)

* Update README.md

* Update clients.md

(cherry picked from commit 7c19f2d)

* list indices (#1519)

* list indices

* Modifications to list indexes procedure

* order db.indexes when testing its output

Co-authored-by: Jeffrey Lovitz <[email protected]>
(cherry picked from commit 7961d5a)

* adding redisinsight to our docs (#1521)

* adding redisinsight to our docs

* fix typo

(cherry picked from commit a03a79f)

* Fixes for zero-length traversals (#1504)

* Fixes for zero-length traversals

* Address PR comments

* handle removal of nested algebraic operand

Co-authored-by: Roi Lipman <[email protected]>
Co-authored-by: swilly22 <[email protected]>
(cherry picked from commit b3ab565)

* Recurse into other operations when checking for transpose ops (#1523)

* Recurse into other operations when checking for transpose ops

* check if operand is transposed

Co-authored-by: swilly22 <[email protected]>
(cherry picked from commit d62b7a2)

* Persist Record values before storing them in OpCreate (#1524)

* Persist Record values before storing them in OpCreate

* Address PR comments

Co-authored-by: Roi Lipman <[email protected]>
(cherry picked from commit dc19665)

* Python 3 tests (#1525)

* Post-rebase fixes

* Switch automation to Python 3

* Fix CI locale

* Fix path tests

* Explicitly decode RLTest responses

(cherry picked from commit 9a45717)

* Ensure label data is up-to-date when initializing index scans (#1534)

Co-authored-by: Roi Lipman <[email protected]>
(cherry picked from commit 4a1cec5)

* Don't attempt to normalize IN filters when utilizing indexes (#1532)

(cherry picked from commit b943a14)

* bump version to 2.2.13

Co-authored-by: Dani Tseiltin <[email protected]>
Co-authored-by: Roi Lipman <[email protected]>
Co-authored-by: Pieter Cailliau <[email protected]>
pnxguide pushed a commit to CMU-SPEED/RedisGraph that referenced this pull request Mar 22, 2023
* Fixes for zero-length traversals

* Address PR comments

* handle removal of nested algebraic operand

Co-authored-by: Roi Lipman <[email protected]>
Co-authored-by: swilly22 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants