Fix Issue # 1159: Server terminates for SET plus-equal

[Zainab-Saad]

This PR is a fix for Issue #1159

[rafsun42]

@Zainab-Saad

1. Could this be solved without using the function agtype_value_to_agtype?
2. Does this issue also appear in other part of the codebase? It seems the root of the issue is pushing an object type with push_agtype_value is prevented by Assert function. Without assertion check, the issue disappears without any patch.

[Zainab-Saad]

@rafsun42

1. agtype_value_to_agtype is used to get agtype to initialize the iterator
2. This is what I thought at first to remove the Assertion, but keeping this assertion makes more sense to me because if the value is an object it should not be pushed right away as we might be missing other checks.
   For example, the assertions for WAGT_KEY, WAGT_ELEM (in case of arrays) etc. So any non-scalar should be iterated through/unpacked when making an agtype_value.
   The approach I have used here for copying the properties is the same used for appending new properties into the parsed_agtype_value in alter_properties() and the later one did not produce any bug for non-scalar values because it treats non-scalar values as AGTV_BINARY which in push_agtype_value is not directly passed to push_agtype_value_scalar to be pushed into agtype_value rather is iterated through and sent to push_agtype_value_scalar for pushing to agtype_value.

P.S: The description for push_agtype_value_scalar is

/*
 * Do the actual pushing, with only scalar or pseudo-scalar-array values
 * accepted.
 */

and for push_agtype_value is

/*
...
 * Values of type AGTV_BINARY, which are rolled up arrays and objects,
 * are unpacked before being added to the result.
 */

In this case, this solution should work. What do you think?

[rafsun42]

@Zainab-Saad
Appending new properties uses this approach because new properties is already in agtype format. However, original properties is not. Can it be done without converting it to an agtype format? This can be expensive.

[Zainab-Saad]

@rafsun42
Mostly such implementations are done with agtype_value as iterator needs this format.
Is writing our own such implementation with agtype (without iterator) an option? since I have not found any such case yet in the codebase

[rafsun42]

@Zainab-Saad
MATCH (n) CREATE (x {prop: n}) RETURN x
You can look into how n (which is a vertex json object) gets inserted into the x's properties?

[Zainab-Saad]

@rafsun42
Are you referring to agtype_build_map_nonull/agtype_build_map ? Because this is the function that would make the properties for the vertex to be created.
These functions utilize datum_to_agtype which given a datum converts in agtype_value format to be added to agtype_in_state

[Zainab-Saad]

@rafsun42
As far as I have researched, such implementations are done with the extra serialization step when the deserialized agtype_value is provided or in the other case agtype structure itself is provided. alter_property_value is an example for this.
So for doing this directly with agtype_value, I have written a helper function that recursively copies the values and works with the non-scalar values i.e; objects and arrays as well (I have tested with the added tests in cypher_set.sql in this PR)

agtype_value *alter_properties(agtype_value *original_properties,
                               agtype *new_properties)
{
    ...
    parsed_agtype_value = push_agtype_value(&parse_state, WAGT_BEGIN_OBJECT,
                                            NULL);
    // Copy original properties.
    if (original_properties)
    {
        if (original_properties->type != AGTV_OBJECT)
        {
            ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                            errmsg("a map is expected")));
        }

        copy_original_properties(parse_state, original_properties,
                                &parsed_agtype_value, true);
    }
...
}

This one might need some refactoring

/*
 * helper function to copy the original properties for SET plus-equal
 * given the original properties are in agtype_value format
 * this function helps skip the serialization step
 */
void copy_original_properties(agtype_parse_state* pstate, agtype_value* props,
                            agtype_value **copied_properties, bool is_top_level)
{
    int i = 0;

    check_stack_depth();

    if (props->type == AGTV_OBJECT)
    {
        if (!is_top_level)
        {
            *copied_properties = push_agtype_value(&pstate, WAGT_BEGIN_OBJECT,
                                                    NULL);
        }

        for(; i < props->val.object.num_pairs; i ++)
        {
            agtype_pair *pair = props->val.object.pairs + i;
            *copied_properties = push_agtype_value(&pstate, WAGT_KEY,
                                                    &pair->key);

            if (IS_A_AGTYPE_SCALAR(&pair->value))
            {
                *copied_properties = push_agtype_value(&pstate, WAGT_VALUE,
                                                    &pair->value);
            }
            else
            {
                copy_original_properties(pstate, &pair->value,
                                        copied_properties, false);
            }
        }

        if (!is_top_level)
        {
            *copied_properties = push_agtype_value(&pstate, WAGT_END_OBJECT,
                                                    NULL);
        }
    }
    else if (props->type == AGTV_ARRAY)
    {
        *copied_properties = push_agtype_value(&pstate, WAGT_BEGIN_ARRAY,
                                                    NULL);

        for (; i < props->val.array.num_elems; i++)
        {
            agtype_value elem = props->val.array.elems[i];

            if (IS_A_AGTYPE_SCALAR(&elem))
            {
                *copied_properties = push_agtype_value(&pstate, WAGT_ELEM,
                                                    &elem);
            }
            else
            {
                copy_original_properties(pstate, &elem,
                                        copied_properties, false);
            }
        }

        *copied_properties = push_agtype_value(&pstate, WAGT_END_ARRAY,
                                                    NULL);
    }
    else
    {
        ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                            errmsg("invalid type provided")));
    }
}

[rafsun42]

@Zainab-Saad If it passes your tests, it should solve the issue?

Only one suggestion, should we give copy_original_properties a more generic name (i.e. copy_agtype_value), since it can be used to copy any agtype_value object (not just properties in SET clause).

[Zainab-Saad]

@rafsun42
Thanks for the suggestion.
I have updated the PR

[rafsun42]

@Zainab-Saad Could you please create this PR for other branches as well?