You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by "spmallette (GitHub)" <gi...@apache.org> on 2019/03/01 16:06:15 UTC
[GitHub] [tinkerpop] spmallette opened pull request #1076:
TINKERPOP-2112 Fold property() so that T values can work in any order
Also optimizes a bit to capture additional possible folds to the "AddElementStep" that would have been otherwise missed before. This should enhance performance of certain mutation traversals depending on the positioning and parameterization of the property() steps as more folding will occur.
[ Full content available at: https://github.com/apache/tinkerpop/pull/1076 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] spmallette commented on issue #1076:
TINKERPOP-2112 Fold property() so that T values can work in any order
Posted by "spmallette (GitHub)" <gi...@apache.org>.
this thing is still busted..............neo4j is not happy which means other stuff probably isn't happy.
[ Full content available at: https://github.com/apache/tinkerpop/pull/1076 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] dkuppitz commented on issue #1076:
TINKERPOP-2112 Fold property() so that T values can work in any order
Posted by "dkuppitz (GitHub)" <gi...@apache.org>.
VOTE +1
[ Full content available at: https://github.com/apache/tinkerpop/pull/1076 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] spmallette commented on issue #1076:
TINKERPOP-2112 Fold property() so that T values can work in any order
Posted by "spmallette (GitHub)" <gi...@apache.org>.
Ok - this is finally good now. I rolled back an eager attempt at improvement around cardinality checks on the graph when folding. That proved to be a mess because we don't do a great job enforcing behavior for that in our test suite so every graph is a little different unfortunately. The current approach to folding seems to fix the initial problem reported with `T.id` (and some other noted issues) without breaking existing behavior.
[ Full content available at: https://github.com/apache/tinkerpop/pull/1076 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] dkuppitz commented on pull request #1076:
TINKERPOP-2112 Fold property() so that T values can work in any order
Posted by "dkuppitz (GitHub)" <gi...@apache.org>.
Yeah, the second one looks good, it's in line with what I would expect the outcome to be.
[ Full content available at: https://github.com/apache/tinkerpop/pull/1076 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] spmallette commented on pull request #1076:
TINKERPOP-2112 Fold property() so that T values can work in any order
Posted by "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
[GitHub] [tinkerpop] dkuppitz commented on pull request #1076:
TINKERPOP-2112 Fold property() so that T values can work in any order
Posted by "dkuppitz (GitHub)" <gi...@apache.org>.
I think this part should also take the graph's default cardinality (`graph().features().vertex().getCardinality(key)`) into account.
[ Full content available at: https://github.com/apache/tinkerpop/pull/1076 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org
[GitHub] [tinkerpop] dkuppitz commented on issue #1076:
TINKERPOP-2112 Fold property() so that T values can work in any order
Posted by "dkuppitz (GitHub)" <gi...@apache.org>.
~~VOTE +1~~
[ Full content available at: https://github.com/apache/tinkerpop/pull/1076 ]
This message was relayed via gitbox.apache.org for dev@tinkerpop.apache.org