You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@age.apache.org by "moeed-k (via GitHub)" <gi...@apache.org> on 2023/01/23 01:14:55 UTC
[GitHub] [age] moeed-k opened a new pull request, #586: Optimize age_exists function
moeed-k opened a new pull request, #586:
URL: https://github.com/apache/age/pull/586
-age_exists, which is the executor for exists(property) function, was making up to 3 redundant memory accesses. -exists(property) predicate function only serves to check if a property exists or not. -In Cypher, if a property is assigned the value of NULL, this is considered the same as the property not existing at all. Thus the function calls to get the value type is not needed as the check for the null argument itself filters out all NULL inputs. If a property passes this check, it implies existence.
--
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] moeed-k commented on pull request #586: Optimize age_exists function
Posted by "moeed-k (via GitHub)" <gi...@apache.org>.
moeed-k commented on PR #586:
URL: https://github.com/apache/age/pull/586#issuecomment-1403032865
> @moeed-k Then I believe that you are correct and we can eliminate that code.
That's good to hear.
--
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 #586: Optimize age_exists function
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on PR #586:
URL: https://github.com/apache/age/pull/586#issuecomment-1400714986
@moeed-k I appreciate that you looked into optimizing this function. However, can you please thoroughly explain why the code that you want to remove, can actually be removed.
--
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 #586: Optimize age_exists function
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on PR #586:
URL: https://github.com/apache/age/pull/586#issuecomment-1402366093
@moeed-k hmmmm,... I see your point. If we can show that a property will only ever be an standard object (AGTV_OBJECT), not anything else, then, yes, that code can be removed.
--
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 merged pull request #586: Optimize age_exists function
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani merged PR #586:
URL: https://github.com/apache/age/pull/586
--
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] moeed-k commented on pull request #586: Optimize age_exists function
Posted by "moeed-k (via GitHub)" <gi...@apache.org>.
moeed-k commented on PR #586:
URL: https://github.com/apache/age/pull/586#issuecomment-1400908540
> @moeed-k I appreciate that you looked into optimizing this function. However, can you please thoroughly explain why the code that you want to remove, can actually be removed.
Yes of course. According to my understanding, all properties in AGE can either have a NULL value, or *some* actual value, and the exists(property) function only serves to check whether this value is NULL (returns false) or not (returns true). I was testing around the code in the debugger and I realized that whenever the property of a function is NULL (e.g. prop1: null), it is always filtered out by line number 5464 ( PG_ARGISNULL(0) ). The macro returns true, and the function returns false in the next line.
The next few lines involve getting the agtype root node, checking if it is a scalar, getting the value of the root if it _is_ a scalar, and then finally accessing the value type to check if it is NULL or not. However, since all NULL values were already filtered out at line 5464, this last check is never actually true (shown below).
```
if (agtv_value->type == AGTV_NULL)
PG_RETURN_BOOL(false);
```
In all my testing, I was unable to make this condition true. What this does mean, however, is that whenever there is a non-null scalar value passed to exists(), it goes through the trouble of making all those redundant checks to verify the scalar's value type.
The regression tests didn't flag any errors, and my own testing was also yielding the same output as before.
--
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 #586: Optimize age_exists function
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on PR #586:
URL: https://github.com/apache/age/pull/586#issuecomment-1400949029
@moeed-k I should point out that one should be very careful about **using existing regression tests as proof that a change doesn't produce an issue or an error**. There have been a few cases where code was accidentally removed for a particular functionality that wasn't covered with an existing regression test. Later on, users found those functionalities to be missing and created issues for them.
As an AGTYPE value can be AGTV_NULL, which is a NULL, we can't ignore that possibility from occurring.
--
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] moeed-k commented on pull request #586: Optimize age_exists function
Posted by "moeed-k (via GitHub)" <gi...@apache.org>.
moeed-k commented on PR #586:
URL: https://github.com/apache/age/pull/586#issuecomment-1402809363
@jrgemignani I went through the code and here's what I found.
Firstly, properties only ever up show up as AGTV_OBJECT types. The following code is always triggered (line 2913):
```
/* if we were passed an agtype_value OBJECT */
else if (map_value != NULL && map_value->type == AGTV_OBJECT)
{
map_value = get_agtype_value_object_value(map_value, key, key_len);
}
```
Second, the returned map_value is:
1. NULL if the property doesn't exist at all.
2. AGTV_NULL if the property exists but the value is stored as a NULL.
I believe line 3461 is the code that makes the difference in the exists function
```
/* for NULL values return NULL */
if (object_value == NULL || object_value->type == AGTV_NULL)
{
PG_RETURN_NULL();
}
```
We can see from here that regardless of whether the property exists or not (Case 1), or if it exists as an AGTV_NULL (Case 2), the result is returned as a NULL. Thus the age_exists function receives NULL for both cases, and performs the same behavior for both cases.
As such, checking for the AGTV_NULL value inside the age_exists function is unnecessary - it was already checked earlier.
As for your question, I'm not entirely sure, but I am inclined to believe that an empty object should be NULL. At least semantically, that should be true since properties that are empty are non-existent i.e NULL. Properties can have length 0, but that is not the same as being empty. Here's something I found on the Neo4j docs:
https://neo4j.com/developer/kb/understanding-non-existent-properties-and-null-values/
--
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 #586: Optimize age_exists function
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on PR #586:
URL: https://github.com/apache/age/pull/586#issuecomment-1402928480
@moeed-k Then I believe that you are correct and we can eliminate that code.
--
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 #586: Optimize age_exists function
Posted by "jrgemignani (via GitHub)" <gi...@apache.org>.
jrgemignani commented on PR #586:
URL: https://github.com/apache/age/pull/586#issuecomment-1402403054
@moeed-k Another issue is that an object appears to always be created, **empty** if nothing is passed. So, we probably need to re-think how the exists function should work. Thoughts?
```
Datum _agtype_build_vertex(PG_FUNCTION_ARGS)
{
...
//if the properties object is null, push an empty object
if (fcinfo->args[2].isnull)
{
result.res = push_agtype_value(&result.parse_state, WAGT_BEGIN_OBJECT,
NULL);
result.res = push_agtype_value(&result.parse_state, WAGT_END_OBJECT,
NULL);
}
Datum _agtype_build_edge(PG_FUNCTION_ARGS)
{
...
/* if the properties object is null, push an empty object */
if (fcinfo->args[4].isnull)
{
result.res = push_agtype_value(&result.parse_state, WAGT_BEGIN_OBJECT,
NULL);
result.res = push_agtype_value(&result.parse_state, WAGT_END_OBJECT,
NULL);
}
```
--
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] moeed-k commented on pull request #586: Optimize age_exists function
Posted by "moeed-k (via GitHub)" <gi...@apache.org>.
moeed-k commented on PR #586:
URL: https://github.com/apache/age/pull/586#issuecomment-1401117782
@jrgemignani Thank you for the tip about regression tests. I'll keep that in mind going forward.
Perhaps there is something I'm missing here, but if we consider a graph with the following nodes:
```
{"id": 844424930131980, "label": "person", "properties": {"name": "A", "eyes": null}}::vertex
{"id": 844424930132014, "label": "person", "properties": {"name": "B"}}::vertex
```
And then the following is query is run:
```
MATCH (p:person)
WHERE NOT exists(p.eyes)
RETURN p
```
Both nodes are returned back from line 5464 ( PG_ARGISNULL(0) ) . For person B it is obvious since the eyes property doesn't exist, but for person A the property exists and is stored as a null value. Doesn't this imply that it is returned early despite having an AGTV_NULL type?
--
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