[GitHub] [tinkerpop] spmallette commented on pull request #1076: TINKERPOP-2112 Fold property() so that T values can work in any order

["spmallette (GitHub)" <gi...@apache.org>]

Here's what is weird when I tried this kind of thing when i was working on the PR. We basically want to fold any `single` cardinality key/value pair, so for that the logic would be:

```java
        if ((endStep instanceof AddVertexStep || endStep instanceof AddEdgeStep ||
                endStep instanceof AddVertexStartStep || endStep instanceof AddEdgeStartStep) &&
                keyValues.length == 0 &&
                (key instanceof T || (key instanceof String &&
                        ((cardinality == VertexProperty.Cardinality.single) ||
                                (null == cardinality && asAdmin().getGraph().orElse(EmptyGraph.instance()).
                                            features().vertex().getCardinality((String) key) == VertexProperty.Cardinality.single))))) {
            ((Mutating) endStep).addPropertyMutations(key, value);
        } else {
            this.asAdmin().addStep(new AddPropertyStep(this.asAdmin(), cardinality, key, value));
            ((AddPropertyStep) this.asAdmin().getEndStep()).addPropertyMutations(keyValues);
        }
```

The problem, which will occur for graphs like TinkerGraph which don't have a schema, is that:

```text
gremlin> g.addV().property(single, 'k',1).property(single,'k',2).profile()
==>Traversal Metrics
Step                                                               Count  Traversers       Time (ms)    % Dur
=============================================================================================================
AddVertexStartStep({k=[1, 2]})                                         1           1           0.138   100.00
                                            >TOTAL                     -           -           0.138        -
gremlin> g.addV().property(single, 'k',1).property(single,'k',2).valueMap()
==>[k:[1,2]]
```

If the user specifies the `single` and the key/values gets folded, the specified cardinality is lost and the graph just does whatever it wants with with the arguments given to `Graph.addVertex()`. For TinkerGraph, its default cardinality is `list` so despite the user saying otherwise, it goes in as `list`. Because the old logic uses `AddPropertyStep` the cardinality is preserved as an argument and things work according to how the user specified them. 

I also tried this logic (which only takes into account the cardinality check you suggested):

```java
        if ((endStep instanceof AddVertexStep || endStep instanceof AddEdgeStep ||
                endStep instanceof AddVertexStartStep || endStep instanceof AddEdgeStartStep) &&
                keyValues.length == 0 &&
                (key instanceof T || (key instanceof String &&
                                (null == cardinality && asAdmin().getGraph().orElse(EmptyGraph.instance()).
                                            features().vertex().getCardinality((String) key) == VertexProperty.Cardinality.single)))) {
            ((Mutating) endStep).addPropertyMutations(key, value);
        } else {
            this.asAdmin().addStep(new AddPropertyStep(this.asAdmin(), cardinality, key, value));
            ((AddPropertyStep) this.asAdmin().getEndStep()).addPropertyMutations(keyValues);
        }
```

and that seemed to work a little better as `single` gets preserved properly:

```text
gremlin> g.addV().property(single, 'k',1).property(single,'k',2).profile()
==>Traversal Metrics
Step                                                               Count  Traversers       Time (ms)    % Dur
=============================================================================================================
AddVertexStartStep({})                                                 1           1           0.081    31.67
AddPropertyStep({key=[k], value=[1]})                                  1           1           0.089    34.66
AddPropertyStep({key=[k], value=[2]})                                  1           1           0.086    33.67
                                            >TOTAL                     -           -           0.257        -
gremlin> 
gremlin> g = TinkerGraph.open().traversal()
==>graphtraversalsource[tinkergraph[vertices:0 edges:0], standard]
gremlin> g.addV().property(single, 'k',1).property(single,'k',2).valueMap()
==>[k:[2]]
```

wdyt?



[ Full content available at: https://github.com/apache/tinkerpop/pull/1076 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org