{{ message }}
Don't attempt to access non-existent properties#1495
Merged
Conversation
Contributor
swilly22
reviewed
Dec 17, 2020
swilly22
left a comment
swilly22
left a comment
Contributor
There was a problem hiding this comment.
CREATE (a {val:1}), (b {val: a.val})
I think it would be better to emit an error in this case, otherwise the user might think the query executed successfully and he/she will wonder about b.val
What do you think?
| self.env.assertEquals(result.properties_set, 1) | ||
|
|
||
| def test05_create_with_property_reference(self): | ||
| query = """CREATE (a), (b {val: a.val})""" |
Contributor
There was a problem hiding this comment.
Consider changing to CREATE (a {val:1}), (b {val: a.val})
jeffreylovitz
force-pushed
the
property-access-on-uncreated-entity
branch
3 times, most recently
from
511a53c to
3935591
Compare
swilly22
reviewed
Dec 22, 2020
| self.env.assertIn("undefined property", e.message) | ||
|
|
||
| # MATCH clauses should be able to use self-referential properties as existential filters. | ||
| query = """MATCH (a {age: a.age}) RETURN a.age""" |
swilly22
suggested changes
Dec 22, 2020
swilly22
left a comment
swilly22
left a comment
Contributor
There was a problem hiding this comment.
Looking good, just one request.
Comment on lines
+61
to
+70
Contributor
There was a problem hiding this comment.
When we discussed documentation I've meant adding comments within the codebase,
It doesn't look to good documenting memory leaks within the user-facing documentation, please remove this section and add inline comments.
jeffreylovitz
force-pushed
the
property-access-on-uncreated-entity
branch
from
aa1a924 to
25ca944
Compare
jeffreylovitz
force-pushed
the
property-access-on-uncreated-entity
branch
from
25ca944 to
fe573a9
Compare
swilly22
approved these changes
Dec 23, 2020
jeffreylovitz
added a commit
that referenced
this pull request
Dec 27, 2020
* Don't attempt to access non-existent properties * Address PR comments * Add additional tests * Add documentation * Skip tests that cause memory leaks * Address PR comments (cherry picked from commit 50aa868)
swilly22
added a commit
that referenced
this pull request
Dec 27, 2020
* Added QA automation support (#1483) * Added QA automation support * fixes 1 * fixes 2 (cherry picked from commit ebdbd6e) * add Graph config command to Rampfile (#1485) Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> (cherry picked from commit 0ead11e) * Refactor aggregate logic (#1468) * Refactor logic for invoking aggregate functions * WIP * Fix unit tests, stDevP logic * Fix formatting flag * reduce and evaluate aggregation functions * consolidation agg and none agg functions * Additional tests, comment corrections Co-authored-by: swilly22 <roi@redislabs.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> (cherry picked from commit aa66db8) * Replace cache per thread with global cache (#1393) * global cache * Replace the cache-per-thread with a global cache: -Use rwlock to synchronize access to the cache. -Replace the hash lookup of queries strings with rax to support safe access of multiple readers. -When cache hit occurs, do (shallow) memcpy of the query AST along with reference count increase, and store the current execution params in the new AST copy. * Fix memory leaks. Bug fix: set AST in thread local storage after the shallow copy. * Update comments in config files and number of threads in test_cache to fit the new cache design. * Defending execution plans that are being cloned from evicted by another thread, by adding ref count. * Change cache implementation so that cache item is copied within the cache get/set functions by using a callback. This ensures that cache item will not be freed when evicted from cache while it is cloned by another thread. * Change cache implementation so that cache item is copied within the cache get/set functions by using a callback. This ensures that cache item will not be freed when evicted from cache while it is cloned by another thread. * moved from ulong to long long * refactor cache unit-test * refactored cache * Refactor cache_set and test_cache. * mostly formatting * switch to new config get Co-authored-by: swilly22 <roi@redislabs.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> (cherry picked from commit 75d0a99) * Add support for startNode and endNode functions (#1490) * Add support for startNode and endNode functions * Address PR comments (cherry picked from commit 584f59b) * Disallow updating properties after deleting their entities (#1494) * Disallow updating properties after deleting their entities * Address PR comments (cherry picked from commit 92125cf) * Disallow assigning properties to complex types in MERGE SET (#1496) * Disallow assigning properties to complex types in MERGE SET constructions * Address PR comments (cherry picked from commit 6bc3552) * Consider both filters and labels in selecting a starting point (#1498) * Consider both filters and labels in selecting a starting point * refactored traverse order * Address PR comments * Update test_traversal_ordering.cpp Co-authored-by: swilly22 <roi@redislabs.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> (cherry picked from commit 818e541) * optimize expression before accessing it (#1499) * optimize expression before accessing it * algebraic expression transposed should handle directly nested transposes (cherry picked from commit 78f5310) * Don't attempt to access non-existent properties (#1495) * Don't attempt to access non-existent properties * Address PR comments * Add additional tests * Add documentation * Skip tests that cause memory leaks * Address PR comments (cherry picked from commit 50aa868) * Error on alias references in parameters (#1503) * Error on alias references in parameters * Address PR comments Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> (cherry picked from commit aed3fba) * Adding 'redis-modules-sdk' package to the README (#1509) (cherry picked from commit 2b8a3c6) * disable runtime config on enterprise (#1508) (cherry picked from commit 9bcc2d2) * bump version to 2.2.12 Co-authored-by: Rafi Einstein <raffapen@outlook.com> Co-authored-by: alonre24 <alonreshef24@gmail.com> Co-authored-by: Roi Lipman <swilly22@users.noreply.github.com> Co-authored-by: Dani Tseiltin <39850374+danitseitlin@users.noreply.github.com>
pnxguide
pushed a commit
to CMU-SPEED/RedisGraph
that referenced
this pull request
Mar 22, 2023
* Don't attempt to access non-existent properties * Address PR comments * Add additional tests * Add documentation * Skip tests that cause memory leaks * Address PR comments
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR resolves a bug in which the server crashes on attempting to access a property that is not yet created:
Unfortunately, with this fix the above query will still only set 1 property. While to my knowledge not formalized in Cypher, by convention both nodes should have
valset to2by this query. That would require considerable redesigning, however, and the value add seems minimal.This PR resolves #1492.